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

[Jan Beulich <jbeulich@xxxxxxxx>]

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