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

[PATCH v2 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 {,un}map_guest_area()
  domain: expose newly introduced hypercalls as XENFEAT

 CHANGELOG.md                  | 2 ++
 xen/arch/x86/domain.c         | 4 ++++
 xen/common/domain.c           | 8 ++++++--
 xen/common/kernel.c           | 6 +++++-
 xen/include/public/features.h | 9 +++++++++
 xen/include/public/vcpu.h     | 3 +++
 6 files changed, 29 insertions(+), 3 deletions(-)

-- 
2.42.0

[PATCH v2 1/2] domain: fix misaligned unmap address in {,un}map_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>
---
Changes since v1:
 - Also page-align the address in map_guest_area().
---
 xen/common/domain.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/common/domain.c b/xen/common/domain.c
index b8281d7cff9d..1468638ade8b 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
  unmap:
     if ( pg )
     {
-        unmap_domain_page_global(map);
+        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
         put_page_and_type(pg);
     }
 
@@ -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 v2 1/2] domain: fix misaligned unmap address in {,un}map_guest_area()

[Henry Wang]

Hi Roger,

> On Oct 6, 2023, at 21:00, 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

> ---
> Changes since v1:
> - Also page-align the address in map_guest_area().
> ---
> xen/common/domain.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/common/domain.c b/xen/common/domain.c
> index b8281d7cff9d..1468638ade8b 100644
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
>  unmap:
>     if ( pg )
>     {
> -        unmap_domain_page_global(map);
> +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
>         put_page_and_type(pg);
>     }
> 
> @@ -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 v2 1/2] domain: fix misaligned unmap address in {,un}map_guest_area()

[Julien Grall]

Hi Roger,

On 06/10/2023 14:00, 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>

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v2 1/2] domain: fix misaligned unmap address in {,un}map_guest_area()

[Jan Beulich]

On 06.10.2023 15:00, Roger Pau Monne wrote:> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
>   unmap:
>      if ( pg )
>      {
> -        unmap_domain_page_global(map);
> +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
>          put_page_and_type(pg);
>      }
>  
> @@ -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);
>      }
>  }

On v1 in a reply to Julien you talk of "limiting misuse" by not relaxing
expecations in Arm's backing code, but I wonder what kind of misuse you
think about. Aiui there's no strong need to insist on page aligned input,
and relaxing things there may simplify code elsewhere as well.

Jan

Re: [PATCH v2 1/2] domain: fix misaligned unmap address in {,un}map_guest_area()

[Roger Pau Monné]

On Mon, Oct 16, 2023 at 02:30:12PM +0200, Jan Beulich wrote:
> On 06.10.2023 15:00, Roger Pau Monne wrote:> --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1601,7 +1601,7 @@ int map_guest_area(struct vcpu *v, paddr_t gaddr, unsigned int size,
> >   unmap:
> >      if ( pg )
> >      {
> > -        unmap_domain_page_global(map);
> > +        unmap_domain_page_global((void *)((unsigned long)map & PAGE_MASK));
> >          put_page_and_type(pg);
> >      }
> >  
> > @@ -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);
> >      }
> >  }
> 
> On v1 in a reply to Julien you talk of "limiting misuse" by not relaxing
> expecations in Arm's backing code, but I wonder what kind of misuse you
> think about. Aiui there's no strong need to insist on page aligned input,
> and relaxing things there may simplify code elsewhere as well.

destroy_xen_mappings() both on Arm and x86 will trigger asserts if the
passed address is not page aligned.  I do think it makes sense to call
unmap_domain_page_global() with page-aligned addresses, as that could
help detect bogus callers or corrupted data passed as input.

IMO an assert for page aligned input address should be placed at
vunmap() in order to not get differing expectations on input address
being page aligned or not whether destroy_xen_mappings() or
map_pages_to_xen() is used.  map_pages_to_xen() doesn't require
page-aligned virtual addresses as input.

Roger.

[PATCH v2 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 currently only implemented for x86, and hence
the feature flag is also only exposed on x86.

Additionally add dummy guards with TODOs in the respective hypercall
implementations, to signal the intention to control the availability of those
hypercalls on a guest-by-guest basis from the toolstack.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - Adjust comments, and add new one in vcpu.h
 - Add dummy guard with TDOD in hypercall implementations.
---
 CHANGELOG.md                  | 2 ++
 xen/arch/x86/domain.c         | 4 ++++
 xen/common/domain.c           | 4 ++++
 xen/common/kernel.c           | 6 +++++-
 xen/include/public/features.h | 9 +++++++++
 xen/include/public/vcpu.h     | 3 +++
 6 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e33cf4e1b113..47ea9e275462 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/virtual addresses.
 
 ### Removed
  - On x86, the "pku" command line option has been removed.  It has never
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8e0af2278104..8d3d52034a6d 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1580,6 +1580,10 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
         struct vcpu_register_time_memory_area area;
 
+        rc = -ENOSYS;
+        if ( 0 /* TODO: Dom's XENFEAT_vcpu_time_phys_area setting */ )
+            break;
+
         rc = -EFAULT;
         if ( copy_from_guest(&area.addr.p, arg, 1) )
             break;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 1468638ade8b..8f9ab01c0cb7 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
     {
         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;
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..22713a51b520 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 domain is permitted to use 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__ */
diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
index 8fb0bd1b6c03..03b031a3e557 100644
--- a/xen/include/public/vcpu.h
+++ b/xen/include/public/vcpu.h
@@ -236,6 +236,9 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
  * Note that the area registered via VCPUOP_register_runstate_memory_area will
  * be updated in the same manner as the one registered via virtual address PLUS
  * VMASST_TYPE_runstate_update_flag engaged by the domain.
+ *
+ * XENFEAT_{runstate,vcpu_time}_phys_area feature bits signal if the domain is
+ * permitted the usage of the hypercalls.
  */
 #define VCPUOP_register_runstate_phys_area      14
 #define VCPUOP_register_vcpu_time_phys_area     15
-- 
2.42.0

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

[Andrew Cooper]

On 06/10/2023 2:00 pm, Roger Pau Monne wrote:
> diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> index d2a9175aae67..22713a51b520 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 domain is permitted to use 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__ */
> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> index 8fb0bd1b6c03..03b031a3e557 100644
> --- a/xen/include/public/vcpu.h
> +++ b/xen/include/public/vcpu.h
> @@ -236,6 +236,9 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>   * Note that the area registered via VCPUOP_register_runstate_memory_area will
>   * be updated in the same manner as the one registered via virtual address PLUS
>   * VMASST_TYPE_runstate_update_flag engaged by the domain.
> + *
> + * XENFEAT_{runstate,vcpu_time}_phys_area feature bits signal if the domain is
> + * permitted the usage of the hypercalls.
>   */
>  #define VCPUOP_register_runstate_phys_area      14
>  #define VCPUOP_register_vcpu_time_phys_area     15

For both of these, I'd suggest s/permitted/able/.  For older versions of
Xen which don't advertise the XENFEAT, it's a matter of capability, not
permission.

Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and
I'm happy to adjust on commit to save sending out a v3.

Thanks,

~Andrew

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

[Roger Pau Monné]

On Fri, Oct 06, 2023 at 02:05:18PM +0100, Andrew Cooper wrote:
> On 06/10/2023 2:00 pm, Roger Pau Monne wrote:
> > diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> > index d2a9175aae67..22713a51b520 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 domain is permitted to use 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__ */
> > diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> > index 8fb0bd1b6c03..03b031a3e557 100644
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -236,6 +236,9 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >   * Note that the area registered via VCPUOP_register_runstate_memory_area will
> >   * be updated in the same manner as the one registered via virtual address PLUS
> >   * VMASST_TYPE_runstate_update_flag engaged by the domain.
> > + *
> > + * XENFEAT_{runstate,vcpu_time}_phys_area feature bits signal if the domain is
> > + * permitted the usage of the hypercalls.
> >   */
> >  #define VCPUOP_register_runstate_phys_area      14
> >  #define VCPUOP_register_vcpu_time_phys_area     15
> 
> For both of these, I'd suggest s/permitted/able/.  For older versions of
> Xen which don't advertise the XENFEAT, it's a matter of capability, not
> permission.
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and
> I'm happy to adjust on commit to save sending out a v3.

TBH I've used permitted because that's the wording you used in your
reply to v1, I'm perfectly fine with switching to able.

https://lore.kernel.org/xen-devel/ac4842a9-7f62-4c64-9a3a-2ec2b1e97699@citrix.com/

Thanks, Roger.

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

[Andrew Cooper]

On 06/10/2023 2:19 pm, Roger Pau Monné wrote:
> On Fri, Oct 06, 2023 at 02:05:18PM +0100, Andrew Cooper wrote:
>> On 06/10/2023 2:00 pm, Roger Pau Monne wrote:
>>> diff --git a/xen/include/public/features.h b/xen/include/public/features.h
>>> index d2a9175aae67..22713a51b520 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 domain is permitted to use 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__ */
>>> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
>>> index 8fb0bd1b6c03..03b031a3e557 100644
>>> --- a/xen/include/public/vcpu.h
>>> +++ b/xen/include/public/vcpu.h
>>> @@ -236,6 +236,9 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>>   * Note that the area registered via VCPUOP_register_runstate_memory_area will
>>>   * be updated in the same manner as the one registered via virtual address PLUS
>>>   * VMASST_TYPE_runstate_update_flag engaged by the domain.
>>> + *
>>> + * XENFEAT_{runstate,vcpu_time}_phys_area feature bits signal if the domain is
>>> + * permitted the usage of the hypercalls.
>>>   */
>>>  #define VCPUOP_register_runstate_phys_area      14
>>>  #define VCPUOP_register_vcpu_time_phys_area     15
>> For both of these, I'd suggest s/permitted/able/.  For older versions of
>> Xen which don't advertise the XENFEAT, it's a matter of capability, not
>> permission.
>>
>> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and
>> I'm happy to adjust on commit to save sending out a v3.
> TBH I've used permitted because that's the wording you used in your
> reply to v1, I'm perfectly fine with switching to able.
>
> https://lore.kernel.org/xen-devel/ac4842a9-7f62-4c64-9a3a-2ec2b1e97699@citrix.com/

Yeah, sorry.  I didn't think fully before making the suggestion.

I was mainly looking to avoid saying "what the hypervisor is capable of" :)

~Andrew

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

[Henry Wang]

Hi,

> On Oct 6, 2023, at 21:05, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> 
> On 06/10/2023 2:00 pm, Roger Pau Monne wrote:
>> diff --git a/xen/include/public/features.h b/xen/include/public/features.h
>> index d2a9175aae67..22713a51b520 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 domain is permitted to use 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__ */
>> diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
>> index 8fb0bd1b6c03..03b031a3e557 100644
>> --- a/xen/include/public/vcpu.h
>> +++ b/xen/include/public/vcpu.h
>> @@ -236,6 +236,9 @@ DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
>>  * Note that the area registered via VCPUOP_register_runstate_memory_area will
>>  * be updated in the same manner as the one registered via virtual address PLUS
>>  * VMASST_TYPE_runstate_update_flag engaged by the domain.
>> + *
>> + * XENFEAT_{runstate,vcpu_time}_phys_area feature bits signal if the domain is
>> + * permitted the usage of the hypercalls.
>>  */
>> #define VCPUOP_register_runstate_phys_area      14
>> #define VCPUOP_register_vcpu_time_phys_area     15
> 
> For both of these, I'd suggest s/permitted/able/.  For older versions of
> Xen which don't advertise the XENFEAT, it's a matter of capability, not
> permission.
> 
> Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> and
> I'm happy to adjust on commit to save sending out a v3.

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

Kind regards,
Henry

> 
> Thanks,
> 
> ~Andrew

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

[Jan Beulich]

On 06.10.2023 15:00, Roger Pau Monne wrote:
> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1580,6 +1580,10 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
>      {
>          struct vcpu_register_time_memory_area area;
>  
> +        rc = -ENOSYS;
> +        if ( 0 /* TODO: Dom's XENFEAT_vcpu_time_phys_area setting */ )
> +            break;
> +
>          rc = -EFAULT;
>          if ( copy_from_guest(&area.addr.p, arg, 1) )
>              break;
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>      {
>          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;

ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
correct.

> --- 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);

No provisions here for the "disabled for this domain" case?

Jan

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

[Roger Pau Monné]

On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
> On 06.10.2023 15:00, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1580,6 +1580,10 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      {
> >          struct vcpu_register_time_memory_area area;
> >  
> > +        rc = -ENOSYS;
> > +        if ( 0 /* TODO: Dom's XENFEAT_vcpu_time_phys_area setting */ )
> > +            break;
> > +
> >          rc = -EFAULT;
> >          if ( copy_from_guest(&area.addr.p, arg, 1) )
> >              break;
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> >      {
> >          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;
> 
> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
> correct.

I don't think so, common_vcpu_op() default case does return -ENOSYS,
and the point of this path is to mimic the behavior of an hypervisor
that doesn't have the hypercalls implemented, hence -ENOSYS is the
correct behavior.

> 
> > --- 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);
> 
> No provisions here for the "disabled for this domain" case?

TBH I'm not sure the `if ( 0` above are of much help, if we ever want
to provide toolstack overwrites for those it's fairly easy to spot the
paths that need to be patched anyway.

Thanks, Roger.

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

[Jan Beulich]

On 16.10.2023 15:00, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
>> On 06.10.2023 15:00, Roger Pau Monne wrote:
>>> --- a/xen/common/domain.c
>>> +++ b/xen/common/domain.c
>>> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>      {
>>>          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;
>>
>> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
>> correct.
> 
> I don't think so, common_vcpu_op() default case does return -ENOSYS,
> and the point of this path is to mimic the behavior of an hypervisor
> that doesn't have the hypercalls implemented, hence -ENOSYS is the
> correct behavior.

Besides that other ENOSYS being wrong too, I question such "mimic-ing".
Imo error codes should be the best representation of the real reason,
not be arbitrarily "made up".

Jan

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

[Roger Pau Monné]

On Mon, Oct 16, 2023 at 03:58:22PM +0200, Jan Beulich wrote:
> On 16.10.2023 15:00, Roger Pau Monné wrote:
> > On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
> >> On 06.10.2023 15:00, Roger Pau Monne wrote:
> >>> --- a/xen/common/domain.c
> >>> +++ b/xen/common/domain.c
> >>> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>      {
> >>>          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;
> >>
> >> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
> >> correct.
> > 
> > I don't think so, common_vcpu_op() default case does return -ENOSYS,
> > and the point of this path is to mimic the behavior of an hypervisor
> > that doesn't have the hypercalls implemented, hence -ENOSYS is the
> > correct behavior.
> 
> Besides that other ENOSYS being wrong too, I question such "mimic-ing".
> Imo error codes should be the best representation of the real reason,
> not be arbitrarily "made up".

The point is for the guest to not take any action that it won't take
on an hypervisor that doesn't have the hypercalls implemented, and the
only way to be sure about that is to return the same exact error code.

Thanks, Roger.

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

[Jan Beulich]

On 16.10.2023 16:01, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 03:58:22PM +0200, Jan Beulich wrote:
>> On 16.10.2023 15:00, Roger Pau Monné wrote:
>>> On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
>>>> On 06.10.2023 15:00, Roger Pau Monne wrote:
>>>>> --- a/xen/common/domain.c
>>>>> +++ b/xen/common/domain.c
>>>>> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
>>>>>      {
>>>>>          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;
>>>>
>>>> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
>>>> correct.
>>>
>>> I don't think so, common_vcpu_op() default case does return -ENOSYS,
>>> and the point of this path is to mimic the behavior of an hypervisor
>>> that doesn't have the hypercalls implemented, hence -ENOSYS is the
>>> correct behavior.
>>
>> Besides that other ENOSYS being wrong too, I question such "mimic-ing".
>> Imo error codes should be the best representation of the real reason,
>> not be arbitrarily "made up".
> 
> The point is for the guest to not take any action that it won't take
> on an hypervisor that doesn't have the hypercalls implemented, and the
> only way to be sure about that is to return the same exact error code.

I don't follow this kind of reasoning. Why would a guest, whose code to
use the new sub-functions has to be newly written, key its decision to
the specific error code it gets, when at the same time you expose
feature bits that it can use to decide whether to invoke the hypercall
in the first place.

Jan

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

[Roger Pau Monné]

On Mon, Oct 16, 2023 at 04:04:34PM +0200, Jan Beulich wrote:
> On 16.10.2023 16:01, Roger Pau Monné wrote:
> > On Mon, Oct 16, 2023 at 03:58:22PM +0200, Jan Beulich wrote:
> >> On 16.10.2023 15:00, Roger Pau Monné wrote:
> >>> On Mon, Oct 16, 2023 at 02:35:44PM +0200, Jan Beulich wrote:
> >>>> On 06.10.2023 15:00, Roger Pau Monne wrote:
> >>>>> --- a/xen/common/domain.c
> >>>>> +++ b/xen/common/domain.c
> >>>>> @@ -1998,6 +1998,10 @@ long common_vcpu_op(int cmd, struct vcpu *v, XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>>>      {
> >>>>>          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;
> >>>>
> >>>> ENOSYS is not correct here. EPERM, EACCES, or EOPNOTSUPP would all be more
> >>>> correct.
> >>>
> >>> I don't think so, common_vcpu_op() default case does return -ENOSYS,
> >>> and the point of this path is to mimic the behavior of an hypervisor
> >>> that doesn't have the hypercalls implemented, hence -ENOSYS is the
> >>> correct behavior.
> >>
> >> Besides that other ENOSYS being wrong too, I question such "mimic-ing".
> >> Imo error codes should be the best representation of the real reason,
> >> not be arbitrarily "made up".
> > 
> > The point is for the guest to not take any action that it won't take
> > on an hypervisor that doesn't have the hypercalls implemented, and the
> > only way to be sure about that is to return the same exact error code.
> 
> I don't follow this kind of reasoning. Why would a guest, whose code to
> use the new sub-functions has to be newly written, key its decision to
> the specific error code it gets, when at the same time you expose
> feature bits that it can use to decide whether to invoke the hypercall
> in the first place.

Because we don't control all possible guest implementations out there.

AIUI the point of introducing a way to disable the newly added
hypercalls is to make the interface between a Xen previous to the
introduction of the hypercalls vs a Xen that has the hypercalls
disabled equal, and that requires returning the same error code IMO,
or else interfaces are not equal.

Thanks, Roger.