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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.04.2020 19:53, Julien Grall wrote:
>> --- 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
> 
> How about naming the function arch_vm_assist_valid_mask?

Certainly a possibility, albeit to me the gain would be marginal
and possibly not outweigh the growth in length. Andrew, any
preference?

>> --- a/xen/include/asm-x86/domain.h
>> +++ b/xen/include/asm-x86/domain.h
>> @@ -700,6 +700,20 @@ static inline void pv_inject_sw_interrup
>>     pv_inject_event(&event);
>> }
>> +#define PV_VM_ASSIST_VALID  ((1UL << VMASST_TYPE_4gb_segments)        | \
>> +                             (1UL << VMASST_TYPE_4gb_segments_notify) | \
>> +                             (1UL << VMASST_TYPE_writable_pagetables) | \
>> +                             (1UL << VMASST_TYPE_pae_extended_cr3)    | \
>> +                             (1UL << VMASST_TYPE_architectural_iopl)  | \
>> +                             (1UL << VMASST_TYPE_runstate_update_flag)| \
>> +                             (1UL << VMASST_TYPE_m2p_strict))
>> +#define HVM_VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag)
>> +
>> +#define arch_vm_assist_valid(d) \
>> +    (is_hvm_domain(d) ? HVM_VM_ASSIST_VALID \
>> +                      : is_pv_32bit_domain(d) ? 
>> (uint32_t)PV_VM_ASSIST_VALID \
> 
> I understand this is matching the current code, however without
> looking at the rest of patch this is not clear why the cast. May
> I suggest to add a comment explaining the rationale?

Hmm, I can state that the rationale is history. Many of the assists in
the low 32 bits are for 32-bit guests only. But we can't start refusing
a 64-bit kernel requesting them. The ones in the high 32 bits are, for
now, applicable to 64-bit guests only, and have always been refused for
32-bit ones.

Imo if anything an explanation on where new bits should be put should
go next to the VMASST_TYPE_* definitions in the public header, yet then
again the public headers aren't (imo) a good place to put
implementation detail comments.

Again, Andrew - since you've ack-ed the patch already, any thoughts
here either way?

Jan