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 <jbeul...@suse.com>
Thankyou. ~Andrew
