[PATCH 0/2] domain: followup for phys address mapping series

[PATCH 0/2] domain: followup for phys address mapping series

[Roger Pau Monne]

Hello,

First patch fixes a bug reported by osstest, second patch is an addition
requested by Andrew, I think this should remove the blocker raised
against the series.

Roger Pau Monne (2):
  domain: fix misaligned unmap address in unmap_guest_area()
  domain: expose newly introduced hypercalls as XENFEAT

 CHANGELOG.md                  | 2 ++
 xen/common/domain.c           | 2 +-
 xen/common/kernel.c           | 6 +++++-
 xen/include/public/features.h | 9 +++++++++
 4 files changed, 17 insertions(+), 2 deletions(-)

-- 
2.42.0

[PATCH 1/2] domain: fix misaligned unmap address in unmap_guest_area()

[Roger Pau Monne]

unmap_domain_page_global() expects the provided address to be page aligned, or
else some of the called functions will trigger assertions, like
modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm.

The following assert has been reported by osstest arm 32bit tests:

(XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243
(XEN) ----[ Xen-4.18-rc  arm32  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     00271a38 destroy_xen_mappings+0x50/0x5c
[...]
(XEN) Xen call trace:
(XEN)    [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC)
(XEN)    [<00235aa8>] vunmap+0x30/0x1a0 (LR)
(XEN)    [<0026ad88>] unmap_domain_page_global+0x10/0x20
(XEN)    [<00208e38>] unmap_guest_area+0x90/0xec
(XEN)    [<00208f98>] domain_kill+0x104/0x180
(XEN)    [<00239e3c>] do_domctl+0x8ac/0x14fc
(XEN)    [<0027ae34>] do_trap_guest_sync+0x570/0x66c
(XEN)    [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4

Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
unmap_domain_page_global() and vunmap() should likely have the same alignment
asserts, as not all paths lead to detecting the misalignment of the provided
linear address.  Will do a separate patch.
---
 xen/common/domain.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index b8281d7cff9d..2dcc64e659cc 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area)
 
     if ( pg )
     {
-        unmap_domain_page_global(map);
+        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
         put_page_and_type(pg);
     }
 }
-- 
2.42.0

Re: [PATCH 1/2] domain: fix misaligned unmap address in unmap_guest_area()

[Henry Wang]

Hi Roger,

> On Oct 6, 2023, at 17:13, Roger Pau Monne <roger.pau@citrix.com> wrote:
> 
> unmap_domain_page_global() expects the provided address to be page aligned, or
> else some of the called functions will trigger assertions, like
> modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm.
> 
> The following assert has been reported by osstest arm 32bit tests:
> 
> (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243
> (XEN) ----[ Xen-4.18-rc  arm32  debug=y  Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     00271a38 destroy_xen_mappings+0x50/0x5c
> [...]
> (XEN) Xen call trace:
> (XEN)    [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC)
> (XEN)    [<00235aa8>] vunmap+0x30/0x1a0 (LR)
> (XEN)    [<0026ad88>] unmap_domain_page_global+0x10/0x20
> (XEN)    [<00208e38>] unmap_guest_area+0x90/0xec
> (XEN)    [<00208f98>] domain_kill+0x104/0x180
> (XEN)    [<00239e3c>] do_domctl+0x8ac/0x14fc
> (XEN)    [<0027ae34>] do_trap_guest_sync+0x570/0x66c
> (XEN)    [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4
> 
> Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Kind regards,
Henry

> ---
> unmap_domain_page_global() and vunmap() should likely have the same alignment
> asserts, as not all paths lead to detecting the misalignment of the provided
> linear address.  Will do a separate patch.
> ---
> xen/common/domain.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b8281d7cff9d..2dcc64e659cc 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area)
> 
>     if ( pg )
>     {
> -        unmap_domain_page_global(map);
> +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
>         put_page_and_type(pg);
>     }
> }
> -- 
> 2.42.0
> 

Re: [PATCH 1/2] domain: fix misaligned unmap address in unmap_guest_area()

[Julien Grall]

Hi Roger,

On 06/10/2023 10:13, Roger Pau Monne wrote:
> unmap_domain_page_global() expects the provided address to be page aligned, or
> else some of the called functions will trigger assertions, like
> modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm.
> 
> The following assert has been reported by osstest arm 32bit tests:
> 
> (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243
> (XEN) ----[ Xen-4.18-rc  arm32  debug=y  Not tainted ]----
> (XEN) CPU:    0
> (XEN) PC:     00271a38 destroy_xen_mappings+0x50/0x5c
> [...]
> (XEN) Xen call trace:
> (XEN)    [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC)
> (XEN)    [<00235aa8>] vunmap+0x30/0x1a0 (LR)
> (XEN)    [<0026ad88>] unmap_domain_page_global+0x10/0x20
> (XEN)    [<00208e38>] unmap_guest_area+0x90/0xec
> (XEN)    [<00208f98>] domain_kill+0x104/0x180
> (XEN)    [<00239e3c>] do_domctl+0x8ac/0x14fc
> (XEN)    [<0027ae34>] do_trap_guest_sync+0x570/0x66c
> (XEN)    [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4
> 
> Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas')
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> unmap_domain_page_global() and vunmap() should likely have the same alignment
> asserts, as not all paths lead to detecting the misalignment of the provided
> linear address.  Will do a separate patch.

unmap_domain_page() seems to be able to deal with unaligned pointer. And 
supporting unaligned pointer in vunmap()/unmap_domain_page_global() 
would simplifyy call to those functions.

So I would consider to deal with the alignment rather than adding 
ASSERT() in the two functions. destroy_xen_mappings() and 
modify_xen_mappings() should stay as-is though.

Anyway, I don't think this is a 4.18 material.

> ---
>   xen/common/domain.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b8281d7cff9d..2dcc64e659cc 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area)
>   
>       if ( pg )
>       {
> -        unmap_domain_page_global(map);
> +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));

Looking at the code, I can't find where 'map' may have become unaligned. 
Do you have a pointer?

Depending on the answer, the call in map_guest_area() may also need some 
adjustment.

Cheers,

-- 
Julien Grall

Re: [PATCH 1/2] domain: fix misaligned unmap address in unmap_guest_area()

[Roger Pau Monné]

On Fri, Oct 06, 2023 at 11:08:09AM +0100, Julien Grall wrote:
> Hi Roger,
> 
> On 06/10/2023 10:13, Roger Pau Monne wrote:
> > unmap_domain_page_global() expects the provided address to be page aligned, or
> > else some of the called functions will trigger assertions, like
> > modify_xen_mappings() on x86 or destroy_xen_mappings() on Arm.
> > 
> > The following assert has been reported by osstest arm 32bit tests:
> > 
> > (XEN) Assertion 'IS_ALIGNED(s, PAGE_SIZE)' failed at arch/arm/mm.c:1243
> > (XEN) ----[ Xen-4.18-rc  arm32  debug=y  Not tainted ]----
> > (XEN) CPU:    0
> > (XEN) PC:     00271a38 destroy_xen_mappings+0x50/0x5c
> > [...]
> > (XEN) Xen call trace:
> > (XEN)    [<00271a38>] destroy_xen_mappings+0x50/0x5c (PC)
> > (XEN)    [<00235aa8>] vunmap+0x30/0x1a0 (LR)
> > (XEN)    [<0026ad88>] unmap_domain_page_global+0x10/0x20
> > (XEN)    [<00208e38>] unmap_guest_area+0x90/0xec
> > (XEN)    [<00208f98>] domain_kill+0x104/0x180
> > (XEN)    [<00239e3c>] do_domctl+0x8ac/0x14fc
> > (XEN)    [<0027ae34>] do_trap_guest_sync+0x570/0x66c
> > (XEN)    [<002019f0>] arch/arm/arm32/entry.o#return_from_trap+0/0x4
> > 
> > Fixes: eadc288cbb0d ('domain: map/unmap GADDR based shared guest areas')
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > unmap_domain_page_global() and vunmap() should likely have the same alignment
> > asserts, as not all paths lead to detecting the misalignment of the provided
> > linear address.  Will do a separate patch.
> 
> unmap_domain_page() seems to be able to deal with unaligned pointer. And
> supporting unaligned pointer in vunmap()/unmap_domain_page_global() would
> simplifyy call to those functions.
> 
> So I would consider to deal with the alignment rather than adding ASSERT()
> in the two functions. destroy_xen_mappings() and modify_xen_mappings()
> should stay as-is though.
> 
> Anyway, I don't think this is a 4.18 material.

Maybe, I don't really have a strong opinion.  It seems more sane to me
to expect a page aligned linear address if the function is unmapping a
page, leaves less room for misuse IMO.

> > ---
> >   xen/common/domain.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/common/domain.c b/xen/common/domain.c
> > index b8281d7cff9d..2dcc64e659cc 100644
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1634,7 +1634,7 @@ void unmap_guest_area(struct vcpu *v, struct guest_area *area)
> >       if ( pg )
> >       {
> > -        unmap_domain_page_global(map);
> > +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
> 
> Looking at the code, I can't find where 'map' may have become unaligned. Do
> you have a pointer?

In map_guest_area(), there's:

if ( ~gaddr ) /* Map (i.e. not just unmap)? */
{
    [...]
    map = __map_domain_page_global(pg);
    if ( !map )
    {
        put_page_and_type(pg);
        return -ENOMEM;
    }
    map += PAGE_OFFSET(gaddr);
}

> Depending on the answer, the call in map_guest_area() may also need some
> adjustment.

Indeed, I've missed that one, let me spin v2.

Thanks, Roger.

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

[Roger Pau Monne]

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

[Henry Wang]

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
> 

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

[Andrew Cooper]

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

[Roger Pau Monné]

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

[Andrew Cooper]

[-- Attachment #1: Type: text/plain, Size: 1224 bytes --]

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

[-- Attachment #2: Type: text/html, Size: 1728 bytes --]

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

[Roger Pau Monné]

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.