diff mbox series

x86/HVM: expose VM assist hypercall

Message ID cb4a6f8f-eda8-f17c-54e5-af1353d6358c@suse.com (mailing list archive)
State New, archived
Headers show
Series x86/HVM: expose VM assist hypercall | expand

Commit Message

Jan Beulich April 2, 2020, 7:46 a.m. UTC 
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>

Comments

Andrew Cooper April 2, 2020, 7:49 p.m. UTC | #1 
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
>
Jan Beulich April 3, 2020, 7:43 a.m. UTC | #2 
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
diff mbox series

Patch

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