1. Patchew
  2. Xen
  3. [PATCH 0/2] domain: followup for phys address mapping series
  4. View patch

[PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT

Roger Pau Monne posted 2 patches 2 years, 4 months ago
There is a newer version of this series
[PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT
Posted by Roger Pau Monne 2 years, 4 months ago 
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é <roger.pau@citrix.com>
---
 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.
 
 ### Removed
  - On x86, the "pku" command line option has been removed.  It has never
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 52aa28762782..b6302e44b34e 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -607,7 +607,11 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
         switch ( fi.submap_idx )
         {
         case 0:
-            fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
+            fi.submap = (1U << XENFEAT_memory_op_vnode_supported) |
+#ifdef CONFIG_X86
+                        (1U << XENFEAT_vcpu_time_phys_area) |
+#endif
+                        (1U << XENFEAT_runstate_phys_area);
             if ( VM_ASSIST(d, pae_extended_cr3) )
                 fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
             if ( paging_mode_translate(d) )
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:
+ *
+ * VCPUOP_register_runstate_phys_area
+ * VCPUOP_register_vcpu_time_phys_area
+ */
+#define XENFEAT_runstate_phys_area	  18
+#define XENFEAT_vcpu_time_phys_area	  19
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
2.42.0
Re: [PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT
Posted by Andrew Cooper 2 years, 4 months ago 
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é <roger.pau@citrix.com>

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.

~Andrew
Re: [PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT
Posted by Roger Pau Monné 2 years, 4 months ago 
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é <roger.pau@citrix.com>
> 
> 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.
Re: [PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT
Posted by Andrew Cooper 2 years, 4 months ago 
On 06/10/2023 12:02 pm, Roger Pau Monné wrote:
> On Fri, Oct 06, 2023 at 11:47:48AM +0100, Andrew Cooper wrote:
>> 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.

Yeah, I'm aware we're stuck there, but a crap interface from the past is
not an excuse not to do new interfaces properly.

~Andrew
Re: [PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT
Posted by Roger Pau Monné 2 years, 4 months ago 
On Fri, Oct 06, 2023 at 12:19:29PM +0100, Andrew Cooper wrote:
> On 06/10/2023 12:02 pm, Roger Pau Monné wrote:
> > On Fri, Oct 06, 2023 at 11:47:48AM +0100, Andrew Cooper wrote:
> >> 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.
> 
> Yeah, I'm aware we're stuck there, but a crap interface from the past is
> not an excuse not to do new interfaces properly.

Right, want I intended to say is that if we are worried the new
{,un}map_guest_area() might be buggy, and would like to have a way to
disable it, just preventing usage of
VCPUOP_register_{runstate,vcpu_info}_phys_area won't be enough as the
newly introduced function is also used by the existing
VCPUOP_register_vcpu_info.

Not that we shouldn't add the checks, just that those won't cover all
usages of {,un}map_guest_area().

Thanks, Roger.
Re: [PATCH 2/2] domain: expose newly introduced hypercalls as XENFEAT
Posted by Henry Wang 2 years, 4 months ago 
Hi Roger,

> On Oct 6, 2023, at 17:13, Roger Pau Monne <roger.pau@citrix.com> 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é <roger.pau@citrix.com>

Release-acked-by: Henry Wang <Henry.Wang@arm.com>

Kind regards,
Henry

> ---
> 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.
> 
> ### Removed
>  - On x86, the "pku" command line option has been removed.  It has never
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index 52aa28762782..b6302e44b34e 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -607,7 +607,11 @@ long do_xen_version(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>         switch ( fi.submap_idx )
>         {
>         case 0:
> -            fi.submap = (1U << XENFEAT_memory_op_vnode_supported);
> +            fi.submap = (1U << XENFEAT_memory_op_vnode_supported) |
> +#ifdef CONFIG_X86
> +                        (1U << XENFEAT_vcpu_time_phys_area) |
> +#endif
> +                        (1U << XENFEAT_runstate_phys_area);
>             if ( VM_ASSIST(d, pae_extended_cr3) )
>                 fi.submap |= (1U << XENFEAT_pae_pgdir_above_4gb);
>             if ( paging_mode_translate(d) )
> 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:
> + *
> + * VCPUOP_register_runstate_phys_area
> + * VCPUOP_register_vcpu_time_phys_area
> + */
> +#define XENFEAT_runstate_phys_area  18
> +#define XENFEAT_vcpu_time_phys_area  19
> +
> #define XENFEAT_NR_SUBMAPS 1
> 
> #endif /* __XEN_PUBLIC_FEATURES_H__ */
> -- 
> 2.42.0
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.