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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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.

~Andrew