We're soon going to need a compressed helper of the same form.
The size of the uncompressed image is a strictly a property of the highest
user state. This can be calculated trivially with xstate_offsets/sizes, and
is much faster than a CPUID instruction in the first place, let alone the two
XCR0 writes surrounding it.
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.
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>
---
xen/arch/x86/domctl.c | 2 +-
xen/arch/x86/hvm/hvm.c | 2 +-
xen/arch/x86/xstate.c | 40 +++++++++++++++++++++++++++++++---------
xen/include/asm-x86/xstate.h | 2 +-
4 files changed, 34 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index e440bd021e..8c3552410d 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -899,7 +899,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 28beacc45b..e5fda6b387 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1203,7 +1203,7 @@ HVM_REGISTER_SAVE_RESTORE(CPU, hvm_save_cpu_ctxt, 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 hvm_save_cpu_xsave_states(struct vcpu *v, hvm_domain_context_t *h)
{
diff --git a/xen/arch/x86/xstate.c b/xen/arch/x86/xstate.c
index e6c225a16b..d4c01da574 100644
--- a/xen/arch/x86/xstate.c
+++ b/xen/arch/x86/xstate.c
@@ -184,7 +184,7 @@ void expand_xsave_states(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 ( !(xsave->xsave_hdr.xcomp_bv & XSTATE_COMPACTION_ENABLED) )
{
@@ -246,7 +246,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;
@@ -568,16 +568,38 @@ 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)
{
- if ( xcr0 == xfeature_mask )
- return xsave_cntxt_size;
+ unsigned int size;
+ int idx = flsl(xcr0) - 1;
- if ( xcr0 == 0 )
- return 0;
+ /*
+ * The maximum size of an uncompressed XSAVE area is determined by the
+ * highest user state, as the size and offset of each component is fixed.
+ */
+ if ( idx >= 2 )
+ {
+ ASSERT(xstate_offsets[idx] && xstate_sizes[idx]);
+ size = xstate_offsets[idx] + xstate_sizes[idx];
+ }
+ else
+ size = XSTATE_AREA_MIN_SIZE;
- return hw_uncompressed_size(xcr0);
+ /* 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;
}
/* Collect the information of processor's extended state */
diff --git a/xen/include/asm-x86/xstate.h b/xen/include/asm-x86/xstate.h
index 7ab0bdde89..02d6f171b8 100644
--- a/xen/include/asm-x86/xstate.h
+++ b/xen/include/asm-x86/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)
{
--
2.11.0
On 03.05.2021 17:39, Andrew Cooper wrote: > @@ -568,16 +568,38 @@ 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) Since you rewrite the function anyway, and since taking into account the XSS-controlled features here is going to be necessary as well (even if just down the road, but that's what your ultimate goal is from all I can tell), how about renaming the parameter to "xstates" or "states" at the same time? Jan
On 04/05/2021 13:20, Jan Beulich wrote: > On 03.05.2021 17:39, Andrew Cooper wrote: >> @@ -568,16 +568,38 @@ 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) > Since you rewrite the function anyway, and since taking into account > the XSS-controlled features here is going to be necessary as well > (even if just down the road, but that's what your ultimate goal is > from all I can tell), how about renaming the parameter to "xstates" > or "states" at the same time? I'm working on some cleanup of terminology, which I haven't posted yet. For this one, I'm not sure. For uncompressed size, we genuinely mean user states only. When there's a suitable constant to use, this will gain an assertion. ~Andrew
On 04.05.2021 14:22, Andrew Cooper wrote: > On 04/05/2021 13:20, Jan Beulich wrote: >> On 03.05.2021 17:39, Andrew Cooper wrote: >>> @@ -568,16 +568,38 @@ 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) >> Since you rewrite the function anyway, and since taking into account >> the XSS-controlled features here is going to be necessary as well >> (even if just down the road, but that's what your ultimate goal is >> from all I can tell), how about renaming the parameter to "xstates" >> or "states" at the same time? > > I'm working on some cleanup of terminology, which I haven't posted yet. > > For this one, I'm not sure. For uncompressed size, we genuinely mean > user states only. Ah, yes - fair point. Jan
On 03/05/2021 16:39, Andrew Cooper wrote: > We're soon going to need a compressed helper of the same form. > > The size of the uncompressed image is a strictly a property of the highest > user state. This can be calculated trivially with xstate_offsets/sizes, and > is much faster than a CPUID instruction in the first place, let alone the two > XCR0 writes surrounding it. > > 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. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> The Qemu smoketests have actually found a bug here. https://gitlab.com/xen-project/patchew/xen/-/jobs/1232118510/artifacts/file/smoke.serial We call into xstate_uncompressed_size() from hvm_register_CPU_save_and_restore() so the previous "xcr0 == 0" path was critical to Xen not exploding on non-xsave platforms. This is straight up buggy - we shouldn't be registering xsave handlers on non-xsave platforms, but the calculation is also wrong (in the safe directly at least) when we use compressed formats. Yet another unexpected surprise for the todo list. ~Andrew
On 03.05.2021 20:17, Andrew Cooper wrote: > On 03/05/2021 16:39, Andrew Cooper wrote: >> We're soon going to need a compressed helper of the same form. >> >> The size of the uncompressed image is a strictly a property of the highest >> user state. This can be calculated trivially with xstate_offsets/sizes, and >> is much faster than a CPUID instruction in the first place, let alone the two >> XCR0 writes surrounding it. >> >> 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. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > The Qemu smoketests have actually found a bug here. > > https://gitlab.com/xen-project/patchew/xen/-/jobs/1232118510/artifacts/file/smoke.serial > > We call into xstate_uncompressed_size() from > hvm_register_CPU_save_and_restore() so the previous "xcr0 == 0" path was > critical to Xen not exploding on non-xsave platforms. > > This is straight up buggy - we shouldn't be registering xsave handlers > on non-xsave platforms, but the calculation is also wrong (in the safe > directly at least) when we use compressed formats. Yet another > unexpected surprise for the todo list. I don't view this as buggy at all - it was an implementation choice. Perhaps not the best one, but still correct afaict. Then again I'm afraid I don't understand "in the safe directly at least", so I may well be overlooking something. Will wait for your updated patch ... Jan
On 04/05/2021 13:08, Jan Beulich wrote: > On 03.05.2021 20:17, Andrew Cooper wrote: >> On 03/05/2021 16:39, Andrew Cooper wrote: >>> We're soon going to need a compressed helper of the same form. >>> >>> The size of the uncompressed image is a strictly a property of the highest >>> user state. This can be calculated trivially with xstate_offsets/sizes, and >>> is much faster than a CPUID instruction in the first place, let alone the two >>> XCR0 writes surrounding it. >>> >>> 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. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> The Qemu smoketests have actually found a bug here. >> >> https://gitlab.com/xen-project/patchew/xen/-/jobs/1232118510/artifacts/file/smoke.serial >> >> We call into xstate_uncompressed_size() from >> hvm_register_CPU_save_and_restore() so the previous "xcr0 == 0" path was >> critical to Xen not exploding on non-xsave platforms. >> >> This is straight up buggy - we shouldn't be registering xsave handlers >> on non-xsave platforms, but the calculation is also wrong (in the safe >> directly at least) when we use compressed formats. Yet another >> unexpected surprise for the todo list. > I don't view this as buggy at all - it was an implementation choice. > Perhaps not the best one, but still correct afaict. Then again I'm > afraid I don't understand "in the safe directly at least", so I may > well be overlooking something. Will wait for your updated patch ... For now, it is a patch 2.5/5 which just puts a cpu_has_xsave guard around the registration. Everything to do with xsave record processing is unnecessary overhead on a non-xsave platform. I don't intend to alter patch 3 as a consequence. ~Andrew
© 2016 - 2024 Red Hat, Inc.