Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
On 14.06.2024 15:56, Andrew Cooper wrote: > On 23/05/2024 4:34 pm, Jan Beulich wrote: >> On 23.05.2024 13:16, Andrew Cooper wrote: >>> Right now, xstate_ctxt_size() performs a cross-check of size with CPUID in >>> for >>> every call. This is expensive, being used for domain create/migrate, as >>> well >>> as to service certain guest CPUID instructions. >>> >>> Instead, arrange to check the sizes once at boot. See the code comments for >>> details. Right now, it just checks hardware against the algorithm >>> expectations. Later patches will add further cross-checking. >>> >>> Introduce the missing X86_XCR0_* and X86_XSS_* constants, and a couple of >>> missing CPUID bits. This is to maximise coverage in the sanity check, even >>> if >>> we don't expect to use/virtualise some of these features any time soon. >>> Leave >>> HDC and HWP alone for now. We don't have CPUID bits from them stored >>> nicely. >> Since you say "the missing", ... >> >>> --- a/xen/arch/x86/include/asm/x86-defns.h >>> +++ b/xen/arch/x86/include/asm/x86-defns.h >>> @@ -77,7 +77,7 @@ >>> #define X86_CR4_PKS0x0100 /* Protection Key Supervisor */ >>> >>> /* >>> - * XSTATE component flags in XCR0 >>> + * XSTATE component flags in XCR0 | MSR_XSS >>> */ >>> #define X86_XCR0_FP_POS 0 >>> #define X86_XCR0_FP (1ULL << X86_XCR0_FP_POS) >>> @@ -95,11 +95,34 @@ >>> #define X86_XCR0_ZMM (1ULL << X86_XCR0_ZMM_POS) >>> #define X86_XCR0_HI_ZMM_POS 7 >>> #define X86_XCR0_HI_ZMM (1ULL << X86_XCR0_HI_ZMM_POS) >>> +#define X86_XSS_PROC_TRACE(_AC(1, ULL) << 8) >>> #define X86_XCR0_PKRU_POS 9 >>> #define X86_XCR0_PKRU (1ULL << X86_XCR0_PKRU_POS) >>> +#define X86_XSS_PASID (_AC(1, ULL) << 10) >>> +#define X86_XSS_CET_U (_AC(1, ULL) << 11) >>> +#define X86_XSS_CET_S (_AC(1, ULL) << 12) >>> +#define X86_XSS_HDC (_AC(1, ULL) << 13) >>> +#define X86_XSS_UINTR (_AC(1, ULL) << 14) >>> +#define X86_XSS_LBR (_AC(1, ULL) << 15) >>> +#define X86_XSS_HWP (_AC(1, ULL) << 16) >>> +#define X86_XCR0_TILE_CFG (_AC(1, ULL) << 17) >>> +#define X86_XCR0_TILE_DATA(_AC(1, ULL) << 18) >> ... I'm wondering if you deliberately left out APX (bit 19). > > It was deliberate. APX isn't in the SDM yet, so in principle is still > subject to change. > > I've tweaked the commit message to avoid using the word 'missing'. Thanks. >> Since you're re-doing some of what I have long had in patches already, >> I'd also like to ask whether the last underscores each in the two AMX >> names really are useful in your opinion. While rebasing isn't going >> to be difficult either way, it would be yet simpler with >> X86_XCR0_TILECFG and X86_XCR0_TILEDATA, as I've had it in my patches >> for over 3 years. > > I'm torn here. I don't want to deliberately make things harder for you, > but I would really prefer that we use the more legible form... Well, okay, so be it then. Jan
Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
On 23/05/2024 4:34 pm, Jan Beulich wrote:
> On 23.05.2024 13:16, Andrew Cooper wrote:
>> Right now, xstate_ctxt_size() performs a cross-check of size with CPUID in
>> for
>> every call. This is expensive, being used for domain create/migrate, as well
>> as to service certain guest CPUID instructions.
>>
>> Instead, arrange to check the sizes once at boot. See the code comments for
>> details. Right now, it just checks hardware against the algorithm
>> expectations. Later patches will add further cross-checking.
>>
>> Introduce the missing X86_XCR0_* and X86_XSS_* constants, and a couple of
>> missing CPUID bits. This is to maximise coverage in the sanity check, even
>> if
>> we don't expect to use/virtualise some of these features any time soon.
>> Leave
>> HDC and HWP alone for now. We don't have CPUID bits from them stored nicely.
> Since you say "the missing", ...
>
>> --- a/xen/arch/x86/include/asm/x86-defns.h
>> +++ b/xen/arch/x86/include/asm/x86-defns.h
>> @@ -77,7 +77,7 @@
>> #define X86_CR4_PKS0x0100 /* Protection Key Supervisor */
>>
>> /*
>> - * XSTATE component flags in XCR0
>> + * XSTATE component flags in XCR0 | MSR_XSS
>> */
>> #define X86_XCR0_FP_POS 0
>> #define X86_XCR0_FP (1ULL << X86_XCR0_FP_POS)
>> @@ -95,11 +95,34 @@
>> #define X86_XCR0_ZMM (1ULL << X86_XCR0_ZMM_POS)
>> #define X86_XCR0_HI_ZMM_POS 7
>> #define X86_XCR0_HI_ZMM (1ULL << X86_XCR0_HI_ZMM_POS)
>> +#define X86_XSS_PROC_TRACE(_AC(1, ULL) << 8)
>> #define X86_XCR0_PKRU_POS 9
>> #define X86_XCR0_PKRU (1ULL << X86_XCR0_PKRU_POS)
>> +#define X86_XSS_PASID (_AC(1, ULL) << 10)
>> +#define X86_XSS_CET_U (_AC(1, ULL) << 11)
>> +#define X86_XSS_CET_S (_AC(1, ULL) << 12)
>> +#define X86_XSS_HDC (_AC(1, ULL) << 13)
>> +#define X86_XSS_UINTR (_AC(1, ULL) << 14)
>> +#define X86_XSS_LBR (_AC(1, ULL) << 15)
>> +#define X86_XSS_HWP (_AC(1, ULL) << 16)
>> +#define X86_XCR0_TILE_CFG (_AC(1, ULL) << 17)
>> +#define X86_XCR0_TILE_DATA(_AC(1, ULL) << 18)
> ... I'm wondering if you deliberately left out APX (bit 19).
It was deliberate. APX isn't in the SDM yet, so in principle is still
subject to change.
I've tweaked the commit message to avoid using the word 'missing'.
> Since you're re-doing some of what I have long had in patches already,
> I'd also like to ask whether the last underscores each in the two AMX
> names really are useful in your opinion. While rebasing isn't going
> to be difficult either way, it would be yet simpler with
> X86_XCR0_TILECFG and X86_XCR0_TILEDATA, as I've had it in my patches
> for over 3 years.
I'm torn here. I don't want to deliberately make things harder for you,
but I would really prefer that we use the more legible form...
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -604,9 +604,156 @@ static bool valid_xcr0(uint64_t xcr0)
>> if ( !(xcr0 & X86_XCR0_BNDREGS) != !(xcr0 & X86_XCR0_BNDCSR) )
>> return false;
>>
>> +/* TILE_CFG and TILE_DATA must be the same. */
>> +if ( !(xcr0 & X86_XCR0_TILE_CFG) != !(xcr0 & X86_XCR0_TILE_DATA) )
>> +return false;
>> +
>> return true;
>> }
>>
>> +struct xcheck_state {
>> +uint64_t states;
>> +uint32_t uncomp_size;
>> +uint32_t comp_size;
>> +};
>> +
>> +static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
>> +{
>> +uint32_t hw_size;
>> +
>> +BUILD_BUG_ON(X86_XCR0_STATES & X86_XSS_STATES);
>> +
>> +BUG_ON(s->states & new); /* States only increase. */
>> +BUG_ON(!valid_xcr0(s->states | new)); /* Xen thinks it's a good value.
>> */
>> +BUG_ON(new & ~(X86_XCR0_STATES | X86_XSS_STATES)); /* Known state. */
>> +BUG_ON((new & X86_XCR0_STATES) &&
>> + (new & X86_XSS_STATES)); /* User or supervisor, not both. */
>> +
>> +s->states |= new;
>> +if ( new & X86_XCR0_STATES )
>> +{
>> +if ( !set_xcr0(s->states & X86_XCR0_STATES) )
>> +BUG();
>> +}
>> +else
>> +set_msr_xss(s->states & X86_XSS_STATES);
>> +
>> +/*
>> + * Check the uncompressed size. Some XSTATEs are out-of-order and fill
>> in
>> + * prior holes in the state area, so we check that the size doesn't
>> + * decrease.
>> + */
>> +hw_size = cpuid_count_ebx(0xd, 0);
>> +
>> +if ( hw_size < s->uncomp_size )
>> +panic("XSTATE 0x%016"PRIx64", new bits {%63pbl}, uncompressed hw
>> size %#x < prev size %#x\n",
>> + s->states, , hw_size, s->uncomp_size);
>> +
>> +s->uncomp_size = hw_size;
>> +
>> +/*
>> + * Check the compressed size, if available. All components strictly
>> + * appear in index order. In principle there are no holes, but some
>> + * components have their base address 64-byte aligned for efficiency
>> + * reasons (e.g. AMX-TILE) and there are other
Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
On 23.05.2024 13:16, Andrew Cooper wrote:
> Right now, xstate_ctxt_size() performs a cross-check of size with CPUID in for
> every call. This is expensive, being used for domain create/migrate, as well
> as to service certain guest CPUID instructions.
>
> Instead, arrange to check the sizes once at boot. See the code comments for
> details. Right now, it just checks hardware against the algorithm
> expectations. Later patches will add further cross-checking.
>
> Introduce the missing X86_XCR0_* and X86_XSS_* constants, and a couple of
> missing CPUID bits. This is to maximise coverage in the sanity check, even if
> we don't expect to use/virtualise some of these features any time soon. Leave
> HDC and HWP alone for now. We don't have CPUID bits from them stored nicely.
Since you say "the missing", ...
> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -77,7 +77,7 @@
> #define X86_CR4_PKS0x0100 /* Protection Key Supervisor */
>
> /*
> - * XSTATE component flags in XCR0
> + * XSTATE component flags in XCR0 | MSR_XSS
> */
> #define X86_XCR0_FP_POS 0
> #define X86_XCR0_FP (1ULL << X86_XCR0_FP_POS)
> @@ -95,11 +95,34 @@
> #define X86_XCR0_ZMM (1ULL << X86_XCR0_ZMM_POS)
> #define X86_XCR0_HI_ZMM_POS 7
> #define X86_XCR0_HI_ZMM (1ULL << X86_XCR0_HI_ZMM_POS)
> +#define X86_XSS_PROC_TRACE(_AC(1, ULL) << 8)
> #define X86_XCR0_PKRU_POS 9
> #define X86_XCR0_PKRU (1ULL << X86_XCR0_PKRU_POS)
> +#define X86_XSS_PASID (_AC(1, ULL) << 10)
> +#define X86_XSS_CET_U (_AC(1, ULL) << 11)
> +#define X86_XSS_CET_S (_AC(1, ULL) << 12)
> +#define X86_XSS_HDC (_AC(1, ULL) << 13)
> +#define X86_XSS_UINTR (_AC(1, ULL) << 14)
> +#define X86_XSS_LBR (_AC(1, ULL) << 15)
> +#define X86_XSS_HWP (_AC(1, ULL) << 16)
> +#define X86_XCR0_TILE_CFG (_AC(1, ULL) << 17)
> +#define X86_XCR0_TILE_DATA(_AC(1, ULL) << 18)
... I'm wondering if you deliberately left out APX (bit 19).
Since you're re-doing some of what I have long had in patches already,
I'd also like to ask whether the last underscores each in the two AMX
names really are useful in your opinion. While rebasing isn't going
to be difficult either way, it would be yet simpler with
X86_XCR0_TILECFG and X86_XCR0_TILEDATA, as I've had it in my patches
for over 3 years.
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -604,9 +604,156 @@ static bool valid_xcr0(uint64_t xcr0)
> if ( !(xcr0 & X86_XCR0_BNDREGS) != !(xcr0 & X86_XCR0_BNDCSR) )
> return false;
>
> +/* TILE_CFG and TILE_DATA must be the same. */
> +if ( !(xcr0 & X86_XCR0_TILE_CFG) != !(xcr0 & X86_XCR0_TILE_DATA) )
> +return false;
> +
> return true;
> }
>
> +struct xcheck_state {
> +uint64_t states;
> +uint32_t uncomp_size;
> +uint32_t comp_size;
> +};
> +
> +static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
> +{
> +uint32_t hw_size;
> +
> +BUILD_BUG_ON(X86_XCR0_STATES & X86_XSS_STATES);
> +
> +BUG_ON(s->states & new); /* States only increase. */
> +BUG_ON(!valid_xcr0(s->states | new)); /* Xen thinks it's a good value. */
> +BUG_ON(new & ~(X86_XCR0_STATES | X86_XSS_STATES)); /* Known state. */
> +BUG_ON((new & X86_XCR0_STATES) &&
> + (new & X86_XSS_STATES)); /* User or supervisor, not both. */
> +
> +s->states |= new;
> +if ( new & X86_XCR0_STATES )
> +{
> +if ( !set_xcr0(s->states & X86_XCR0_STATES) )
> +BUG();
> +}
> +else
> +set_msr_xss(s->states & X86_XSS_STATES);
> +
> +/*
> + * Check the uncompressed size. Some XSTATEs are out-of-order and fill
> in
> + * prior holes in the state area, so we check that the size doesn't
> + * decrease.
> + */
> +hw_size = cpuid_count_ebx(0xd, 0);
> +
> +if ( hw_size < s->uncomp_size )
> +panic("XSTATE 0x%016"PRIx64", new bits {%63pbl}, uncompressed hw
> size %#x < prev size %#x\n",
> + s->states, , hw_size, s->uncomp_size);
> +
> +s->uncomp_size = hw_size;
> +
> +/*
> + * Check the compressed size, if available. All components strictly
> + * appear in index order. In principle there are no holes, but some
> + * components have their base address 64-byte aligned for efficiency
> + * reasons (e.g. AMX-TILE) and there are other components small enough to
> + * fit in the gap (e.g. PKRU) without increasing the overall length.
> + */
> +hw_size = cpuid_count_ebx(0xd, 1);
> +
> +if ( cpu_has_xsavec )
> +{
> +if ( hw_size < s->comp_size )
> +panic("XSTATE 0x%016"PRIx64", new bits {%63pbl}, compressed hw
> size %#x < prev size %#x\n",
> + s->states, , hw_size, s->comp_size);
> +
> +s->comp_size = hw_size;
> +}
> +else
[PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
Right now, xstate_ctxt_size() performs a cross-check of size with CPUID in for
every call. This is expensive, being used for domain create/migrate, as well
as to service certain guest CPUID instructions.
Instead, arrange to check the sizes once at boot. See the code comments for
details. Right now, it just checks hardware against the algorithm
expectations. Later patches will add further cross-checking.
Introduce the missing X86_XCR0_* and X86_XSS_* constants, and a couple of
missing CPUID bits. This is to maximise coverage in the sanity check, even if
we don't expect to use/virtualise some of these features any time soon. Leave
HDC and HWP alone for now. We don't have CPUID bits from them stored nicely.
Only perform the cross-checks in debug builds. It's only developers or new
hardware liable to trip these checks, and Xen at least tracks "maximum value
ever seen in xcr0" for the lifetime of the VM, which we don't want to be
tickling in the general case.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Roger Pau Monné
v3:
* New
On Sapphire Rapids with the whole series inc diagnostics, we get this pattern:
(XEN) *** check_new_xstate(, 0x0003)
(XEN) *** check_new_xstate(, 0x0004)
(XEN) *** check_new_xstate(, 0x00e0)
(XEN) *** check_new_xstate(, 0x0200)
(XEN) *** check_new_xstate(, 0x0006)
(XEN) *** check_new_xstate(, 0x0100)
(XEN) *** check_new_xstate(, 0x0400)
(XEN) *** check_new_xstate(, 0x0800)
(XEN) *** check_new_xstate(, 0x1000)
(XEN) *** check_new_xstate(, 0x4000)
(XEN) *** check_new_xstate(, 0x8000)
and on Genoa, this pattern:
(XEN) *** check_new_xstate(, 0x0003)
(XEN) *** check_new_xstate(, 0x0004)
(XEN) *** check_new_xstate(, 0x00e0)
(XEN) *** check_new_xstate(, 0x0200)
(XEN) *** check_new_xstate(, 0x0800)
(XEN) *** check_new_xstate(, 0x1000)
---
xen/arch/x86/include/asm/x86-defns.h| 25 +++-
xen/arch/x86/xstate.c | 150
xen/include/public/arch-x86/cpufeatureset.h | 3 +
3 files changed, 177 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/x86-defns.h
b/xen/arch/x86/include/asm/x86-defns.h
index 48d7a3b7af45..d7602ab225c4 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -77,7 +77,7 @@
#define X86_CR4_PKS0x0100 /* Protection Key Supervisor */
/*
- * XSTATE component flags in XCR0
+ * XSTATE component flags in XCR0 | MSR_XSS
*/
#define X86_XCR0_FP_POS 0
#define X86_XCR0_FP (1ULL << X86_XCR0_FP_POS)
@@ -95,11 +95,34 @@
#define X86_XCR0_ZMM (1ULL << X86_XCR0_ZMM_POS)
#define X86_XCR0_HI_ZMM_POS 7
#define X86_XCR0_HI_ZMM (1ULL << X86_XCR0_HI_ZMM_POS)
+#define X86_XSS_PROC_TRACE(_AC(1, ULL) << 8)
#define X86_XCR0_PKRU_POS 9
#define X86_XCR0_PKRU (1ULL << X86_XCR0_PKRU_POS)
+#define X86_XSS_PASID (_AC(1, ULL) << 10)
+#define X86_XSS_CET_U (_AC(1, ULL) << 11)
+#define X86_XSS_CET_S (_AC(1, ULL) << 12)
+#define X86_XSS_HDC (_AC(1, ULL) << 13)
+#define X86_XSS_UINTR (_AC(1, ULL) << 14)
+#define X86_XSS_LBR (_AC(1, ULL) << 15)
+#define X86_XSS_HWP (_AC(1, ULL) << 16)
+#define X86_XCR0_TILE_CFG (_AC(1, ULL) << 17)
+#define X86_XCR0_TILE_DATA(_AC(1, ULL) << 18)
#define X86_XCR0_LWP_POS 62
#define X86_XCR0_LWP (1ULL << X86_XCR0_LWP_POS)
+#define X86_XCR0_STATES \
+(X86_XCR0_FP | X86_XCR0_SSE | X86_XCR0_YMM | X86_XCR0_BNDREGS | \
+ X86_XCR0_BNDCSR | X86_XCR0_OPMASK | X86_XCR0_ZMM | \
+ X86_XCR0_HI_ZMM | X86_XCR0_PKRU | X86_XCR0_TILE_CFG | \
+ X86_XCR0_TILE_DATA | \
+ X86_XCR0_LWP)
+
+#define X86_XSS_STATES \
+(X86_XSS_PROC_TRACE | X86_XSS_PASID | X86_XSS_CET_U | \
+ X86_XSS_CET_S | X86_XSS_HDC | X86_XSS_UINTR | X86_XSS_LBR |\
+ X86_XSS_HWP | \
+ 0)
+
/*
* Debug status flags in DR6.
*
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 75788147966a..33a5a89719ef 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -604,9 +604,156 @@ static bool valid_xcr0(uint64_t xcr0)
if ( !(xcr0 & X86_XCR0_BNDREGS) != !(xcr0 & X86_XCR0_BNDCSR) )
return false;
+/* TILE_CFG and TILE_DATA must be the same. */
+if ( !(xcr0 & X86_XCR0_TILE_CFG) != !(xcr0 & X86_XCR0_TILE_DATA) )
+return false;
+
return true;
}
+struct xcheck_state {
+uint64_t states;
+uint32_t uncomp_size;
+uint32_t comp_size;
+};
+
+static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
+{
+uint32_t
