[PATCH] x86/HVM: expose VM assist hypercall

[PATCH] x86/HVM: expose VM assist hypercall

[Jan Beulich]

In preparation for the addition of VMASST_TYPE_runstate_update_flag
commit 72c538cca957 ("arm: add support for vm_assist hypercall") enabled
the hypercall for Arm. I consider it not logical that it then isn't also
exposed to x86 HVM guests (with the same single feature permitted to be
enabled as Arm has); Linux actually tries to use it afaict.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -78,6 +78,11 @@ static long hvm_grant_table_op(
 }
 #endif
 
+static long hvm_vm_assist(unsigned int cmd, unsigned int type)
+{
+    return vm_assist(current->domain, cmd, type, HVM_VM_ASSIST_VALID);
+}
+
 static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     const struct vcpu *curr = current;
@@ -128,6 +133,7 @@ static const hypercall_table_t hvm_hyper
 #ifdef CONFIG_GRANT_TABLE
     HVM_CALL(grant_table_op),
 #endif
+    HVM_CALL(vm_assist),
     COMPAT_CALL(vcpu_op),
     HVM_CALL(physdev_op),
     COMPAT_CALL(xen_version),
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -319,6 +319,7 @@ extern unsigned long xen_phys_start;
 #define VM_ASSIST_VALID          NATIVE_VM_ASSIST_VALID
 #define COMPAT_VM_ASSIST_VALID   (NATIVE_VM_ASSIST_VALID & \
                                   ((1UL << COMPAT_BITS_PER_LONG) - 1))
+#define HVM_VM_ASSIST_VALID      (1UL << VMASST_TYPE_runstate_update_flag)
 
 #define ELFSIZE 64
 

Re: [PATCH] x86/HVM: expose VM assist hypercall

[Andrew Cooper]

On 02/04/2020 08:46, Jan Beulich wrote:
> In preparation for the addition of VMASST_TYPE_runstate_update_flag
> commit 72c538cca957 ("arm: add support for vm_assist hypercall") enabled
> the hypercall for Arm. I consider it not logical that it then isn't also
> exposed to x86 HVM guests (with the same single feature permitted to be
> enabled as Arm has); Linux actually tries to use it afaict.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'd declare this a bug in 2529c850ea4.  It was clearly intended as a
common feature, and wasn't tested for HVM guests.

However, ...

>
> --- a/xen/arch/x86/hvm/hypercall.c
> +++ b/xen/arch/x86/hvm/hypercall.c
> @@ -78,6 +78,11 @@ static long hvm_grant_table_op(
>  }
>  #endif
>  
> +static long hvm_vm_assist(unsigned int cmd, unsigned int type)
> +{
> +    return vm_assist(current->domain, cmd, type, HVM_VM_ASSIST_VALID);
> +}
> +
>  static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>  {
>      const struct vcpu *curr = current;
> @@ -128,6 +133,7 @@ static const hypercall_table_t hvm_hyper
>  #ifdef CONFIG_GRANT_TABLE
>      HVM_CALL(grant_table_op),
>  #endif
> +    HVM_CALL(vm_assist),

... this means we've now got 3 stub functions making no-op ABI changes
for the general vm_assist() function.

Renaming it to do_vm_assist(), latch current->domain internally, and an
arch_vm_assist_valid(d) helper can cover the final parameter.

~Andrew

>      COMPAT_CALL(vcpu_op),
>      HVM_CALL(physdev_op),
>      COMPAT_CALL(xen_version),
> --- a/xen/include/asm-x86/config.h
> +++ b/xen/include/asm-x86/config.h
> @@ -319,6 +319,7 @@ extern unsigned long xen_phys_start;
>  #define VM_ASSIST_VALID          NATIVE_VM_ASSIST_VALID
>  #define COMPAT_VM_ASSIST_VALID   (NATIVE_VM_ASSIST_VALID & \
>                                    ((1UL << COMPAT_BITS_PER_LONG) - 1))
> +#define HVM_VM_ASSIST_VALID      (1UL << VMASST_TYPE_runstate_update_flag)
>  
>  #define ELFSIZE 64
>  

Re: [PATCH] x86/HVM: expose VM assist hypercall

[Jan Beulich]

On 02.04.2020 21:49, Andrew Cooper wrote:
> On 02/04/2020 08:46, Jan Beulich wrote:
>> In preparation for the addition of VMASST_TYPE_runstate_update_flag
>> commit 72c538cca957 ("arm: add support for vm_assist hypercall") enabled
>> the hypercall for Arm. I consider it not logical that it then isn't also
>> exposed to x86 HVM guests (with the same single feature permitted to be
>> enabled as Arm has); Linux actually tries to use it afaict.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'd declare this a bug in 2529c850ea4.  It was clearly intended as a
> common feature, and wasn't tested for HVM guests.
> 
> However, ...
> 
>>
>> --- a/xen/arch/x86/hvm/hypercall.c
>> +++ b/xen/arch/x86/hvm/hypercall.c
>> @@ -78,6 +78,11 @@ static long hvm_grant_table_op(
>>  }
>>  #endif
>>  
>> +static long hvm_vm_assist(unsigned int cmd, unsigned int type)
>> +{
>> +    return vm_assist(current->domain, cmd, type, HVM_VM_ASSIST_VALID);
>> +}
>> +
>>  static long hvm_physdev_op(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>  {
>>      const struct vcpu *curr = current;
>> @@ -128,6 +133,7 @@ static const hypercall_table_t hvm_hyper
>>  #ifdef CONFIG_GRANT_TABLE
>>      HVM_CALL(grant_table_op),
>>  #endif
>> +    HVM_CALL(vm_assist),
> 
> ... this means we've now got 3 stub functions making no-op ABI changes
> for the general vm_assist() function.

Not sure what you mean with "no-op" here. It's not like the
mask values would all be the same.

> Renaming it to do_vm_assist(), latch current->domain internally, and an
> arch_vm_assist_valid(d) helper can cover the final parameter.

I can certainly do this, but it very much seems to me to be a
request you'd call "scope creep". I was meaning to address the
issue at hand with a minimally invasive change. The bigger
variant you suggest is unlikely to cause backporting issues,
yes, but still ... Anyway, I'll do as you ask, to cut the
discussion short.

Jan