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 <[email protected]> Jan
