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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2020 22:16, Andrew Cooper wrote:
> On 14/04/2020 12:34, 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.
>>
>> Rather than introducing yet another thin wrapper around vm_assist(),
>> make that function the main handler, requiring a per-arch
>> arch_vm_assist_valid() definition instead.
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Re-work vm_assist() handling/layering at the same time. Also adjust
>>     arch_set_info_guest().
> 
> Much nicer.  Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks.

> However, ...
> 
>> --- a/xen/common/domain.c
>> +++ b/xen/common/domain.c
>> @@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc
>>      return rc;
>>  }
>>  
>> -#ifdef VM_ASSIST_VALID
>> -long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
>> -               unsigned long valid)
>> +#ifdef arch_vm_assist_valid
>> +long do_vm_assist(unsigned int cmd, unsigned int type)
>>  {
>> +    struct domain *currd = current->domain;
>> +    const unsigned long valid = arch_vm_assist_valid(currd);
>> +
>>      if ( type >= BITS_PER_LONG || !test_bit(type, &valid) )
>>          return -EINVAL;
> 
> As a thought, would it be better to have a guest_bits_per_long()
> helper?  This type >= BITS_PER_LONG isn't terribly correct for 32bit
> guests, and it would avoid needing the truncation in the arch helper,
> which is asymmetric on the ARM side.

I'd rather not - the concept of guest bitness is already fuzzy
enough for HVM (see our 32-bit shared info latching), and
introducing a generic predicate like you suggest would invite
for use of it in places where people may forget how fuzzy the
concept is.

I also don't view the BITS_PER_LONG check here as pertaining
to a guest property - all we want is to bound the test_bit().
There's nothing wrong to, in the future, define bits beyond
possible guest bitness. It's merely a "helps for now" that on
x86 we've decided to put the 1st 64-bit only assist bit in
the high 32 bits (it may well be that this was added back
when we still had 32-bit support for Xen itself).

Jan