* [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations
@ 2024-05-23 11:16 Andrew Cooper
2024-05-23 11:16 ` [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache Andrew Cooper
` (6 more replies)
0 siblings, 7 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
Oleksii Kurochko
This has grown somewhat from v2, but is better for it IMO.
The headline change is patch 2 performing all the cross-checking at boot time.
This turned into needing prepare the Raw CPU policy earlier on boot (to avoid
further-adding to scheme we're already looking to retire).
The end result has been tested across the entire XenServer hardware lab. This
found several false assupmtion about how the dynamic sizes behave.
Patches 1 and 6 the main bugfixes from this series. There's still lots more
work to do in order to get AMX and/or CET working, but this is at least a 4-yo
series finally off my plate.
Andrew Cooper (7):
x86/xstate: Fix initialisation of XSS cache
x86/xstate: Cross-check dynamic XSTATE sizes at boot
x86/boot: Collect the Raw CPU Policy earlier on boot
x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
x86/cpu-policy: Simplify recalculate_xstate()
x86/cpuid: Fix handling of XSAVE dynamic leaves
x86/defns: Clean up X86_{XCR0,XSS}_* constants
xen/arch/x86/cpu-policy.c | 56 ++--
xen/arch/x86/cpuid.c | 24 +-
xen/arch/x86/domctl.c | 2 +-
xen/arch/x86/hvm/hvm.c | 2 +-
xen/arch/x86/i387.c | 2 +-
xen/arch/x86/include/asm/x86-defns.h | 55 ++--
xen/arch/x86/include/asm/xstate.h | 8 +-
xen/arch/x86/setup.c | 4 +-
xen/arch/x86/xstate.c | 286 +++++++++++++++++---
xen/include/public/arch-x86/cpufeatureset.h | 3 +
xen/include/xen/lib/x86/cpu-policy.h | 2 +-
11 files changed, 322 insertions(+), 122 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 15:09 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot Andrew Cooper
` (5 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
The clobbering of this_cpu(xcr0) and this_cpu(xss) to architecturally invalid
values is to force the subsequent set_xcr0() and set_msr_xss() to reload the
hardware register.
While XCR0 is reloaded in xstate_init(), MSR_XSS isn't. This causes
get_msr_xss() to return the invalid value, and logic of the form:
old = get_msr_xss();
set_msr_xss(new);
...
set_msr_xss(old);
to try and restore the architecturally invalid value.
The architecturally invalid value must be purged from the cache, meaning the
hardware register must be written at least once. This in turn highlights that
the invalid value must only be used in the case that the hardware register is
available.
Fixes: f7f4a523927f ("x86/xstate: reset cached register values on resume")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
v3:
* Split out of later patch
---
xen/arch/x86/xstate.c | 18 +++++++++++-------
1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 99cedb4f5e24..75788147966a 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -641,13 +641,6 @@ void xstate_init(struct cpuinfo_x86 *c)
return;
}
- /*
- * Zap the cached values to make set_xcr0() and set_msr_xss() really
- * write it.
- */
- this_cpu(xcr0) = 0;
- this_cpu(xss) = ~0;
-
cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
feature_mask = (((u64)edx << 32) | eax) & XCNTXT_MASK;
BUG_ON(!valid_xcr0(feature_mask));
@@ -657,8 +650,19 @@ void xstate_init(struct cpuinfo_x86 *c)
* Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
*/
set_in_cr4(X86_CR4_OSXSAVE);
+
+ /*
+ * Zap the cached values to make set_xcr0() and set_msr_xss() really write
+ * the hardware register.
+ */
+ this_cpu(xcr0) = 0;
if ( !set_xcr0(feature_mask) )
BUG();
+ if ( cpu_has_xsaves )
+ {
+ this_cpu(xss) = ~0;
+ set_msr_xss(0);
+ }
if ( bsp )
{
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
2024-05-23 11:16 ` [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 15:34 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot Andrew Cooper
` (4 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
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 <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
v3:
* New
On Sapphire Rapids with the whole series inc diagnostics, we get this pattern:
(XEN) *** check_new_xstate(, 0x00000003)
(XEN) *** check_new_xstate(, 0x00000004)
(XEN) *** check_new_xstate(, 0x000000e0)
(XEN) *** check_new_xstate(, 0x00000200)
(XEN) *** check_new_xstate(, 0x00060000)
(XEN) *** check_new_xstate(, 0x00000100)
(XEN) *** check_new_xstate(, 0x00000400)
(XEN) *** check_new_xstate(, 0x00000800)
(XEN) *** check_new_xstate(, 0x00001000)
(XEN) *** check_new_xstate(, 0x00004000)
(XEN) *** check_new_xstate(, 0x00008000)
and on Genoa, this pattern:
(XEN) *** check_new_xstate(, 0x00000003)
(XEN) *** check_new_xstate(, 0x00000004)
(XEN) *** check_new_xstate(, 0x000000e0)
(XEN) *** check_new_xstate(, 0x00000200)
(XEN) *** check_new_xstate(, 0x00000800)
(XEN) *** check_new_xstate(, 0x00001000)
---
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_PKS 0x01000000 /* 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 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, &new, 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, &new, hw_size, s->comp_size);
+
+ s->comp_size = hw_size;
+ }
+ else
+ BUG_ON(hw_size); /* Compressed size reported, but no XSAVEC ? */
+}
+
+/*
+ * The {un,}compressed XSTATE sizes are reported by dynamic CPUID value, based
+ * on the current %XCR0 and MSR_XSS values. The exact layout is also feature
+ * and vendor specific. Cross-check Xen's understanding against real hardware
+ * on boot.
+ *
+ * Testing every combination is prohibitive, so we use a partial approach.
+ * Starting with nothing active, we add new XSTATEs and check that the CPUID
+ * dynamic values never decreases.
+ */
+static void __init noinline xstate_check_sizes(void)
+{
+ uint64_t old_xcr0 = get_xcr0();
+ uint64_t old_xss = get_msr_xss();
+ struct xcheck_state s = {};
+
+ /*
+ * User XSTATEs, increasing by index.
+ *
+ * Chronologically, Intel and AMD had identical layouts for AVX (YMM).
+ * AMD introduced LWP in Fam15h, following immediately on from YMM. Intel
+ * left an LWP-shaped hole when adding MPX (BND{CSR,REGS}) in Skylake.
+ * AMD removed LWP in Fam17h, putting PKRU in the same space, breaking
+ * layout compatibility with Intel and having a knock-on effect on all
+ * subsequent states.
+ */
+ check_new_xstate(&s, X86_XCR0_SSE | X86_XCR0_FP);
+
+ if ( cpu_has_avx )
+ check_new_xstate(&s, X86_XCR0_YMM);
+
+ if ( cpu_has_mpx )
+ check_new_xstate(&s, X86_XCR0_BNDCSR | X86_XCR0_BNDREGS);
+
+ if ( cpu_has_avx512f )
+ check_new_xstate(&s, X86_XCR0_HI_ZMM | X86_XCR0_ZMM | X86_XCR0_OPMASK);
+
+ if ( cpu_has_pku )
+ check_new_xstate(&s, X86_XCR0_PKRU);
+
+ if ( boot_cpu_has(X86_FEATURE_AMX_TILE) )
+ check_new_xstate(&s, X86_XCR0_TILE_DATA | X86_XCR0_TILE_CFG);
+
+ if ( boot_cpu_has(X86_FEATURE_LWP) )
+ check_new_xstate(&s, X86_XCR0_LWP);
+
+ /*
+ * Supervisor XSTATEs, increasing by index.
+ *
+ * Intel Broadwell in particular had Processor Trace but no XSAVES. There
+ * doesn't appear to have been a new enumeration when X86_XSS_PROC_TRACE
+ * was introduced in Skylake.
+ */
+ if ( cpu_has_xsaves )
+ {
+ if ( cpu_has_proc_trace )
+ check_new_xstate(&s, X86_XSS_PROC_TRACE);
+
+ if ( boot_cpu_has(X86_FEATURE_ENQCMD) )
+ check_new_xstate(&s, X86_XSS_PASID);
+
+ if ( boot_cpu_has(X86_FEATURE_CET_SS) ||
+ boot_cpu_has(X86_FEATURE_CET_IBT) )
+ {
+ check_new_xstate(&s, X86_XSS_CET_U);
+ check_new_xstate(&s, X86_XSS_CET_S);
+ }
+
+ if ( boot_cpu_has(X86_FEATURE_UINTR) )
+ check_new_xstate(&s, X86_XSS_UINTR);
+
+ if ( boot_cpu_has(X86_FEATURE_ARCH_LBR) )
+ check_new_xstate(&s, X86_XSS_LBR);
+ }
+
+ /* Restore old state now the test is done. */
+ if ( !set_xcr0(old_xcr0) )
+ BUG();
+ if ( cpu_has_xsaves )
+ set_msr_xss(old_xss);
+}
+
/* Collect the information of processor's extended state */
void xstate_init(struct cpuinfo_x86 *c)
{
@@ -683,6 +830,9 @@ void xstate_init(struct cpuinfo_x86 *c)
if ( setup_xstate_features(bsp) && bsp )
BUG();
+
+ if ( IS_ENABLED(CONFIG_DEBUG) && bsp )
+ xstate_check_sizes();
}
int validate_xstate(const struct domain *d, uint64_t xcr0, uint64_t xcr0_accum,
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 6627453e3985..d9eba5e9a714 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -266,6 +266,7 @@ XEN_CPUFEATURE(IBPB_RET, 8*32+30) /*A IBPB clears RSB/RAS too. */
XEN_CPUFEATURE(AVX512_4VNNIW, 9*32+ 2) /*A AVX512 Neural Network Instructions */
XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A AVX512 Multiply Accumulation Single Precision */
XEN_CPUFEATURE(FSRM, 9*32+ 4) /*A Fast Short REP MOVS */
+XEN_CPUFEATURE(UINTR, 9*32+ 5) /* User-mode Interrupts */
XEN_CPUFEATURE(AVX512_VP2INTERSECT, 9*32+8) /*a VP2INTERSECT{D,Q} insns */
XEN_CPUFEATURE(SRBDS_CTRL, 9*32+ 9) /* MSR_MCU_OPT_CTRL and RNGDS_MITG_DIS. */
XEN_CPUFEATURE(MD_CLEAR, 9*32+10) /*!A| VERW clears microarchitectural buffers */
@@ -274,8 +275,10 @@ XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */
XEN_CPUFEATURE(SERIALIZE, 9*32+14) /*A SERIALIZE insn */
XEN_CPUFEATURE(HYBRID, 9*32+15) /* Heterogeneous platform */
XEN_CPUFEATURE(TSXLDTRK, 9*32+16) /*a TSX load tracking suspend/resume insns */
+XEN_CPUFEATURE(ARCH_LBR, 9*32+19) /* Architectural Last Branch Record */
XEN_CPUFEATURE(CET_IBT, 9*32+20) /* CET - Indirect Branch Tracking */
XEN_CPUFEATURE(AVX512_FP16, 9*32+23) /*A AVX512 FP16 instructions */
+XEN_CPUFEATURE(AMX_TILE, 9*32+24) /* AMX Tile architecture */
XEN_CPUFEATURE(IBRSB, 9*32+26) /*A IBRS and IBPB support (used by Intel) */
XEN_CPUFEATURE(STIBP, 9*32+27) /*A STIBP */
XEN_CPUFEATURE(L1D_FLUSH, 9*32+28) /*S MSR_FLUSH_CMD and L1D flush. */
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
2024-05-23 11:16 ` [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache Andrew Cooper
2024-05-23 11:16 ` [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 15:44 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
` (3 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
This is a tangle, but it's a small step in the right direction.
xstate_init() is shortly going to want data from the Raw policy.
calculate_raw_cpu_policy() is sufficiently separate from the other policies to
be safe to do.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
This is necessary for the forthcoming xstate_{un,}compressed_size() to perform
boot-time sanity checks on state components which aren't fully enabled yet. I
decided that doing this was better than extending the xstate_{offsets,sizes}[]
logic that we're intending to retire in due course.
v3:
* New.
---
xen/arch/x86/cpu-policy.c | 1 -
xen/arch/x86/setup.c | 4 +++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index b96f4ee55cc4..5b66f002df05 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -845,7 +845,6 @@ static void __init calculate_hvm_def_policy(void)
void __init init_guest_cpu_policies(void)
{
- calculate_raw_cpu_policy();
calculate_host_policy();
if ( IS_ENABLED(CONFIG_PV) )
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index b50c9c84af6d..8850e5637a98 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1888,7 +1888,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */
- identify_cpu(&boot_cpu_data);
+ calculate_raw_cpu_policy(); /* Needs microcode. No other dependenices. */
+
+ identify_cpu(&boot_cpu_data); /* Needs microcode and raw policy. */
set_in_cr4(X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT);
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
` (2 preceding siblings ...)
2024-05-23 11:16 ` [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 16:09 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 5/7] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
` (2 subsequent siblings)
6 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
We're soon going to need a compressed helper of the same form.
The size of the uncompressed image depends on the single element with the
largest offset + size. Sadly this isn't always the element with the largest
index.
Name the per-xstate-component cpu_policy struture, for legibility of the logic
in xstate_uncompressed_size(). Cross-check with hardware during boot, and
remove hw_uncompressed_size(). This means that the migration paths don't need
to mess with XCR0 just to sanity check the buffer size.
The users of hw_uncompressed_size() in xstate_init() can (and indeed need) to
be replaced with CPUID instructions. They run with feature_mask in XCR0, and
prior to setup_xstate_features() on the BSP.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
v2:
* Scan all features. LWP/APX_F are out-of-order.
v3:
* Rebase over boot time check.
* Use the raw CPU policy.
---
xen/arch/x86/domctl.c | 2 +-
xen/arch/x86/hvm/hvm.c | 2 +-
xen/arch/x86/include/asm/xstate.h | 2 +-
xen/arch/x86/xstate.c | 78 +++++++++++++++++-----------
xen/include/xen/lib/x86/cpu-policy.h | 2 +-
5 files changed, 51 insertions(+), 35 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 9a72d57333e9..c2f2016ed45a 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -833,7 +833,7 @@ long arch_do_domctl(
uint32_t offset = 0;
#define PV_XSAVE_HDR_SIZE (2 * sizeof(uint64_t))
-#define PV_XSAVE_SIZE(xcr0) (PV_XSAVE_HDR_SIZE + xstate_ctxt_size(xcr0))
+#define PV_XSAVE_SIZE(xcr0) (PV_XSAVE_HDR_SIZE + xstate_uncompressed_size(xcr0))
ret = -ESRCH;
if ( (evc->vcpu >= d->max_vcpus) ||
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2c66fe0f7a16..b84f4d2387d1 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1190,7 +1190,7 @@ HVM_REGISTER_SAVE_RESTORE(CPU, hvm_save_cpu_ctxt, NULL, hvm_load_cpu_ctxt, 1,
#define HVM_CPU_XSAVE_SIZE(xcr0) (offsetof(struct hvm_hw_cpu_xsave, \
save_area) + \
- xstate_ctxt_size(xcr0))
+ xstate_uncompressed_size(xcr0))
static int cf_check hvm_save_cpu_xsave_states(
struct vcpu *v, hvm_domain_context_t *h)
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index c08c267884f0..f5115199d4f9 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -107,7 +107,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size);
void xstate_free_save_area(struct vcpu *v);
int xstate_alloc_save_area(struct vcpu *v);
void xstate_init(struct cpuinfo_x86 *c);
-unsigned int xstate_ctxt_size(u64 xcr0);
+unsigned int xstate_uncompressed_size(uint64_t xcr0);
static inline uint64_t xgetbv(unsigned int index)
{
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 33a5a89719ef..1b3153600d9c 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -8,6 +8,8 @@
#include <xen/param.h>
#include <xen/percpu.h>
#include <xen/sched.h>
+
+#include <asm/cpu-policy.h>
#include <asm/current.h>
#include <asm/processor.h>
#include <asm/i387.h>
@@ -183,7 +185,7 @@ void expand_xsave_states(const struct vcpu *v, void *dest, unsigned int size)
/* Check there is state to serialise (i.e. at least an XSAVE_HDR) */
BUG_ON(!v->arch.xcr0_accum);
/* Check there is the correct room to decompress into. */
- BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+ BUG_ON(size != xstate_uncompressed_size(v->arch.xcr0_accum));
if ( !(xstate->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
{
@@ -245,7 +247,7 @@ void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
u64 xstate_bv, valid;
BUG_ON(!v->arch.xcr0_accum);
- BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
+ BUG_ON(size != xstate_uncompressed_size(v->arch.xcr0_accum));
ASSERT(!xsave_area_compressed(src));
xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
@@ -553,32 +555,6 @@ void xstate_free_save_area(struct vcpu *v)
v->arch.xsave_area = NULL;
}
-static unsigned int hw_uncompressed_size(uint64_t xcr0)
-{
- u64 act_xcr0 = get_xcr0();
- unsigned int size;
- bool ok = set_xcr0(xcr0);
-
- ASSERT(ok);
- size = cpuid_count_ebx(XSTATE_CPUID, 0);
- ok = set_xcr0(act_xcr0);
- ASSERT(ok);
-
- return size;
-}
-
-/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
-unsigned int xstate_ctxt_size(u64 xcr0)
-{
- if ( xcr0 == xfeature_mask )
- return xsave_cntxt_size;
-
- if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
- return 0;
-
- return hw_uncompressed_size(xcr0);
-}
-
static bool valid_xcr0(uint64_t xcr0)
{
/* FP must be unconditionally set. */
@@ -611,6 +587,40 @@ static bool valid_xcr0(uint64_t xcr0)
return true;
}
+unsigned int xstate_uncompressed_size(uint64_t xcr0)
+{
+ unsigned int size = XSTATE_AREA_MIN_SIZE, i;
+
+ ASSERT((xcr0 & ~X86_XCR0_STATES) == 0);
+
+ if ( xcr0 == xfeature_mask )
+ return xsave_cntxt_size;
+
+ if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
+ return 0;
+
+ if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )
+ return size;
+
+ /*
+ * For the non-legacy states, search all activate states and find the
+ * maximum offset+size. Some states (e.g. LWP, APX_F) are out-of-order
+ * with respect their index.
+ */
+ xcr0 &= ~(X86_XCR0_SSE | X86_XCR0_FP);
+ for_each_set_bit ( i, &xcr0, 63 )
+ {
+ const struct xstate_component *c = &raw_cpu_policy.xstate.comp[i];
+ unsigned int s = c->offset + c->size;
+
+ ASSERT(c->offset && c->size);
+
+ size = max(size, s);
+ }
+
+ return size;
+}
+
struct xcheck_state {
uint64_t states;
uint32_t uncomp_size;
@@ -619,7 +629,7 @@ struct xcheck_state {
static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
{
- uint32_t hw_size;
+ uint32_t hw_size, xen_size;
BUILD_BUG_ON(X86_XCR0_STATES & X86_XSS_STATES);
@@ -651,6 +661,12 @@ static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
s->uncomp_size = hw_size;
+ xen_size = xstate_uncompressed_size(s->states & X86_XCR0_STATES);
+
+ if ( xen_size != hw_size )
+ panic("XSTATE 0x%016"PRIx64", uncompressed hw size %#x != xen size %#x\n",
+ s->states, hw_size, xen_size);
+
/*
* Check the compressed size, if available. All components strictly
* appear in index order. In principle there are no holes, but some
@@ -818,14 +834,14 @@ void xstate_init(struct cpuinfo_x86 *c)
* xsave_cntxt_size is the max size required by enabled features.
* We know FP/SSE and YMM about eax, and nothing about edx at present.
*/
- xsave_cntxt_size = hw_uncompressed_size(feature_mask);
+ xsave_cntxt_size = cpuid_count_ebx(0xd, 0);
printk("xstate: size: %#x and states: %#"PRIx64"\n",
xsave_cntxt_size, xfeature_mask);
}
else
{
BUG_ON(xfeature_mask != feature_mask);
- BUG_ON(xsave_cntxt_size != hw_uncompressed_size(feature_mask));
+ BUG_ON(xsave_cntxt_size != cpuid_count_ebx(0xd, 0));
}
if ( setup_xstate_features(bsp) && bsp )
diff --git a/xen/include/xen/lib/x86/cpu-policy.h b/xen/include/xen/lib/x86/cpu-policy.h
index d5e447e9dc06..d26012c6da78 100644
--- a/xen/include/xen/lib/x86/cpu-policy.h
+++ b/xen/include/xen/lib/x86/cpu-policy.h
@@ -248,7 +248,7 @@ struct cpu_policy
};
/* Per-component common state. Valid for i >= 2. */
- struct {
+ struct xstate_component {
uint32_t size, offset;
bool xss:1, align:1;
uint32_t _res_d;
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/7] x86/cpu-policy: Simplify recalculate_xstate()
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
` (3 preceding siblings ...)
2024-05-23 11:16 ` [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 11:16 ` [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves Andrew Cooper
2024-05-23 11:16 ` [PATCH 7/7] x86/defns: Clean up X86_{XCR0,XSS}_* constants Andrew Cooper
6 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Jan Beulich, Roger Pau Monné
Make use of xstate_uncompressed_size() helper rather than maintaining the
running calculation while accumulating feature components.
The rest of the CPUID data can come direct from the raw cpu policy. All
per-component data form an ABI through the behaviour of the X{SAVE,RSTOR}*
instructions.
Use for_each_set_bit() rather than opencoding a slightly awkward version of
it. Mask the attributes in ecx down based on the visible features. This
isn't actually necessary for any components or attributes defined at the time
of writing (up to AMX), but is added out of an abundance of caution.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
v2:
* Tie ALIGN64 to xsavec rather than xsaves.
v3:
* Tweak commit message.
---
xen/arch/x86/cpu-policy.c | 55 +++++++++++--------------------
xen/arch/x86/include/asm/xstate.h | 1 +
2 files changed, 21 insertions(+), 35 deletions(-)
diff --git a/xen/arch/x86/cpu-policy.c b/xen/arch/x86/cpu-policy.c
index 5b66f002df05..304dc20cfab8 100644
--- a/xen/arch/x86/cpu-policy.c
+++ b/xen/arch/x86/cpu-policy.c
@@ -193,8 +193,7 @@ static void sanitise_featureset(uint32_t *fs)
static void recalculate_xstate(struct cpu_policy *p)
{
uint64_t xstates = XSTATE_FP_SSE;
- uint32_t xstate_size = XSTATE_AREA_MIN_SIZE;
- unsigned int i, Da1 = p->xstate.Da1;
+ unsigned int i, ecx_mask = 0, Da1 = p->xstate.Da1;
/*
* The Da1 leaf is the only piece of information preserved in the common
@@ -206,61 +205,47 @@ static void recalculate_xstate(struct cpu_policy *p)
return;
if ( p->basic.avx )
- {
xstates |= X86_XCR0_YMM;
- xstate_size = max(xstate_size,
- xstate_offsets[X86_XCR0_YMM_POS] +
- xstate_sizes[X86_XCR0_YMM_POS]);
- }
if ( p->feat.mpx )
- {
xstates |= X86_XCR0_BNDREGS | X86_XCR0_BNDCSR;
- xstate_size = max(xstate_size,
- xstate_offsets[X86_XCR0_BNDCSR_POS] +
- xstate_sizes[X86_XCR0_BNDCSR_POS]);
- }
if ( p->feat.avx512f )
- {
xstates |= X86_XCR0_OPMASK | X86_XCR0_ZMM | X86_XCR0_HI_ZMM;
- xstate_size = max(xstate_size,
- xstate_offsets[X86_XCR0_HI_ZMM_POS] +
- xstate_sizes[X86_XCR0_HI_ZMM_POS]);
- }
if ( p->feat.pku )
- {
xstates |= X86_XCR0_PKRU;
- xstate_size = max(xstate_size,
- xstate_offsets[X86_XCR0_PKRU_POS] +
- xstate_sizes[X86_XCR0_PKRU_POS]);
- }
- p->xstate.max_size = xstate_size;
+ /* Subleaf 0 */
+ p->xstate.max_size =
+ xstate_uncompressed_size(xstates & ~XSTATE_XSAVES_ONLY);
p->xstate.xcr0_low = xstates & ~XSTATE_XSAVES_ONLY;
p->xstate.xcr0_high = (xstates & ~XSTATE_XSAVES_ONLY) >> 32;
+ /* Subleaf 1 */
p->xstate.Da1 = Da1;
+ if ( p->xstate.xsavec )
+ ecx_mask |= XSTATE_ALIGN64;
+
if ( p->xstate.xsaves )
{
+ ecx_mask |= XSTATE_XSS;
p->xstate.xss_low = xstates & XSTATE_XSAVES_ONLY;
p->xstate.xss_high = (xstates & XSTATE_XSAVES_ONLY) >> 32;
}
- else
- xstates &= ~XSTATE_XSAVES_ONLY;
- for ( i = 2; i < min(63UL, ARRAY_SIZE(p->xstate.comp)); ++i )
+ /* Subleafs 2+ */
+ xstates &= ~XSTATE_FP_SSE;
+ BUILD_BUG_ON(ARRAY_SIZE(p->xstate.comp) < 63);
+ for_each_set_bit ( i, &xstates, 63 )
{
- uint64_t curr_xstate = 1UL << i;
-
- if ( !(xstates & curr_xstate) )
- continue;
-
- p->xstate.comp[i].size = xstate_sizes[i];
- p->xstate.comp[i].offset = xstate_offsets[i];
- p->xstate.comp[i].xss = curr_xstate & XSTATE_XSAVES_ONLY;
- p->xstate.comp[i].align = curr_xstate & xstate_align;
+ /*
+ * Pass through size (eax) and offset (ebx) directly. Visbility of
+ * attributes in ecx limited by visible features in Da1.
+ */
+ p->xstate.raw[i].a = raw_cpu_policy.xstate.raw[i].a;
+ p->xstate.raw[i].b = raw_cpu_policy.xstate.raw[i].b;
+ p->xstate.raw[i].c = raw_cpu_policy.xstate.raw[i].c & ecx_mask;
}
}
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index f5115199d4f9..bfb66dd766b6 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -40,6 +40,7 @@ extern uint32_t mxcsr_mask;
#define XSTATE_XSAVES_ONLY 0
#define XSTATE_COMPACTION_ENABLED (1ULL << 63)
+#define XSTATE_XSS (1U << 0)
#define XSTATE_ALIGN64 (1U << 1)
extern u64 xfeature_mask;
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
` (4 preceding siblings ...)
2024-05-23 11:16 ` [PATCH 5/7] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 16:16 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 7/7] x86/defns: Clean up X86_{XCR0,XSS}_* constants Andrew Cooper
6 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu
First, if XSAVE is available in hardware but not visible to the guest, the
dynamic leaves shouldn't be filled in.
Second, the comment concerning XSS state is wrong. VT-x doesn't manage
host/guest state automatically, but there is provision for "host only" bits to
be set, so the implications are still accurate.
Introduce xstate_compressed_size() to mirror the uncompressed one. Cross
check it at boot.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
v3:
* Adjust commit message about !XSAVE guests
* Rebase over boot time cross check
* Use raw policy
---
xen/arch/x86/cpuid.c | 24 ++++++++--------------
xen/arch/x86/include/asm/xstate.h | 1 +
xen/arch/x86/xstate.c | 34 +++++++++++++++++++++++++++++++
3 files changed, 43 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index 7a38e032146a..a822e80c7ea7 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -330,23 +330,15 @@ void guest_cpuid(const struct vcpu *v, uint32_t leaf,
case XSTATE_CPUID:
switch ( subleaf )
{
- case 1:
- if ( !p->xstate.xsavec && !p->xstate.xsaves )
- break;
-
- /*
- * TODO: Figure out what to do for XSS state. VT-x manages host
- * vs guest MSR_XSS automatically, so as soon as we start
- * supporting any XSS states, the wrong XSS will be in context.
- */
- BUILD_BUG_ON(XSTATE_XSAVES_ONLY != 0);
- fallthrough;
case 0:
- /*
- * Read CPUID[0xD,0/1].EBX from hardware. They vary with enabled
- * XSTATE, and appropriate XCR0|XSS are in context.
- */
- res->b = cpuid_count_ebx(leaf, subleaf);
+ if ( p->basic.xsave )
+ res->b = xstate_uncompressed_size(v->arch.xcr0);
+ break;
+
+ case 1:
+ if ( p->xstate.xsavec )
+ res->b = xstate_compressed_size(v->arch.xcr0 |
+ v->arch.msrs->xss.raw);
break;
}
break;
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index bfb66dd766b6..da1d89d2f416 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -109,6 +109,7 @@ void xstate_free_save_area(struct vcpu *v);
int xstate_alloc_save_area(struct vcpu *v);
void xstate_init(struct cpuinfo_x86 *c);
unsigned int xstate_uncompressed_size(uint64_t xcr0);
+unsigned int xstate_compressed_size(uint64_t xstates);
static inline uint64_t xgetbv(unsigned int index)
{
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 1b3153600d9c..7b7f2dcaf651 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -621,6 +621,34 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
return size;
}
+unsigned int xstate_compressed_size(uint64_t xstates)
+{
+ unsigned int i, size = XSTATE_AREA_MIN_SIZE;
+
+ if ( xstates == 0 ) /* TODO: clean up paths passing 0 in here. */
+ return 0;
+
+ if ( xstates <= (X86_XCR0_SSE | X86_XCR0_FP) )
+ return size;
+
+ /*
+ * For the compressed size, every component matters. Some componenets are
+ * rounded up to 64 first.
+ */
+ xstates &= ~(X86_XCR0_SSE | X86_XCR0_FP);
+ for_each_set_bit ( i, &xstates, 63 )
+ {
+ const struct xstate_component *c = &raw_cpu_policy.xstate.comp[i];
+
+ if ( c->align )
+ size = ROUNDUP(size, 64);
+
+ size += c->size;
+ }
+
+ return size;
+}
+
struct xcheck_state {
uint64_t states;
uint32_t uncomp_size;
@@ -683,6 +711,12 @@ static void __init check_new_xstate(struct xcheck_state *s, uint64_t new)
s->states, &new, hw_size, s->comp_size);
s->comp_size = hw_size;
+
+ xen_size = xstate_compressed_size(s->states);
+
+ if ( xen_size != hw_size )
+ panic("XSTATE 0x%016"PRIx64", compressed hw size %#x != xen size %#x\n",
+ s->states, hw_size, xen_size);
}
else
BUG_ON(hw_size); /* Compressed size reported, but no XSAVEC ? */
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 7/7] x86/defns: Clean up X86_{XCR0,XSS}_* constants
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
` (5 preceding siblings ...)
2024-05-23 11:16 ` [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves Andrew Cooper
@ 2024-05-23 11:16 ` Andrew Cooper
2024-05-23 15:46 ` Jan Beulich
6 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-05-23 11:16 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
With the exception of one case in read_bndcfgu() which can use ilog2(),
the *_POS defines are unused.
X86_XCR0_X87 is the name used by both the SDM and APM, rather than
X86_XCR0_FP.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
v3:
* New
---
xen/arch/x86/i387.c | 2 +-
xen/arch/x86/include/asm/x86-defns.h | 32 ++++++++++------------------
xen/arch/x86/include/asm/xstate.h | 4 ++--
xen/arch/x86/xstate.c | 18 ++++++++--------
4 files changed, 23 insertions(+), 33 deletions(-)
diff --git a/xen/arch/x86/i387.c b/xen/arch/x86/i387.c
index 7a4297cc921e..fcdee10a6e69 100644
--- a/xen/arch/x86/i387.c
+++ b/xen/arch/x86/i387.c
@@ -369,7 +369,7 @@ void vcpu_setup_fpu(struct vcpu *v, struct xsave_struct *xsave_area,
{
v->arch.xsave_area->xsave_hdr.xstate_bv &= ~XSTATE_FP_SSE;
if ( fcw_default != FCW_DEFAULT )
- v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_FP;
+ v->arch.xsave_area->xsave_hdr.xstate_bv |= X86_XCR0_X87;
}
}
diff --git a/xen/arch/x86/include/asm/x86-defns.h b/xen/arch/x86/include/asm/x86-defns.h
index d7602ab225c4..3bcdbaccd3aa 100644
--- a/xen/arch/x86/include/asm/x86-defns.h
+++ b/xen/arch/x86/include/asm/x86-defns.h
@@ -79,25 +79,16 @@
/*
* XSTATE component flags in XCR0 | MSR_XSS
*/
-#define X86_XCR0_FP_POS 0
-#define X86_XCR0_FP (1ULL << X86_XCR0_FP_POS)
-#define X86_XCR0_SSE_POS 1
-#define X86_XCR0_SSE (1ULL << X86_XCR0_SSE_POS)
-#define X86_XCR0_YMM_POS 2
-#define X86_XCR0_YMM (1ULL << X86_XCR0_YMM_POS)
-#define X86_XCR0_BNDREGS_POS 3
-#define X86_XCR0_BNDREGS (1ULL << X86_XCR0_BNDREGS_POS)
-#define X86_XCR0_BNDCSR_POS 4
-#define X86_XCR0_BNDCSR (1ULL << X86_XCR0_BNDCSR_POS)
-#define X86_XCR0_OPMASK_POS 5
-#define X86_XCR0_OPMASK (1ULL << X86_XCR0_OPMASK_POS)
-#define X86_XCR0_ZMM_POS 6
-#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_XCR0_X87 (_AC(1, ULL) << 0)
+#define X86_XCR0_SSE (_AC(1, ULL) << 1)
+#define X86_XCR0_YMM (_AC(1, ULL) << 2)
+#define X86_XCR0_BNDREGS (_AC(1, ULL) << 3)
+#define X86_XCR0_BNDCSR (_AC(1, ULL) << 4)
+#define X86_XCR0_OPMASK (_AC(1, ULL) << 5)
+#define X86_XCR0_ZMM (_AC(1, ULL) << 6)
+#define X86_XCR0_HI_ZMM (_AC(1, ULL) << 7)
#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_XCR0_PKRU (_AC(1, ULL) << 9)
#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)
@@ -107,11 +98,10 @@
#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_LWP (_AC(1, ULL) << 62)
#define X86_XCR0_STATES \
- (X86_XCR0_FP | X86_XCR0_SSE | X86_XCR0_YMM | X86_XCR0_BNDREGS | \
+ (X86_XCR0_X87 | 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 | \
diff --git a/xen/arch/x86/include/asm/xstate.h b/xen/arch/x86/include/asm/xstate.h
index da1d89d2f416..f4a8e5f814a0 100644
--- a/xen/arch/x86/include/asm/xstate.h
+++ b/xen/arch/x86/include/asm/xstate.h
@@ -29,8 +29,8 @@ extern uint32_t mxcsr_mask;
#define XSAVE_HDR_OFFSET FXSAVE_SIZE
#define XSTATE_AREA_MIN_SIZE (FXSAVE_SIZE + XSAVE_HDR_SIZE)
-#define XSTATE_FP_SSE (X86_XCR0_FP | X86_XCR0_SSE)
-#define XCNTXT_MASK (X86_XCR0_FP | X86_XCR0_SSE | X86_XCR0_YMM | \
+#define XSTATE_FP_SSE (X86_XCR0_X87 | X86_XCR0_SSE)
+#define XCNTXT_MASK (X86_XCR0_X87 | X86_XCR0_SSE | X86_XCR0_YMM | \
X86_XCR0_OPMASK | X86_XCR0_ZMM | X86_XCR0_HI_ZMM | \
XSTATE_NONLAZY)
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index 7b7f2dcaf651..0ed2541665b3 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -313,7 +313,7 @@ void xsave(struct vcpu *v, uint64_t mask)
"=m" (*ptr), \
"a" (lmask), "d" (hmask), "D" (ptr))
- if ( fip_width == 8 || !(mask & X86_XCR0_FP) )
+ if ( fip_width == 8 || !(mask & X86_XCR0_X87) )
{
XSAVE("0x48,");
}
@@ -366,7 +366,7 @@ void xsave(struct vcpu *v, uint64_t mask)
fip_width = 8;
}
#undef XSAVE
- if ( mask & X86_XCR0_FP )
+ if ( mask & X86_XCR0_X87 )
ptr->fpu_sse.x[FPU_WORD_SIZE_OFFSET] = fip_width;
}
@@ -558,7 +558,7 @@ void xstate_free_save_area(struct vcpu *v)
static bool valid_xcr0(uint64_t xcr0)
{
/* FP must be unconditionally set. */
- if ( !(xcr0 & X86_XCR0_FP) )
+ if ( !(xcr0 & X86_XCR0_X87) )
return false;
/* YMM depends on SSE. */
@@ -599,7 +599,7 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
return 0;
- if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )
+ if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_X87) )
return size;
/*
@@ -607,7 +607,7 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
* maximum offset+size. Some states (e.g. LWP, APX_F) are out-of-order
* with respect their index.
*/
- xcr0 &= ~(X86_XCR0_SSE | X86_XCR0_FP);
+ xcr0 &= ~(X86_XCR0_SSE | X86_XCR0_X87);
for_each_set_bit ( i, &xcr0, 63 )
{
const struct xstate_component *c = &raw_cpu_policy.xstate.comp[i];
@@ -628,14 +628,14 @@ unsigned int xstate_compressed_size(uint64_t xstates)
if ( xstates == 0 ) /* TODO: clean up paths passing 0 in here. */
return 0;
- if ( xstates <= (X86_XCR0_SSE | X86_XCR0_FP) )
+ if ( xstates <= (X86_XCR0_SSE | X86_XCR0_X87) )
return size;
/*
* For the compressed size, every component matters. Some componenets are
* rounded up to 64 first.
*/
- xstates &= ~(X86_XCR0_SSE | X86_XCR0_FP);
+ xstates &= ~(X86_XCR0_SSE | X86_XCR0_X87);
for_each_set_bit ( i, &xstates, 63 )
{
const struct xstate_component *c = &raw_cpu_policy.xstate.comp[i];
@@ -748,7 +748,7 @@ static void __init noinline xstate_check_sizes(void)
* layout compatibility with Intel and having a knock-on effect on all
* subsequent states.
*/
- check_new_xstate(&s, X86_XCR0_SSE | X86_XCR0_FP);
+ check_new_xstate(&s, X86_XCR0_SSE | X86_XCR0_X87);
if ( cpu_has_avx )
check_new_xstate(&s, X86_XCR0_YMM);
@@ -1000,7 +1000,7 @@ uint64_t read_bndcfgu(void)
: "=m" (*xstate)
: "a" (X86_XCR0_BNDCSR), "d" (0), "D" (xstate) );
- bndcsr = (void *)xstate + xstate_offsets[X86_XCR0_BNDCSR_POS];
+ bndcsr = (void *)xstate + xstate_offsets[ilog2(X86_XCR0_BNDCSR)];
}
if ( cr0 & X86_CR0_TS )
--
2.30.2
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache
2024-05-23 11:16 ` [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache Andrew Cooper
@ 2024-05-23 15:09 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-05-23 15:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 23.05.2024 13:16, Andrew Cooper wrote:
> The clobbering of this_cpu(xcr0) and this_cpu(xss) to architecturally invalid
> values is to force the subsequent set_xcr0() and set_msr_xss() to reload the
> hardware register.
>
> While XCR0 is reloaded in xstate_init(), MSR_XSS isn't. This causes
> get_msr_xss() to return the invalid value, and logic of the form:
>
> old = get_msr_xss();
> set_msr_xss(new);
> ...
> set_msr_xss(old);
>
> to try and restore the architecturally invalid value.
>
> The architecturally invalid value must be purged from the cache, meaning the
> hardware register must be written at least once. This in turn highlights that
> the invalid value must only be used in the case that the hardware register is
> available.
>
> Fixes: f7f4a523927f ("x86/xstate: reset cached register values on resume")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
However, I view it as pretty unfair that now I will need to re-base
https://lists.xen.org/archives/html/xen-devel/2021-04/msg01336.html
over ...
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -641,13 +641,6 @@ void xstate_init(struct cpuinfo_x86 *c)
> return;
> }
>
> - /*
> - * Zap the cached values to make set_xcr0() and set_msr_xss() really
> - * write it.
> - */
> - this_cpu(xcr0) = 0;
> - this_cpu(xss) = ~0;
> -
> cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
> feature_mask = (((u64)edx << 32) | eax) & XCNTXT_MASK;
> BUG_ON(!valid_xcr0(feature_mask));
> @@ -657,8 +650,19 @@ void xstate_init(struct cpuinfo_x86 *c)
> * Set CR4_OSXSAVE and run "cpuid" to get xsave_cntxt_size.
> */
> set_in_cr4(X86_CR4_OSXSAVE);
> +
> + /*
> + * Zap the cached values to make set_xcr0() and set_msr_xss() really write
> + * the hardware register.
> + */
> + this_cpu(xcr0) = 0;
> if ( !set_xcr0(feature_mask) )
> BUG();
> + if ( cpu_has_xsaves )
> + {
> + this_cpu(xss) = ~0;
> + set_msr_xss(0);
> + }
... this change, kind of breaking again your nice arrangement. Seeing for how
long that change has been pending, it _really_ should have gone in ahead of
this one, with you then sorting how you'd like things to be arranged in the
combined result, rather than me re-posting and then either again not getting
any feedback for years, or you disliking what I've done. Oh well ...
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
2024-05-23 11:16 ` [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot Andrew Cooper
@ 2024-05-23 15:34 ` Jan Beulich
2024-06-14 13:56 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-05-23 15:34 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
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_PKS 0x01000000 /* 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, &new, 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, &new, hw_size, s->comp_size);
> +
> + s->comp_size = hw_size;
> + }
> + else
> + BUG_ON(hw_size); /* Compressed size reported, but no XSAVEC ? */
I'm not quite happy with this being fatal to booting. Maybe just WARN_ON()?
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot
2024-05-23 11:16 ` [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot Andrew Cooper
@ 2024-05-23 15:44 ` Jan Beulich
2024-06-14 18:26 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-05-23 15:44 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 23.05.2024 13:16, Andrew Cooper wrote:
> This is a tangle, but it's a small step in the right direction.
>
> xstate_init() is shortly going to want data from the Raw policy.
> calculate_raw_cpu_policy() is sufficiently separate from the other policies to
> be safe to do.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Would you mind taking a look at
https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html
to make clear (to me at least) in how far we can perhaps find common grounds
on what wants doing when? (Of course the local version I have has been
constantly re-based, so some of the function names would have changed from
what's visible there.)
> --- a/xen/arch/x86/cpu-policy.c
> +++ b/xen/arch/x86/cpu-policy.c
> @@ -845,7 +845,6 @@ static void __init calculate_hvm_def_policy(void)
>
> void __init init_guest_cpu_policies(void)
> {
> - calculate_raw_cpu_policy();
> calculate_host_policy();
>
> if ( IS_ENABLED(CONFIG_PV) )
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1888,7 +1888,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>
> tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */
>
> - identify_cpu(&boot_cpu_data);
> + calculate_raw_cpu_policy(); /* Needs microcode. No other dependenices. */
> +
> + identify_cpu(&boot_cpu_data); /* Needs microcode and raw policy. */
You don't introduce any dependency on raw policy here, and there cannot possibly
have been such a dependency before (unless there was a bug somewhere). Therefore
I consider this latter comment misleading at this point.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 7/7] x86/defns: Clean up X86_{XCR0,XSS}_* constants
2024-05-23 11:16 ` [PATCH 7/7] x86/defns: Clean up X86_{XCR0,XSS}_* constants Andrew Cooper
@ 2024-05-23 15:46 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-05-23 15:46 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 23.05.2024 13:16, Andrew Cooper wrote:
> With the exception of one case in read_bndcfgu() which can use ilog2(),
> the *_POS defines are unused.
>
> X86_XCR0_X87 is the name used by both the SDM and APM, rather than
> X86_XCR0_FP.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
2024-05-23 11:16 ` [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
@ 2024-05-23 16:09 ` Jan Beulich
2024-06-14 13:56 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-05-23 16:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 23.05.2024 13:16, Andrew Cooper wrote:
> @@ -611,6 +587,40 @@ static bool valid_xcr0(uint64_t xcr0)
> return true;
> }
>
> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
> +{
> + unsigned int size = XSTATE_AREA_MIN_SIZE, i;
> +
> + ASSERT((xcr0 & ~X86_XCR0_STATES) == 0);
I'm puzzled by the combination of this assertion and ...
> + if ( xcr0 == xfeature_mask )
> + return xsave_cntxt_size;
... this conditional return. Yes, right now we don't support/use any XSS
components, but without any comment the assertion looks overly restrictive
to me.
> @@ -818,14 +834,14 @@ void xstate_init(struct cpuinfo_x86 *c)
> * xsave_cntxt_size is the max size required by enabled features.
> * We know FP/SSE and YMM about eax, and nothing about edx at present.
> */
> - xsave_cntxt_size = hw_uncompressed_size(feature_mask);
> + xsave_cntxt_size = cpuid_count_ebx(0xd, 0);
> printk("xstate: size: %#x and states: %#"PRIx64"\n",
> xsave_cntxt_size, xfeature_mask);
> }
> else
> {
> BUG_ON(xfeature_mask != feature_mask);
> - BUG_ON(xsave_cntxt_size != hw_uncompressed_size(feature_mask));
> + BUG_ON(xsave_cntxt_size != cpuid_count_ebx(0xd, 0));
> }
Hmm, this may make re-basing of said earlier patch touching this code yet
more interesting. Or maybe it actually simplifies things, will need to see
... The overall comment remains though: Patches pending for so long should
really take priority over creating yet more new ones. But what do I do - I
can't enforce this, unless I was now going to block your work the same way.
Which I don't mean to do.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves
2024-05-23 11:16 ` [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves Andrew Cooper
@ 2024-05-23 16:16 ` Jan Beulich
2024-06-14 14:06 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-05-23 16:16 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 23.05.2024 13:16, Andrew Cooper wrote:
> First, if XSAVE is available in hardware but not visible to the guest, the
> dynamic leaves shouldn't be filled in.
>
> Second, the comment concerning XSS state is wrong. VT-x doesn't manage
> host/guest state automatically, but there is provision for "host only" bits to
> be set, so the implications are still accurate.
>
> Introduce xstate_compressed_size() to mirror the uncompressed one. Cross
> check it at boot.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Irrespective ...
> v3:
> * Adjust commit message about !XSAVE guests
> * Rebase over boot time cross check
> * Use raw policy
... it should probably have occurred to me earlier on to ask: Why raw policy?
Isn't the host one the more appropriate one to use for any kind of internal
decisions?
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
2024-05-23 15:34 ` Jan Beulich
@ 2024-06-14 13:56 ` Andrew Cooper
2024-06-17 10:08 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-06-14 13:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
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_PKS 0x01000000 /* 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, &new, 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, &new, hw_size, s->comp_size);
>> +
>> + s->comp_size = hw_size;
>> + }
>> + else
>> + BUG_ON(hw_size); /* Compressed size reported, but no XSAVEC ? */
> I'm not quite happy with this being fatal to booting. Maybe just WARN_ON()?
It's going to trigger on every pass. I've reworked it to be an
opencoded WARN_ONCE() (as we don't have this construct yet), but it's
ended up as a plain WARN().
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
2024-05-23 16:09 ` Jan Beulich
@ 2024-06-14 13:56 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-06-14 13:56 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 23/05/2024 5:09 pm, Jan Beulich wrote:
> On 23.05.2024 13:16, Andrew Cooper wrote:
>> @@ -611,6 +587,40 @@ static bool valid_xcr0(uint64_t xcr0)
>> return true;
>> }
>>
>> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
>> +{
>> + unsigned int size = XSTATE_AREA_MIN_SIZE, i;
>> +
>> + ASSERT((xcr0 & ~X86_XCR0_STATES) == 0);
> I'm puzzled by the combination of this assertion and ...
>
>> + if ( xcr0 == xfeature_mask )
>> + return xsave_cntxt_size;
> ... this conditional return. Yes, right now we don't support/use any XSS
> components, but without any comment the assertion looks overly restrictive
> to me.
The ASSERT() is to cover a bug I found during testing.
It is a hard error to pass in non-XCR0 states. XSS states do not exist
in an uncompressed image, and have a base of 0, which ends up hitting a
later assertion.
This snippet with xfeature_mask is just code motion from
xstate_ctxt_size(), expressed as an addition because of diff. It was to
save a double XCR0 write in a perceived common case.
But, your AMX series makes both xfeature_mask and xsave_cntxt_size bogus
by there no longer being a uniform size of the save area, so I can
probably get away with simply dropping it here.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves
2024-05-23 16:16 ` Jan Beulich
@ 2024-06-14 14:06 ` Andrew Cooper
0 siblings, 0 replies; 22+ messages in thread
From: Andrew Cooper @ 2024-06-14 14:06 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Wei Liu, Xen-devel
On 23/05/2024 5:16 pm, Jan Beulich wrote:
> On 23.05.2024 13:16, Andrew Cooper wrote:
>> First, if XSAVE is available in hardware but not visible to the guest, the
>> dynamic leaves shouldn't be filled in.
>>
>> Second, the comment concerning XSS state is wrong. VT-x doesn't manage
>> host/guest state automatically, but there is provision for "host only" bits to
>> be set, so the implications are still accurate.
>>
>> Introduce xstate_compressed_size() to mirror the uncompressed one. Cross
>> check it at boot.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> Irrespective ...
>
>> v3:
>> * Adjust commit message about !XSAVE guests
>> * Rebase over boot time cross check
>> * Use raw policy
> ... it should probably have occurred to me earlier on to ask: Why raw policy?
> Isn't the host one the more appropriate one to use for any kind of internal
> decisions?
State information is identical in all policies. It's the ABI of the
X{SAVE,RSTOR}* instructions.
Beyond that, consistency.
xstate_uncompressed_size() does strictly need to be the raw policy,
because it is used by recalculate_xstate() to calculate the host policy.
xstate_compressed_size() doesn't have the same restriction, but should
use the same source of data.
Finally, xstate_{un,}compressed_size() aren't really tied to a choice of
features in the first place. They shouldn't be limited to the
host_policy's subset of active features.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot
2024-05-23 15:44 ` Jan Beulich
@ 2024-06-14 18:26 ` Andrew Cooper
2024-06-17 10:25 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-06-14 18:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 23/05/2024 4:44 pm, Jan Beulich wrote:
> On 23.05.2024 13:16, Andrew Cooper wrote:
>> This is a tangle, but it's a small step in the right direction.
>>
>> xstate_init() is shortly going to want data from the Raw policy.
>> calculate_raw_cpu_policy() is sufficiently separate from the other policies to
>> be safe to do.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Would you mind taking a look at
> https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html
> to make clear (to me at least) in how far we can perhaps find common grounds
> on what wants doing when? (Of course the local version I have has been
> constantly re-based, so some of the function names would have changed from
> what's visible there.)
That's been covered several times, at least in part.
I want to eventually move the host policy too, but I'm not willing to
compound the mess we've currently got just to do it earlier. It's just
creating even more obstacles to doing it nicely.
Nothing in this series needs (or indeed should) use the host policy.
The same is true of your AMX series. You're (correctly) breaking the
uniform allocation size and (when policy selection is ordered WRT vCPU
creation, as discussed) it becomes solely depend on the guest policy.
xsave.c really has no legitimate use for the host policy once the
uniform allocation size aspect has gone away.
>> --- a/xen/arch/x86/cpu-policy.c
>> +++ b/xen/arch/x86/cpu-policy.c
>> @@ -845,7 +845,6 @@ static void __init calculate_hvm_def_policy(void)
>>
>> void __init init_guest_cpu_policies(void)
>> {
>> - calculate_raw_cpu_policy();
>> calculate_host_policy();
>>
>> if ( IS_ENABLED(CONFIG_PV) )
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -1888,7 +1888,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>>
>> tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */
>>
>> - identify_cpu(&boot_cpu_data);
>> + calculate_raw_cpu_policy(); /* Needs microcode. No other dependenices. */
>> +
>> + identify_cpu(&boot_cpu_data); /* Needs microcode and raw policy. */
> You don't introduce any dependency on raw policy here, and there cannot possibly
> have been such a dependency before (unless there was a bug somewhere). Therefore
> I consider this latter comment misleading at this point.
It's made true by the next patch, and I'm not included to split the
comment across two patches which are going to be committed together in a
unit.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot
2024-06-14 13:56 ` Andrew Cooper
@ 2024-06-17 10:08 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-06-17 10:08 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
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_PKS 0x01000000 /* 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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot
2024-06-14 18:26 ` Andrew Cooper
@ 2024-06-17 10:25 ` Jan Beulich
2024-06-17 17:30 ` Andrew Cooper
0 siblings, 1 reply; 22+ messages in thread
From: Jan Beulich @ 2024-06-17 10:25 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 14.06.2024 20:26, Andrew Cooper wrote:
> On 23/05/2024 4:44 pm, Jan Beulich wrote:
>> On 23.05.2024 13:16, Andrew Cooper wrote:
>>> This is a tangle, but it's a small step in the right direction.
>>>
>>> xstate_init() is shortly going to want data from the Raw policy.
>>> calculate_raw_cpu_policy() is sufficiently separate from the other policies to
>>> be safe to do.
>>>
>>> No functional change.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Would you mind taking a look at
>> https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html
>> to make clear (to me at least) in how far we can perhaps find common grounds
>> on what wants doing when? (Of course the local version I have has been
>> constantly re-based, so some of the function names would have changed from
>> what's visible there.)
>
> That's been covered several times, at least in part.
>
> I want to eventually move the host policy too, but I'm not willing to
> compound the mess we've currently got just to do it earlier. It's just
> creating even more obstacles to doing it nicely.
>
> Nothing in this series needs (or indeed should) use the host policy.
Hmm, I'm irritated: You talk about host policy here, ...
> The same is true of your AMX series. You're (correctly) breaking the
> uniform allocation size and (when policy selection is ordered WRT vCPU
> creation, as discussed) it becomes solely depend on the guest policy.
... then guest policy, and ...
> xsave.c really has no legitimate use for the host policy once the
> uniform allocation size aspect has gone away.
... then host policy again. Whereas my patch switches to using the raw
policy, simply to eliminate redundant data. And your patch here is about
collecting raw policy earlier, too, for that to become usable by
xstate_init(). Differences between your any my variant are when exactly
raw policy collection happens, and that mine _additionally_ calculates
host policy a first time right after having calculated the raw one. My
patch specifically does not use the host policy in xstate_init(), nor in
the two new macros that are being introduced.
In the end it sounds like all you object to is my patch calculating the
host policy (a first time) earlier, too. As the description there says,
a subsequent change in that series needs this movement anyway. If some
suitable replacement for that dependency exists, I'm sure that early
calculation could be left out of the patch referenced above, if that's
indeed the sole concern.
>>> --- a/xen/arch/x86/cpu-policy.c
>>> +++ b/xen/arch/x86/cpu-policy.c
>>> @@ -845,7 +845,6 @@ static void __init calculate_hvm_def_policy(void)
>>>
>>> void __init init_guest_cpu_policies(void)
>>> {
>>> - calculate_raw_cpu_policy();
>>> calculate_host_policy();
>>>
>>> if ( IS_ENABLED(CONFIG_PV) )
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1888,7 +1888,9 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
>>>
>>> tsx_init(); /* Needs microcode. May change HLE/RTM feature bits. */
>>>
>>> - identify_cpu(&boot_cpu_data);
>>> + calculate_raw_cpu_policy(); /* Needs microcode. No other dependenices. */
>>> +
>>> + identify_cpu(&boot_cpu_data); /* Needs microcode and raw policy. */
>> You don't introduce any dependency on raw policy here, and there cannot possibly
>> have been such a dependency before (unless there was a bug somewhere). Therefore
>> I consider this latter comment misleading at this point.
>
> It's made true by the next patch, and I'm not included to split the
> comment across two patches which are going to be committed together in a
> unit.
Which is fine, so long as this is then not done silently, leaving it to
reviewers to notice (or not). IOW please: Just mention anomalies like this
in half a sentence in the description. (Committing as a unit is also an
uncertain thing, as long as that's not put forth as a strict requirement
somewhere. We do partial commits of series all the time, after all.)
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot
2024-06-17 10:25 ` Jan Beulich
@ 2024-06-17 17:30 ` Andrew Cooper
2024-06-18 7:04 ` Jan Beulich
0 siblings, 1 reply; 22+ messages in thread
From: Andrew Cooper @ 2024-06-17 17:30 UTC (permalink / raw)
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 17/06/2024 11:25 am, Jan Beulich wrote:
> On 14.06.2024 20:26, Andrew Cooper wrote:
>> On 23/05/2024 4:44 pm, Jan Beulich wrote:
>>> On 23.05.2024 13:16, Andrew Cooper wrote:
>>>> This is a tangle, but it's a small step in the right direction.
>>>>
>>>> xstate_init() is shortly going to want data from the Raw policy.
>>>> calculate_raw_cpu_policy() is sufficiently separate from the other policies to
>>>> be safe to do.
>>>>
>>>> No functional change.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Would you mind taking a look at
>>> https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html
>>> to make clear (to me at least) in how far we can perhaps find common grounds
>>> on what wants doing when? (Of course the local version I have has been
>>> constantly re-based, so some of the function names would have changed from
>>> what's visible there.)
>> That's been covered several times, at least in part.
>>
>> I want to eventually move the host policy too, but I'm not willing to
>> compound the mess we've currently got just to do it earlier. It's just
>> creating even more obstacles to doing it nicely.
>>
>> Nothing in this series needs (or indeed should) use the host policy.
> Hmm, I'm irritated: You talk about host policy here, ...
>
>> The same is true of your AMX series. You're (correctly) breaking the
>> uniform allocation size and (when policy selection is ordered WRT vCPU
>> creation, as discussed) it becomes solely depend on the guest policy.
> ... then guest policy, and ...
>
>> xsave.c really has no legitimate use for the host policy once the
>> uniform allocation size aspect has gone away.
> ... then host policy again.
Yes. Notice how host policy is always referred to in the negative.
The raw policy should be used for everything pertaining to the
instruction ABI itself, and the guest policy for sizing information.
> Whereas my patch switches to using the raw
> policy, simply to eliminate redundant data.
Except it doesn't. The latest posted version of your series contains:
-static u32 __read_mostly xsave_cntxt_size;
+#define xsave_cntxt_size (host_cpuid_policy.xstate.max_size | 0)
and you've even stated that I should have acked this patch simply on its
age.
I acked the patches that were good, and you did committed them at the
time. Then I put together this series to fix the bugs the latent bugs
which you were making less latent; this series really is the same one
discussed back then, and really does have some 2020/2021 author dates in it.
It is, AFAICT, not safe to move the calculation of the host policy as
early as you did, without arranging for setup_{force,clear}_cap() to
edit the host policy synchronously. Recalculating a second time later
isn't good enough, because you've created an asymmetry for most of boot
between two pieces of state which are supposed to be in sync, and that
you're intentionally starting to use. So yes - while I do intend to
eventually make the host policy usable that early too, I'm really not
happy doing so in a manner that has "ticking timebomb" written all over it.
As to xsave_cntxt_size, it can (and should) be eliminated entirely when
xstate_alloc_save_area() can use the guest policy to size the allocation.
~Andrew
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot
2024-06-17 17:30 ` Andrew Cooper
@ 2024-06-18 7:04 ` Jan Beulich
0 siblings, 0 replies; 22+ messages in thread
From: Jan Beulich @ 2024-06-18 7:04 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 17.06.2024 19:30, Andrew Cooper wrote:
> On 17/06/2024 11:25 am, Jan Beulich wrote:
>> On 14.06.2024 20:26, Andrew Cooper wrote:
>>> On 23/05/2024 4:44 pm, Jan Beulich wrote:
>>>> On 23.05.2024 13:16, Andrew Cooper wrote:
>>>>> This is a tangle, but it's a small step in the right direction.
>>>>>
>>>>> xstate_init() is shortly going to want data from the Raw policy.
>>>>> calculate_raw_cpu_policy() is sufficiently separate from the other policies to
>>>>> be safe to do.
>>>>>
>>>>> No functional change.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Would you mind taking a look at
>>>> https://lists.xen.org/archives/html/xen-devel/2021-04/msg01335.html
>>>> to make clear (to me at least) in how far we can perhaps find common grounds
>>>> on what wants doing when? (Of course the local version I have has been
>>>> constantly re-based, so some of the function names would have changed from
>>>> what's visible there.)
>>> That's been covered several times, at least in part.
>>>
>>> I want to eventually move the host policy too, but I'm not willing to
>>> compound the mess we've currently got just to do it earlier. It's just
>>> creating even more obstacles to doing it nicely.
>>>
>>> Nothing in this series needs (or indeed should) use the host policy.
>> Hmm, I'm irritated: You talk about host policy here, ...
>>
>>> The same is true of your AMX series. You're (correctly) breaking the
>>> uniform allocation size and (when policy selection is ordered WRT vCPU
>>> creation, as discussed) it becomes solely depend on the guest policy.
>> ... then guest policy, and ...
>>
>>> xsave.c really has no legitimate use for the host policy once the
>>> uniform allocation size aspect has gone away.
>> ... then host policy again.
>
> Yes. Notice how host policy is always referred to in the negative.
>
> The raw policy should be used for everything pertaining to the
> instruction ABI itself, and the guest policy for sizing information.
>
>> Whereas my patch switches to using the raw
>> policy, simply to eliminate redundant data.
>
> Except it doesn't. The latest posted version of your series contains:
>
> -static u32 __read_mostly xsave_cntxt_size;
> +#define xsave_cntxt_size (host_cpuid_policy.xstate.max_size | 0)
That's "x86/xstate: replace xsave_cntxt_size and drop XCNTXT_MASK", but
what I referenced in the context here is "x86/xstate: drop
xstate_offsets[] and xstate_sizes[]". If there use of the host policy
is wrong in your opinion, then can you please reply there to clarify
what's wrong with the justification for doing so? (See also below.)
> and you've even stated that I should have acked this patch simply on its
> age.
Well, it's not quite "simply on its age". Part of me thinks that our
rules should permit for things to go in when no-one looked at them
despite reminders, but the other part of me knows that then we'll move
back to what we had in the old days, where all kinds of (subtle or
supposedly obvious) breakage would be introduced. So it's more like
"considering its age, it really should have had replies". Anyway, there
was now a vague promise that this series is going to be helped make
progress in the 4.20 cycle.
> I acked the patches that were good, and you did committed them at the
> time. Then I put together this series to fix the bugs the latent bugs
> which you were making less latent; this series really is the same one
> discussed back then, and really does have some 2020/2021 author dates in it.
>
>
> It is, AFAICT, not safe to move the calculation of the host policy as
> early as you did, without arranging for setup_{force,clear}_cap() to
> edit the host policy synchronously. Recalculating a second time later
> isn't good enough, because you've created an asymmetry for most of boot
> between two pieces of state which are supposed to be in sync, and that
> you're intentionally starting to use. So yes - while I do intend to
> eventually make the host policy usable that early too, I'm really not
> happy doing so in a manner that has "ticking timebomb" written all over it.
That's a fair concern. My counter argument is that right now the host
policy can't possibly be used ahead of when its calculated prior to
that patch (i.e. ahead of the 2nd calculation after the patch). Yes,
care will need to be taken (for the time being) to, between the two
points, only access parts which can't change again during the 2nd
calculation.
Considering the plans to invoke identify_cpu() earlier, it simply didn't
appear necessary to, transiently, change setup_{force,clear}_cap().
> As to xsave_cntxt_size, it can (and should) be eliminated entirely when
> xstate_alloc_save_area() can use the guest policy to size the allocation.
Again I'm confused, I'm afraid: xsave_cntxt_size isn't really used for
much guest-ish right now; its main use is in xstate_init(). Hence why
that other patch is replacing it by use of the host policy. Perhaps
you're simply suggesting that its present uses aren't quite right then;
as you say, that sole truly guest related use in xstate_alloc_save_area()
is there only because right now we need to over-estimate the eventual
size needed. For that purpose, host policy is the right thing to use, imo.
xsave_cntxt_size going away altogether at some point is no reason to,
transiently, switch it from being an actual variable to referencing the
host policy. It's just that if it can be made go away earlier, that part
of that other patch could then simply be dropped.
Jan
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2024-06-18 7:04 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-23 11:16 [PATCH for-4.19 v3 0/7] x86/xstate: Fixes to size calculations Andrew Cooper
2024-05-23 11:16 ` [PATCH 1/7] x86/xstate: Fix initialisation of XSS cache Andrew Cooper
2024-05-23 15:09 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 2/7] x86/xstate: Cross-check dynamic XSTATE sizes at boot Andrew Cooper
2024-05-23 15:34 ` Jan Beulich
2024-06-14 13:56 ` Andrew Cooper
2024-06-17 10:08 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 3/7] x86/boot: Collect the Raw CPU Policy earlier on boot Andrew Cooper
2024-05-23 15:44 ` Jan Beulich
2024-06-14 18:26 ` Andrew Cooper
2024-06-17 10:25 ` Jan Beulich
2024-06-17 17:30 ` Andrew Cooper
2024-06-18 7:04 ` Jan Beulich
2024-05-23 11:16 ` [PATCH 4/7] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
2024-05-23 16:09 ` Jan Beulich
2024-06-14 13:56 ` Andrew Cooper
2024-05-23 11:16 ` [PATCH 5/7] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
2024-05-23 11:16 ` [PATCH 6/7] x86/cpuid: Fix handling of XSAVE dynamic leaves Andrew Cooper
2024-05-23 16:16 ` Jan Beulich
2024-06-14 14:06 ` Andrew Cooper
2024-05-23 11:16 ` [PATCH 7/7] x86/defns: Clean up X86_{XCR0,XSS}_* constants Andrew Cooper
2024-05-23 15:46 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.