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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/04/2020 13:35, Julien Grall wrote:
>>>> --- 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.
>
> How about splitting PV_VM_ASSIST_VALID in two? One would contain
> 64-bit PV specific flags and the other common PV flags?
>
> This should make the code more obvious and easier to read for someone
> less familiar with the area.
>
> It also means we could have a BUILD_BUG_ON() to check at build time
> that no flags are added above 32-bit.

I like this suggestion, but would suggest a 64/32 split, rather than
64/common.  These are a total mixed bag and every new one should be
considered for all ABIs rather than falling automatically into some.

~Andrew