On Fri, Oct 06, 2023 at 11:47:48AM +0100, Andrew Cooper wrote:
> On 06/10/2023 10:13 am, Roger Pau Monne wrote:
> > XENFEAT_runstate_phys_area is exposed to all architectures, while
> > XENFEAT_vcpu_time_phys_area is currnelty only implemented for x86, and hence
> > the feature flag is also only exposed on x86.
> >
> > Signed-off-by: Roger Pau Monné <[email protected]>
>
> Thanks for doing this.
>
> > ---
> > CHANGELOG.md | 2 ++
> > xen/common/kernel.c | 6 +++++-
> > xen/include/public/features.h | 9 +++++++++
> > 3 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/CHANGELOG.md b/CHANGELOG.md
> > index e33cf4e1b113..41da710426f6 100644
> > --- a/CHANGELOG.md
> > +++ b/CHANGELOG.md
> > @@ -31,6 +31,8 @@ The format is based on [Keep a
> > Changelog](https://keepachangelog.com/en/1.0.0/)
> > - Add Intel Hardware P-States (HWP) cpufreq driver.
> > - On Arm, experimental support for dynamic addition/removal of Xen device
> > tree
> > nodes using a device tree overlay binary (.dtbo).
> > + - Introduce two new hypercalls to map the vCPU runstate and time areas by
> > + physical rather than linear addresses.
>
> I'd suggest linear/virtual here. Linear is the correct term in x86, but
> virtual is the correct term in pretty much every other architecture.
>
> > diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> > index d2a9175aae67..cffb2f14a562 100644
> > --- a/xen/include/public/features.h
> > +++ b/xen/include/public/features.h
> > @@ -111,6 +111,15 @@
> > #define XENFEAT_not_direct_mapped 16
> > #define XENFEAT_direct_mapped 17
> >
> > +/*
> > + * Signal whether the hypervisor implements the following hypercalls:
>
> This is not what the hypervisor implements. It's what the domain is
> permitted to use.
>
> There also needs to be a matching patch to public/vcpu.h to require
> implementations to check for these feature bits before using the new
> hypercalls.
>
> Also,
>
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b8281d7cff9d..df994bd30fd2 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v
> {
> struct vcpu_register_runstate_memory_area area;
>
> + rc = -ENOSYS;
> + if ( 0 /* TODO: Dom's XENFEAT_runstate_phys_area setting */ )
> + break;
> +
> rc = -EFAULT;
> if ( copy_from_guest(&area.addr.p, arg, 1) )
> break;
>
> and a matching one for XENFEAT_vcpu_time_phys_area because I'm even more
> serious about this becoming a domain controllable setting following what
> OSSTest had to say overnight.
While this is all fine, please note that the newly added code
{,un}map_guest_area() is also used by the existing
VCPUOP_register_vcpu_info hypercall, and that one can't be disabled.
IOW: even if we add knobs to make the new hypercalls selectable, most
of the newly added code could still be reached from
VCPUOP_register_vcpu_info.
Thanks, Roger.
