| Message ID | 5ddd32a453b098f277f2d4aa9e044a40183d4bff.1670300446.git.demi@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make PAT handling less brittle | expand |
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@invisiblethingslab.com> > --- > 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. 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) in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants want removing. 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. > + > /* > * 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(). ~Andrew
On 06.12.2022 05:33, Demi Marie Obenour wrote: > --- 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 would be the at least 4th instance of these constants in our tree (see xen/arch/x86/include/asm/mtrr.h, xen/include/public/domctl.h, and xen/include/public/hvm/dm_op.h). Since they're part of the public interface, I'm inclined to suggest to use those. If that faces opposition, the next best approach would apparently be to move mtrr.h's into x86-defns.h (including the largely identical MTRR values), thus then also allowing hvmloader sources (which currently open-code the MTRR values in cacheattr.c). > /* > * 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"); I'm afraid this won't compile on all gcc versions we support; see xen/include/xen/lib.h's conditional use thereof for BUILD_BUG_ON(). You really want to use BUILD_BUG_ON() here instead of partly open- coding it. (Yes, it requires to be put where a statement can be put, not just a declaration, but the check doesn't strictly need to live in a header file, so that's reasonably easy to accommodate for.) Jan
On 06.12.2022 12:32, Andrew Cooper wrote: > On 06/12/2022 04:33, Demi Marie Obenour wrote: >> --- 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. 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) > > in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants > want removing. > > 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. The unavailability of UCM is common to MTRR and EPT's EMT. WC, at the same time, is a valid type to use in both. Which makes sense - EMT after all merely is replacing what otherwise is expressed by MTRRs. Jan
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@invisiblethingslab.com> > > --- > > 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
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@invisiblethingslab.com> > > --- > > 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. 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) > > in x86-defns.h, and the PAT_TYPE_*, MTRR_TYPE_* and EPT_EMT_* constants > want removing. > > There are two minor restrictions (EPT can't have UCM, MTRR can't have > WC), The code seems to indicate otherwise: it behaves as if both EPT and MTRR cannot have UCM.
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) + /* * 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"); struct domain; struct vcpu;
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@invisiblethingslab.com> --- xen/arch/x86/include/asm/processor.h | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)