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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 24/03/2023 9:32 am, Jan Beulich wrote:
> 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>

Thankyou.

~Andrew