Re: [PATCH] x86/hvm: Improve hvm_set_guest_pat() code generation again

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.03.2023 01:59, Andrew Cooper wrote:
> On 19/12/2022 7:28 am, Jan Beulich wrote:
>> On 16.12.2022 21:53, Andrew Cooper wrote:
>> Again - one way to look at things. Plus, with Demi's series now also in
>> mind, what's done here is moving us in exactly the opposite direction.
>> Is this hot enough a function to warrant that?
> 
> Yes - from the first cset, 9ce0a5e207f3 - it's used on virtual
> vmentry/exit so is (or will be) reasonably hot in due course.
> 
> What is more important in the short term is avoiding the catastrophic
> code generation that Clang still does with default Xen build settings,
> also linked from the commit message.
> 
>>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>>> @@ -302,24 +302,43 @@ void hvm_get_guest_pat(struct vcpu *v, u64 
>>>>> *guest_pat)
>>>>>          *guest_pat = v->arch.hvm.pat_cr;
>>>>>  }
>>>>>  
>>>>> -int hvm_set_guest_pat(struct vcpu *v, uint64_t guest_pat)
>>>>> +/*
>>>>> + * MSR_PAT takes 8 uniform fields, each of which must be a valid 
>>>>> architectural
>>>>> + * memory type (0, 1, 4-7).  This is a fully vectorised form of the
>>>>> + * 8-iteration loop over bytes looking for PAT_TYPE_* constants.
>>>> While grep-ing for PAT_TYPE_ will hit this line, I think we want
>>>> every individual type to also be found here when grep-ing for one.
>>>> The actual values aren't going to change, but perhaps the beast
>>>> way to do so would still be by way of BUILD_BUG_ON()s.
>>> Why?  What does that solve or improve?
>>>
>>> "pat" is the thing people are going to be looking for if they're
>>> actually trying to find this logic.
>>>
>>> (And I bring this patch up specifically after reviewing Demi's series,
>>> where PAT_TYPE_* changes to X86_MT_* but "pat" is still the useful
>>> search term IMO.)
>> I don't think "PAT" is a good thing to grep for when trying to find uses
>> of particular memory types.
> 
> This is not a logical use of a particular memory type.  Being an
> architectural auditing function, the only legitimate use of these
> constants here is all of them at once.  This is the one place you firmly
> don't care about finding when asking the question "How does Xen go about
> handling WP mappings".
> 
> I have swapped PAT_TYPE_* for X86_MT_* now that Demi's series has been
> committed, but that is the extent to which I think there are relevant
> changes to be made.

In the interest of getting the code gen issue addressed, but without
being fully convinced this is a good move:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan