Re: [PATCH 6/8] x86: Derive XEN_MSR_PAT from its individual entries

[Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>]

On Tue, Dec 06, 2022 at 11:32:16AM +0000, Andrew Cooper wrote:
> On 06/12/2022 04:33, Demi Marie Obenour wrote:
> > This avoids it being a magic constant that is difficult for humans to
> > decode.  Use a _Static_assert to check that the old and new values are
> > identical.
> >
> > Signed-off-by: Demi Marie Obenour <demi@xxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >  xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/x86/include/asm/processor.h 
> > b/xen/arch/x86/include/asm/processor.h
> > index 
> > 8e2816fae9b97bd4e153a30cc3802971fe0355af..64b75e444947c64e2e5eba457deec92a873d7a63
> >  100644
> > --- a/xen/arch/x86/include/asm/processor.h
> > +++ b/xen/arch/x86/include/asm/processor.h
> > @@ -92,13 +92,33 @@
> >                            X86_EFLAGS_NT|X86_EFLAGS_DF|X86_EFLAGS_IF|    \
> >                            X86_EFLAGS_TF)
> >  
> > +/* Individual entries in IA32_CR_PAT */
> > +#define MSR_PAT_UC  _AC(0x00, ULL)
> > +#define MSR_PAT_WC  _AC(0x01, ULL)
> > +#define MSR_PAT_RESERVED_1  _AC(0x02, ULL)
> > +#define MSR_PAT_RESERVED_2  _AC(0x03, ULL)
> > +#define MSR_PAT_WT  _AC(0x04, ULL)
> > +#define MSR_PAT_WP  _AC(0x05, ULL)
> > +#define MSR_PAT_WB  _AC(0x06, ULL)
> > +#define MSR_PAT_UCM _AC(0x07, ULL)
> 
> This isn't really correct.

Do you mean that this in and of itself is buggy, or that the code ought
to be structured differently?

> Constants for MSRs typically live in
> msr-index.h, but these are architectural x86 memory types.
> 
> These ought be
> 
> #define X86_MT_$X ...  (skipping the two reserved values)

I will use the reserved values in BUILD_BUG_ON()s, so I would prefer to
keep (possibly defined somewhere else) if that is okay.

> in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants
> want removing.

This seems like a larger refactor that might belong in a separate patch.

> There are two minor restrictions (EPT can't have UCM, MTRR can't have
> WC), but they are all operating in terms of architectural memory type
> values, and the code ought to reflect this.

That makes sense.

> > +
> >  /*
> >   * Host IA32_CR_PAT value to cover all memory types.  This is not the 
> > default
> >   * MSR_PAT value, and is an ABI with PV guests.
> >   */
> > -#define XEN_MSR_PAT _AC(0x050100070406, ULL)
> > +#define XEN_MSR_PAT (MSR_PAT_WB  << 0x00 | \
> > +                     MSR_PAT_WT  << 0x08 | \
> > +                     MSR_PAT_UCM << 0x10 | \
> > +                     MSR_PAT_UC  << 0x18 | \
> > +                     MSR_PAT_WC  << 0x20 | \
> > +                     MSR_PAT_WP  << 0x28 | \
> > +                     MSR_PAT_UC  << 0x30 | \
> > +                     MSR_PAT_UC  << 0x38 | \
> > +                     0)
> >  
> >  #ifndef __ASSEMBLY__
> > +_Static_assert(XEN_MSR_PAT == 0x050100070406ULL,
> > +               "wrong XEN_MSR_PAT breaks PV guests");
> 
> This wants to be in the build_assertions() that you introduce in the
> next patch, and a BUILD_BUG_ON().  We still support compilers which
> don't know _Static_assert().

That’s fair
-- 
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature