* [PATCH v2 0/4] x86/xstate: Fixes to size calculations
@ 2024-04-29 18:28 Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states() Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Andrew Cooper @ 2024-04-29 18:28 UTC (permalink / raw
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Various fixes and improvements to xsave image size calculations.
Since v1:
* Rebase over exposing XSAVEC. This has highlighted several latent bugs.
* Rework the uncompressed size handling. LWP / APX_F make for much sadness.
Be aware that Intel and AMD have different ABIs for the xstate layout.
Andrew Cooper (4):
x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states()
x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
x86/cpu-policy: Simplify recalculate_xstate()
x86/cpuid: Fix handling of xsave dynamic leaves
xen/arch/x86/cpu-policy.c | 55 ++++++---------
xen/arch/x86/cpuid.c | 24 +++----
xen/arch/x86/domctl.c | 2 +-
xen/arch/x86/hvm/hvm.c | 12 +++-
xen/arch/x86/include/asm/xstate.h | 4 +-
xen/arch/x86/xstate.c | 111 ++++++++++++++++++++++++++++--
6 files changed, 145 insertions(+), 63 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states()
2024-04-29 18:28 [PATCH v2 0/4] x86/xstate: Fixes to size calculations Andrew Cooper
@ 2024-04-29 18:28 ` Andrew Cooper
2024-05-02 12:08 ` Jan Beulich
2024-04-29 18:28 ` [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2024-04-29 18:28 UTC (permalink / raw
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
HVM_CPU_XSAVE_SIZE() may rewrite %xcr0 twice. Defer the caluclation it until
after we've decided to write out an XSAVE record.
Note in hvm_load_cpu_xsave_states() that there were versions of Xen which
wrote out a useless XSAVE record. This sadly limits out ability to tidy up
the existing infrastructure. Also leave a note in xstate_ctxt_size() that 0
still needs tolerating for now.
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>
v2:
* New
---
xen/arch/x86/hvm/hvm.c | 10 ++++++++--
xen/arch/x86/xstate.c | 2 +-
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0ce45b177cf4..9594e0a5c530 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1197,12 +1197,13 @@ static int cf_check hvm_save_cpu_xsave_states(
struct vcpu *v, hvm_domain_context_t *h)
{
struct hvm_hw_cpu_xsave *ctxt;
- unsigned int size = HVM_CPU_XSAVE_SIZE(v->arch.xcr0_accum);
+ unsigned int size;
int err;
- if ( !cpu_has_xsave || !xsave_enabled(v) )
+ if ( !xsave_enabled(v) )
return 0; /* do nothing */
+ size = HVM_CPU_XSAVE_SIZE(v->arch.xcr0_accum);
err = _hvm_init_entry(h, CPU_XSAVE_CODE, v->vcpu_id, size);
if ( err )
return err;
@@ -1255,6 +1256,11 @@ static int cf_check hvm_load_cpu_xsave_states(
if ( !cpu_has_xsave )
return -EOPNOTSUPP;
+ /*
+ * Note: Xen prior to 4.12 would write out empty XSAVE records for VMs
+ * running on XSAVE-capable hardware but without XSAVE active.
+ */
+
/* Customized checking for entry since our entry is of variable length */
desc = (struct hvm_save_descriptor *)&h->data[h->cur];
if ( sizeof (*desc) > h->size - h->cur)
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index cf94761d0542..99cedb4f5e24 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -573,7 +573,7 @@ unsigned int xstate_ctxt_size(u64 xcr0)
if ( xcr0 == xfeature_mask )
return xsave_cntxt_size;
- if ( xcr0 == 0 )
+ if ( xcr0 == 0 ) /* TODO: clean up paths passing 0 in here. */
return 0;
return hw_uncompressed_size(xcr0);
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
2024-04-29 18:28 [PATCH v2 0/4] x86/xstate: Fixes to size calculations Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states() Andrew Cooper
@ 2024-04-29 18:28 ` Andrew Cooper
2024-05-02 12:19 ` Jan Beulich
2024-04-29 18:28 ` [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves Andrew Cooper
3 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2024-04-29 18:28 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.
Retain the cross-check with hardware in debug builds, but forgo it normal
builds. In particular, this means that the migration paths don't need to mess
with XCR0 just to sanity check the buffer size.
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.
---
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 | 45 +++++++++++++++++++++++++++----
4 files changed, 43 insertions(+), 8 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 9594e0a5c530..9e677d891d6c 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1191,7 +1191,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 99cedb4f5e24..a94f4025fce5 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -183,7 +183,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 +245,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;
@@ -567,16 +567,51 @@ static unsigned int hw_uncompressed_size(uint64_t xcr0)
return size;
}
-/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
-unsigned int xstate_ctxt_size(u64 xcr0)
+unsigned int xstate_uncompressed_size(uint64_t xcr0)
{
+ unsigned int size = XSTATE_AREA_MIN_SIZE, i;
+
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);
+ 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 &= ~XSTATE_FP_SSE;
+ for_each_set_bit ( i, &xcr0, 63 )
+ {
+ unsigned int s;
+
+ ASSERT(xstate_offsets[i] && xstate_sizes[i]);
+
+ s = xstate_offsets[i] && xstate_sizes[i];
+
+ size = max(size, s);
+ }
+
+ /* In debug builds, cross-check our calculation with hardware. */
+ if ( IS_ENABLED(CONFIG_DEBUG) )
+ {
+ unsigned int hwsize;
+
+ xcr0 |= XSTATE_FP_SSE;
+ hwsize = hw_uncompressed_size(xcr0);
+
+ if ( size != hwsize )
+ printk_once(XENLOG_ERR "%s(%#"PRIx64") size %#x != hwsize %#x\n",
+ __func__, xcr0, size, hwsize);
+ size = hwsize;
+ }
+
+ return size;
}
static bool valid_xcr0(uint64_t xcr0)
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate()
2024-04-29 18:28 [PATCH v2 0/4] x86/xstate: Fixes to size calculations Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states() Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
@ 2024-04-29 18:28 ` Andrew Cooper
2024-05-02 12:39 ` Jan Beulich
2024-04-29 18:28 ` [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves Andrew Cooper
3 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2024-04-29 18:28 UTC (permalink / raw
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
Make use of the new 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>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
v2:
* Tie ALIGN64 to xsavec rather than xsaves.
---
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 4b6d96276399..fc7933be8577 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] 13+ messages in thread
* [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves
2024-04-29 18:28 [PATCH v2 0/4] x86/xstate: Fixes to size calculations Andrew Cooper
` (2 preceding siblings ...)
2024-04-29 18:28 ` [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
@ 2024-04-29 18:28 ` Andrew Cooper
2024-05-02 13:04 ` Jan Beulich
3 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2024-04-29 18:28 UTC (permalink / raw
To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné
If max leaf is greater than 0xd but xsave not available to the guest, then the
current XSAVE size should not be filled in. This is a latent bug for now as
the guest max leaf is 0xd, but will become problematic in the future.
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,hw}_compressed_size() helpers to mirror the uncompressed
ones.
This in turn higlights a bug in xstate_init(). Defaulting this_cpu(xss) to ~0
requires a forced write to clear it back out. This in turn highlights that
it's only a safe default on systems with XSAVES.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
The more I think about it, the more I think that cross-checking with hardware
is a bad move. It's horribly expensive, and for supervisor states in
particular, liable to interfere with functionality.
v2:
* Cope with blind reads of CPUID.0xD[1] prior to %xcr0 having been set up.
---
xen/arch/x86/cpuid.c | 24 ++++--------
xen/arch/x86/include/asm/xstate.h | 1 +
xen/arch/x86/xstate.c | 64 ++++++++++++++++++++++++++++++-
3 files changed, 72 insertions(+), 17 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 a94f4025fce5..b2cf3ae6acee 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -614,6 +614,65 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
return size;
}
+static unsigned int hw_compressed_size(uint64_t xstates)
+{
+ uint64_t curr_xcr0 = get_xcr0(), curr_xss = get_msr_xss();
+ unsigned int size;
+ bool ok;
+
+ ok = set_xcr0(xstates & ~XSTATE_XSAVES_ONLY);
+ ASSERT(ok);
+ set_msr_xss(xstates & XSTATE_XSAVES_ONLY);
+
+ size = cpuid_count_ebx(XSTATE_CPUID, 1);
+
+ ok = set_xcr0(curr_xcr0);
+ ASSERT(ok);
+ set_msr_xss(curr_xss);
+
+ 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 are
+ * automatically rounded up to 64 first.
+ */
+ xstates &= ~XSTATE_FP_SSE;
+ for_each_set_bit ( i, &xstates, 63 )
+ {
+ if ( test_bit(i, &xstate_align) )
+ size = ROUNDUP(size, 64);
+
+ size += xstate_sizes[i];
+ }
+
+ /* In debug builds, cross-check our calculation with hardware. */
+ if ( IS_ENABLED(CONFIG_DEBUG) )
+ {
+ unsigned int hwsize;
+
+ xstates |= XSTATE_FP_SSE;
+ hwsize = hw_compressed_size(xstates);
+
+ if ( size != hwsize )
+ printk_once(XENLOG_ERR "%s(%#"PRIx64") size %#x != hwsize %#x\n",
+ __func__, xstates, size, hwsize);
+ size = hwsize;
+ }
+
+ return size;
+}
+
static bool valid_xcr0(uint64_t xcr0)
{
/* FP must be unconditionally set. */
@@ -681,7 +740,8 @@ void xstate_init(struct cpuinfo_x86 *c)
* write it.
*/
this_cpu(xcr0) = 0;
- this_cpu(xss) = ~0;
+ if ( cpu_has_xsaves )
+ this_cpu(xss) = ~0;
cpuid_count(XSTATE_CPUID, 0, &eax, &ebx, &ecx, &edx);
feature_mask = (((u64)edx << 32) | eax) & XCNTXT_MASK;
@@ -694,6 +754,8 @@ void xstate_init(struct cpuinfo_x86 *c)
set_in_cr4(X86_CR4_OSXSAVE);
if ( !set_xcr0(feature_mask) )
BUG();
+ if ( cpu_has_xsaves )
+ set_msr_xss(0);
if ( bsp )
{
--
2.30.2
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states()
2024-04-29 18:28 ` [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states() Andrew Cooper
@ 2024-05-02 12:08 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2024-05-02 12:08 UTC (permalink / raw
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 29.04.2024 20:28, Andrew Cooper wrote:
> HVM_CPU_XSAVE_SIZE() may rewrite %xcr0 twice. Defer the caluclation it until
> after we've decided to write out an XSAVE record.
>
> Note in hvm_load_cpu_xsave_states() that there were versions of Xen which
> wrote out a useless XSAVE record. This sadly limits out ability to tidy up
> the existing infrastructure. Also leave a note in xstate_ctxt_size() that 0
> still needs tolerating for now.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a nit that there are a few spelling/grammar issues in the text above.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
2024-04-29 18:28 ` [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
@ 2024-05-02 12:19 ` Jan Beulich
2024-05-02 13:17 ` Andrew Cooper
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2024-05-02 12:19 UTC (permalink / raw
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 29.04.2024 20:28, Andrew Cooper wrote:
> @@ -567,16 +567,51 @@ static unsigned int hw_uncompressed_size(uint64_t xcr0)
> return size;
> }
>
> -/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
> -unsigned int xstate_ctxt_size(u64 xcr0)
> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
> {
> + unsigned int size = XSTATE_AREA_MIN_SIZE, i;
> +
> 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);
> + if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )
This is open-coded XSTATE_FP_SSE, which I wouldn't mind if ...
> + 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 &= ~XSTATE_FP_SSE;
... you didn't use that macro here (and once further down). IOW please
be consistent, no matter which way round.
> + for_each_set_bit ( i, &xcr0, 63 )
> + {
> + unsigned int s;
> +
> + ASSERT(xstate_offsets[i] && xstate_sizes[i]);
> +
> + s = xstate_offsets[i] && xstate_sizes[i];
You mean + here, don't you? Then:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
I'm also inclined to suggest making this the initializer of s.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate()
2024-04-29 18:28 ` [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
@ 2024-05-02 12:39 ` Jan Beulich
2024-05-02 13:24 ` Andrew Cooper
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2024-05-02 12:39 UTC (permalink / raw
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 29.04.2024 20:28, Andrew Cooper wrote:
> Make use of the new xstate_uncompressed_size() helper rather than maintaining
> the running calculation while accumulating feature components.
xstate_uncompressed_size() isn't really a new function, but the re-work of
an earlier one. That, aiui, could have been used here, too, just that it
would have been inefficient to do so. IOW perhaps drop "the new"?
> 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.
As to this, ...
> @@ -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;
... for this bit, isn't the move from this ...
> + /*
> + * 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;
... to this changing what guests get to see, i.e. (mildly?) incompatible?
(For .xss there's no issue because XSTATE_XSAVES_ONLY is still 0.)
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves
2024-04-29 18:28 ` [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves Andrew Cooper
@ 2024-05-02 13:04 ` Jan Beulich
2024-05-02 14:32 ` Andrew Cooper
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2024-05-02 13:04 UTC (permalink / raw
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 29.04.2024 20:28, Andrew Cooper wrote:
> If max leaf is greater than 0xd but xsave not available to the guest, then the
> current XSAVE size should not be filled in. This is a latent bug for now as
> the guest max leaf is 0xd, but will become problematic in the future.
Why would this not be an issue when .max_leaf == 0xd, but .xsave == 0? Without
my "x86/CPUID: shrink max_{,sub}leaf fields according to actual leaf contents"
I don't think we shrink max_leaf to below 0xd when there's no xsave for the
guest?
> 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,hw}_compressed_size() helpers to mirror the uncompressed
> ones.
>
> This in turn higlights a bug in xstate_init(). Defaulting this_cpu(xss) to ~0
> requires a forced write to clear it back out. This in turn highlights that
> it's only a safe default on systems with XSAVES.
Well, yes, such an explicit write was expected to appear when some xsaves-
based component would actually be enabled. Much like the set_xcr0() there.
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> The more I think about it, the more I think that cross-checking with hardware
> is a bad move. It's horribly expensive, and for supervisor states in
> particular, liable to interfere with functionality.
I agree, but I'd also like to see the cross checking not dropped entirely.
Can't we arrange for such to happen during boot, before we enable any such
functionality? After all even in debug builds it's not overly useful to do
the same cross-checking (i.e. for identical inputs) over and over again.
Of course doing in an exhaustive manner may be okay for the uncompressed
values, but might be a little too much for all possible combinations in
order to check compressed values, too.
> --- a/xen/arch/x86/xstate.c
> +++ b/xen/arch/x86/xstate.c
> @@ -614,6 +614,65 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
> return size;
> }
>
> +static unsigned int hw_compressed_size(uint64_t xstates)
> +{
> + uint64_t curr_xcr0 = get_xcr0(), curr_xss = get_msr_xss();
> + unsigned int size;
> + bool ok;
> +
> + ok = set_xcr0(xstates & ~XSTATE_XSAVES_ONLY);
> + ASSERT(ok);
> + set_msr_xss(xstates & XSTATE_XSAVES_ONLY);
> +
> + size = cpuid_count_ebx(XSTATE_CPUID, 1);
> +
> + ok = set_xcr0(curr_xcr0);
> + ASSERT(ok);
> + set_msr_xss(curr_xss);
> +
> + 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) )
Same comment as on the earlier change regarding the (lack of) use of ....
> + return size;
> +
> + /*
> + * For the compressed size, every component matters. Some are
> + * automatically rounded up to 64 first.
> + */
> + xstates &= ~XSTATE_FP_SSE;
... this constant up there.
> + for_each_set_bit ( i, &xstates, 63 )
> + {
> + if ( test_bit(i, &xstate_align) )
> + size = ROUNDUP(size, 64);
> +
> + size += xstate_sizes[i];
> + }
The comment is a little misleading: As you have it in code, it is not the
component's size that is rounded up, but its position. Maybe "Some have
their start automatically rounded up to 64 first"?
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size()
2024-05-02 12:19 ` Jan Beulich
@ 2024-05-02 13:17 ` Andrew Cooper
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2024-05-02 13:17 UTC (permalink / raw
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 02/05/2024 1:19 pm, Jan Beulich wrote:
> On 29.04.2024 20:28, Andrew Cooper wrote:
>> @@ -567,16 +567,51 @@ static unsigned int hw_uncompressed_size(uint64_t xcr0)
>> return size;
>> }
>>
>> -/* Fastpath for common xstate size requests, avoiding reloads of xcr0. */
>> -unsigned int xstate_ctxt_size(u64 xcr0)
>> +unsigned int xstate_uncompressed_size(uint64_t xcr0)
>> {
>> + unsigned int size = XSTATE_AREA_MIN_SIZE, i;
>> +
>> 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);
>> + if ( xcr0 <= (X86_XCR0_SSE | X86_XCR0_FP) )
> This is open-coded XSTATE_FP_SSE, which I wouldn't mind if ...
>
>> + 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 &= ~XSTATE_FP_SSE;
> ... you didn't use that macro here (and once further down). IOW please
> be consistent, no matter which way round.
Oh yes - that's a consequence of these hunks having between written 3
years apart.
It's important for the first one (logical comparison against a bitmap)
that it's split apart.
>
>> + for_each_set_bit ( i, &xcr0, 63 )
>> + {
>> + unsigned int s;
>> +
>> + ASSERT(xstate_offsets[i] && xstate_sizes[i]);
>> +
>> + s = xstate_offsets[i] && xstate_sizes[i];
> You mean + here, don't you?
Yes I do... That was a victim of a last minute refactor.
It also shows that even the cross-check with hardware isn't terribly
effective. More on that in the other thread.
> Then:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
> I'm also inclined to suggest making this the initializer of s.
Hmm, good point. Will change.
Thanks,
~Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate()
2024-05-02 12:39 ` Jan Beulich
@ 2024-05-02 13:24 ` Andrew Cooper
2024-05-02 14:33 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2024-05-02 13:24 UTC (permalink / raw
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 02/05/2024 1:39 pm, Jan Beulich wrote:
> On 29.04.2024 20:28, Andrew Cooper wrote:
>> Make use of the new xstate_uncompressed_size() helper rather than maintaining
>> the running calculation while accumulating feature components.
> xstate_uncompressed_size() isn't really a new function, but the re-work of
> an earlier one. That, aiui, could have been used here, too, just that it
> would have been inefficient to do so. IOW perhaps drop "the new"?
Ok.
>
>> 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.
> As to this, ...
>
>> @@ -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;
> ... for this bit, isn't the move from this ...
>
>> + /*
>> + * 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;
> ... to this changing what guests get to see, i.e. (mildly?) incompatible?
No.
The only "rows" in leaf 0xd we expose to guests are AVX, MPX, AVX512 and
PKU (higher up in this hunk, selecting valid bits in xstates). None of
these have a non-zero value in ecx.
This is a latent bug until we offer AMX or CET, hence why I wanted to
complete this series before your AMX series goes in.
~Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves
2024-05-02 13:04 ` Jan Beulich
@ 2024-05-02 14:32 ` Andrew Cooper
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2024-05-02 14:32 UTC (permalink / raw
To: Jan Beulich; +Cc: Roger Pau Monné, Xen-devel
On 02/05/2024 2:04 pm, Jan Beulich wrote:
> On 29.04.2024 20:28, Andrew Cooper wrote:
>> If max leaf is greater than 0xd but xsave not available to the guest, then the
>> current XSAVE size should not be filled in. This is a latent bug for now as
>> the guest max leaf is 0xd, but will become problematic in the future.
> Why would this not be an issue when .max_leaf == 0xd, but .xsave == 0?
Hmm, true. I'll adjust the description.
>
>> 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,hw}_compressed_size() helpers to mirror the uncompressed
>> ones.
>>
>> This in turn higlights a bug in xstate_init(). Defaulting this_cpu(xss) to ~0
>> requires a forced write to clear it back out. This in turn highlights that
>> it's only a safe default on systems with XSAVES.
> Well, yes, such an explicit write was expected to appear when some xsaves-
> based component would actually be enabled. Much like the set_xcr0() there.
I stumbled over this because the debug logic ended up trying to restore
0xffffffff (the thing it read out as the "last" value) back into XSS.
This works about as well as you'd expect.
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>
>> The more I think about it, the more I think that cross-checking with hardware
>> is a bad move. It's horribly expensive, and for supervisor states in
>> particular, liable to interfere with functionality.
> I agree, but I'd also like to see the cross checking not dropped entirely.
> Can't we arrange for such to happen during boot, before we enable any such
> functionality? After all even in debug builds it's not overly useful to do
> the same cross-checking (i.e. for identical inputs) over and over again.
> Of course doing in an exhaustive manner may be okay for the uncompressed
> values, but might be a little too much for all possible combinations in
> order to check compressed values, too.
Given the observation of patch 2 being buggy and my final sanity test
before posting didn't notice, I think doing this all at boot would be a
much better idea.
I think I'm going to do a new patch early in the series as an adjustment
to xstate_init().
We can't feasibly test every combination, but what ought to do is
linearly accumulate the xstates Xen knows about and checking that in
each case the size(s) increase as appropriate.
This will have a substantial impact on the remainder of the series, but
I think the end result will be all the better for it.
>> --- a/xen/arch/x86/xstate.c
>> +++ b/xen/arch/x86/xstate.c
>> @@ -614,6 +614,65 @@ unsigned int xstate_uncompressed_size(uint64_t xcr0)
>> return size;
>> }
>>
>> +static unsigned int hw_compressed_size(uint64_t xstates)
>> +{
>> + uint64_t curr_xcr0 = get_xcr0(), curr_xss = get_msr_xss();
>> + unsigned int size;
>> + bool ok;
>> +
>> + ok = set_xcr0(xstates & ~XSTATE_XSAVES_ONLY);
>> + ASSERT(ok);
>> + set_msr_xss(xstates & XSTATE_XSAVES_ONLY);
>> +
>> + size = cpuid_count_ebx(XSTATE_CPUID, 1);
>> +
>> + ok = set_xcr0(curr_xcr0);
>> + ASSERT(ok);
>> + set_msr_xss(curr_xss);
>> +
>> + 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) )
> Same comment as on the earlier change regarding the (lack of) use of ....
>
>> + return size;
>> +
>> + /*
>> + * For the compressed size, every component matters. Some are
>> + * automatically rounded up to 64 first.
>> + */
>> + xstates &= ~XSTATE_FP_SSE;
> ... this constant up there.
>
>> + for_each_set_bit ( i, &xstates, 63 )
>> + {
>> + if ( test_bit(i, &xstate_align) )
>> + size = ROUNDUP(size, 64);
>> +
>> + size += xstate_sizes[i];
>> + }
> The comment is a little misleading: As you have it in code, it is not the
> component's size that is rounded up, but its position. Maybe "Some have
> their start automatically rounded up to 64 first"?
Size in that sentence referees to the compressed size of everything, not
the size of the component.
But I'll try to make it clearer.
~Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate()
2024-05-02 13:24 ` Andrew Cooper
@ 2024-05-02 14:33 ` Jan Beulich
0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2024-05-02 14:33 UTC (permalink / raw
To: Andrew Cooper; +Cc: Roger Pau Monné, Xen-devel
On 02.05.2024 15:24, Andrew Cooper wrote:
> On 02/05/2024 1:39 pm, Jan Beulich wrote:
>> On 29.04.2024 20:28, Andrew Cooper wrote:
>>> Make use of the new xstate_uncompressed_size() helper rather than maintaining
>>> the running calculation while accumulating feature components.
>> xstate_uncompressed_size() isn't really a new function, but the re-work of
>> an earlier one. That, aiui, could have been used here, too, just that it
>> would have been inefficient to do so. IOW perhaps drop "the new"?
>
> Ok.
>
>>
>>> 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.
>> As to this, ...
>>
>>> @@ -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;
>> ... for this bit, isn't the move from this ...
>>
>>> + /*
>>> + * 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;
>> ... to this changing what guests get to see, i.e. (mildly?) incompatible?
>
> No.
>
> The only "rows" in leaf 0xd we expose to guests are AVX, MPX, AVX512 and
> PKU (higher up in this hunk, selecting valid bits in xstates). None of
> these have a non-zero value in ecx.
>
> This is a latent bug until we offer AMX or CET, hence why I wanted to
> complete this series before your AMX series goes in.
Oh, so finally a description of that very issue you kept mentioning. With
the small tweak to the description then
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-05-02 14:34 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-29 18:28 [PATCH v2 0/4] x86/xstate: Fixes to size calculations Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 1/4] x86/hvm: Defer the size calculation in hvm_save_cpu_xsave_states() Andrew Cooper
2024-05-02 12:08 ` Jan Beulich
2024-04-29 18:28 ` [PATCH v2 2/4] x86/xstate: Rework xstate_ctxt_size() as xstate_uncompressed_size() Andrew Cooper
2024-05-02 12:19 ` Jan Beulich
2024-05-02 13:17 ` Andrew Cooper
2024-04-29 18:28 ` [PATCH v2 3/4] x86/cpu-policy: Simplify recalculate_xstate() Andrew Cooper
2024-05-02 12:39 ` Jan Beulich
2024-05-02 13:24 ` Andrew Cooper
2024-05-02 14:33 ` Jan Beulich
2024-04-29 18:28 ` [PATCH v2 4/4] x86/cpuid: Fix handling of xsave dynamic leaves Andrew Cooper
2024-05-02 13:04 ` Jan Beulich
2024-05-02 14:32 ` Andrew Cooper
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.