* [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA
@ 2022-12-27 18:37 Aaron Lewis
2022-12-27 18:37 ` [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear Aaron Lewis
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Aaron Lewis @ 2022-12-27 18:37 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, like.xu.linux, Aaron Lewis
When running a SPR guest without AMX enabled
CPUID.(EAX=0DH,ECX=0):EAX.XTILE_CFG[bit-17] will be set and
CPUID.(EAX=0DH,ECX=0):EAX.XTILE_DATA[bit-18] will be clear. While this
is architecturally correct it can be a little awkward for userspace
or a guest when using them. Instead of leaving the CPUID leaf in a
half baked state, either clear them both or leave them both set.
Additionally, add testing to verify the CPUID isn't in such a state.
Aaron Lewis (3):
KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
KVM: selftests: Add XCR0 Test
arch/x86/kvm/cpuid.c | 4 +
tools/testing/selftests/kvm/Makefile | 1 +
.../selftests/kvm/include/x86_64/processor.h | 19 +++
tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +---
.../selftests/kvm/x86_64/xcr0_cpuid_test.c | 134 ++++++++++++++++++
5 files changed, 161 insertions(+), 21 deletions(-)
create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
--
2.39.0.314.g84b9a713c41-goog
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
2022-12-27 18:37 [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Aaron Lewis
@ 2022-12-27 18:37 ` Aaron Lewis
2022-12-27 19:56 ` Jim Mattson
2022-12-27 18:37 ` [PATCH 2/3] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible Aaron Lewis
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Aaron Lewis @ 2022-12-27 18:37 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, like.xu.linux, Aaron Lewis
Be good citizens by clearing both
CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[bit-17] and
CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[bit-18] if they are both not set.
That way userspace or a guest doesn't fail if it attempts to set XCR0
with the user xfeature bits, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0).
Suggested-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
arch/x86/kvm/cpuid.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0b5bf013fcb8e..2d9910847786a 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
u64 permitted_xss = kvm_caps.supported_xss;
+ if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) ||
+ !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA))
+ permitted_xcr0 &= ~XFEATURE_MASK_XTILE;
+
entry->eax &= permitted_xcr0;
entry->ebx = xstate_required_size(permitted_xcr0, false);
entry->ecx = entry->ebx;
--
2.39.0.314.g84b9a713c41-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
2022-12-27 18:37 [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Aaron Lewis
2022-12-27 18:37 ` [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear Aaron Lewis
@ 2022-12-27 18:37 ` Aaron Lewis
2022-12-27 21:23 ` Jim Mattson
2022-12-27 18:37 ` [PATCH 3/3] KVM: selftests: Add XCR0 Test Aaron Lewis
2022-12-27 20:00 ` [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Jim Mattson
3 siblings, 1 reply; 13+ messages in thread
From: Aaron Lewis @ 2022-12-27 18:37 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, like.xu.linux, Aaron Lewis
The instructions XGETBV and XSETBV are useful to other tests. Move
them to processor.h to make them available to be used more broadly.
No functional change intended.
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
.../selftests/kvm/include/x86_64/processor.h | 19 +++++++++++++++
tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +++----------------
2 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
index b1a31de7108ac..34957137be375 100644
--- a/tools/testing/selftests/kvm/include/x86_64/processor.h
+++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
@@ -492,6 +492,25 @@ static inline void set_cr4(uint64_t val)
__asm__ __volatile__("mov %0, %%cr4" : : "r" (val) : "memory");
}
+static inline u64 xgetbv(u32 index)
+{
+ u32 eax, edx;
+
+ __asm__ __volatile__("xgetbv;"
+ : "=a" (eax), "=d" (edx)
+ : "c" (index));
+ return eax | ((u64)edx << 32);
+}
+
+static inline void xsetbv(u32 index, u64 value)
+{
+ u32 eax = value;
+ u32 edx = value >> 32;
+
+ __asm__ __volatile__("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
+}
+
+
static inline struct desc_ptr get_gdt(void)
{
struct desc_ptr gdt;
diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index bd72c6eb3b670..4b733ad218313 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -68,24 +68,6 @@ struct xtile_info {
static struct xtile_info xtile;
-static inline u64 __xgetbv(u32 index)
-{
- u32 eax, edx;
-
- asm volatile("xgetbv;"
- : "=a" (eax), "=d" (edx)
- : "c" (index));
- return eax + ((u64)edx << 32);
-}
-
-static inline void __xsetbv(u32 index, u64 value)
-{
- u32 eax = value;
- u32 edx = value >> 32;
-
- asm volatile("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
-}
-
static inline void __ldtilecfg(void *cfg)
{
asm volatile(".byte 0xc4,0xe2,0x78,0x49,0x00"
@@ -121,7 +103,7 @@ static inline void check_cpuid_xsave(void)
static bool check_xsave_supports_xtile(void)
{
- return __xgetbv(0) & XFEATURE_MASK_XTILE;
+ return xgetbv(0) & XFEATURE_MASK_XTILE;
}
static void check_xtile_info(void)
@@ -177,9 +159,9 @@ static void init_regs(void)
cr4 |= X86_CR4_OSXSAVE;
set_cr4(cr4);
- xcr0 = __xgetbv(0);
+ xcr0 = xgetbv(0);
xcr0 |= XFEATURE_MASK_XTILE;
- __xsetbv(0x0, xcr0);
+ xsetbv(0x0, xcr0);
}
static void __attribute__((__flatten__)) guest_code(struct tile_config *amx_cfg,
--
2.39.0.314.g84b9a713c41-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] KVM: selftests: Add XCR0 Test
2022-12-27 18:37 [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Aaron Lewis
2022-12-27 18:37 ` [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear Aaron Lewis
2022-12-27 18:37 ` [PATCH 2/3] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible Aaron Lewis
@ 2022-12-27 18:37 ` Aaron Lewis
2022-12-27 22:07 ` Jim Mattson
2022-12-27 20:00 ` [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Jim Mattson
3 siblings, 1 reply; 13+ messages in thread
From: Aaron Lewis @ 2022-12-27 18:37 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, like.xu.linux, Aaron Lewis
Test that the user xfeature bits, EDX:EAX of CPUID.(EAX=0DH,ECX=0),
don't set up userspace for failure.
Though it isn't architectural, test that the user xfeature bits aren't
set in a half baked state that will cause a #GP if used when setting
XCR0.
Check that the rules for XCR0 described in the SDM vol 1, section
13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED FEATURES, are
followed for the xfeature bits too.
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
tools/testing/selftests/kvm/Makefile | 1 +
.../selftests/kvm/x86_64/xcr0_cpuid_test.c | 134 ++++++++++++++++++
2 files changed, 135 insertions(+)
create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 1750f91dd9362..e2e56c82b8a90 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -104,6 +104,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test
TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test
+TEST_GEN_PROGS_x86_64 += x86_64/xcr0_cpuid_test
TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
diff --git a/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
new file mode 100644
index 0000000000000..644791ff5c48b
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
@@ -0,0 +1,134 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * XCR0 cpuid test
+ *
+ * Copyright (C) 2022, Google LLC.
+ */
+
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/ioctl.h>
+
+#include "test_util.h"
+
+#include "kvm_util.h"
+#include "processor.h"
+
+#define XFEATURE_MASK_SSE (1ul << 1)
+#define XFEATURE_MASK_YMM (1ul << 2)
+#define XFEATURE_MASK_BNDREGS (1ul << 3)
+#define XFEATURE_MASK_BNDCSR (1ul << 4)
+#define XFEATURE_MASK_OPMASK (1ul << 5)
+#define XFEATURE_MASK_ZMM_Hi256 (1ul << 6)
+#define XFEATURE_MASK_Hi16_ZMM (1ul << 7)
+#define XFEATURE_MASK_XTILECFG (1ul << 17)
+#define XFEATURE_MASK_XTILEDATA (1ul << 18)
+#define XFEATURE_MASK_XTILE (XFEATURE_MASK_XTILECFG | \
+ XFEATURE_MASK_XTILEDATA)
+
+static uint64_t get_supported_user_xfeatures(void)
+{
+ uint32_t a, b, c, d;
+
+ cpuid(0xd, &a, &b, &c, &d);
+
+ return a | ((uint64_t)d << 32);
+}
+
+static void guest_code(void)
+{
+ uint64_t xcr0_rest;
+ uint64_t supported_xcr0;
+ uint64_t xfeature_mask;
+ uint64_t supported_state;
+
+ set_cr4(get_cr4() | X86_CR4_OSXSAVE);
+
+ xcr0_rest = xgetbv(0);
+ supported_xcr0 = get_supported_user_xfeatures();
+
+ GUEST_ASSERT(xcr0_rest == 1ul);
+
+ /* Check AVX */
+ xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
+ supported_state = supported_xcr0 & xfeature_mask;
+ GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM);
+
+ /* Check MPX */
+ xfeature_mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
+ supported_state = supported_xcr0 & xfeature_mask;
+ GUEST_ASSERT((supported_state == xfeature_mask) ||
+ (supported_state == 0ul));
+
+ /* Check AVX-512 */
+ xfeature_mask = XFEATURE_MASK_OPMASK |
+ XFEATURE_MASK_ZMM_Hi256 |
+ XFEATURE_MASK_Hi16_ZMM;
+ supported_state = supported_xcr0 & xfeature_mask;
+ GUEST_ASSERT((supported_state == xfeature_mask) ||
+ (supported_state == 0ul));
+
+ /* Check AMX */
+ xfeature_mask = XFEATURE_MASK_XTILE;
+ supported_state = supported_xcr0 & xfeature_mask;
+ GUEST_ASSERT((supported_state == xfeature_mask) ||
+ (supported_state == 0ul));
+
+ GUEST_SYNC(0);
+
+ xsetbv(0, supported_xcr0);
+
+ GUEST_DONE();
+}
+
+static void guest_gp_handler(struct ex_regs *regs)
+{
+ GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0.");
+}
+
+int main(int argc, char *argv[])
+{
+ struct kvm_vcpu *vcpu;
+ struct kvm_run *run;
+ struct kvm_vm *vm;
+ struct ucall uc;
+
+ TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
+
+ /* Tell stdout not to buffer its content */
+ setbuf(stdout, NULL);
+
+ vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+ run = vcpu->run;
+
+ vm_init_descriptor_tables(vm);
+ vcpu_init_descriptor_tables(vcpu);
+
+ while (1) {
+ vcpu_run(vcpu);
+
+ TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
+ "Unexpected exit reason: %u (%s),\n",
+ run->exit_reason,
+ exit_reason_str(run->exit_reason));
+
+ switch (get_ucall(vcpu, &uc)) {
+ case UCALL_SYNC:
+ vm_install_exception_handler(vm, GP_VECTOR,
+ guest_gp_handler);
+ break;
+ case UCALL_ABORT:
+ REPORT_GUEST_ASSERT(uc);
+ break;
+ case UCALL_DONE:
+ goto done;
+ default:
+ TEST_FAIL("Unknown ucall %lu", uc.cmd);
+ }
+ }
+
+done:
+ kvm_vm_free(vm);
+ return 0;
+}
--
2.39.0.314.g84b9a713c41-goog
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
2022-12-27 18:37 ` [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear Aaron Lewis
@ 2022-12-27 19:56 ` Jim Mattson
2022-12-27 20:43 ` Aaron Lewis
0 siblings, 1 reply; 13+ messages in thread
From: Jim Mattson @ 2022-12-27 19:56 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote:
>
> Be good citizens by clearing both
> CPUID.(EAX=0DH,ECX=0):EAX.XTILECFG[bit-17] and
> CPUID.(EAX=0DH,ECX=0):EAX.XTILEDATA[bit-18] if they are both not set.
> That way userspace or a guest doesn't fail if it attempts to set XCR0
> with the user xfeature bits, i.e. EDX:EAX of CPUID.(EAX=0DH,ECX=0).
>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
> arch/x86/kvm/cpuid.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0b5bf013fcb8e..2d9910847786a 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> u64 permitted_xss = kvm_caps.supported_xss;
>
> + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) ||
> + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA))
> + permitted_xcr0 &= ~XFEATURE_MASK_XTILE;
> +
> entry->eax &= permitted_xcr0;
> entry->ebx = xstate_required_size(permitted_xcr0, false);
> entry->ecx = entry->ebx;
> --
> 2.39.0.314.g84b9a713c41-goog
>
Two questions:
1) Under what circumstances would this happen?
2) Shouldn't we also clear XFEATURE_MASK_CFG if both bits are not set?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA
2022-12-27 18:37 [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Aaron Lewis
` (2 preceding siblings ...)
2022-12-27 18:37 ` [PATCH 3/3] KVM: selftests: Add XCR0 Test Aaron Lewis
@ 2022-12-27 20:00 ` Jim Mattson
2022-12-27 20:40 ` Aaron Lewis
3 siblings, 1 reply; 13+ messages in thread
From: Jim Mattson @ 2022-12-27 20:00 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote:
>
> When running a SPR guest without AMX enabled
Can you clarify what "without AMX enabled" means? Do you mean that
userspace hasn't opted in to AMX via arch_prctl()?
> CPUID.(EAX=0DH,ECX=0):EAX.XTILE_CFG[bit-17] will be set and
> CPUID.(EAX=0DH,ECX=0):EAX.XTILE_DATA[bit-18] will be clear. While this
> is architecturally correct it can be a little awkward for userspace
> or a guest when using them. Instead of leaving the CPUID leaf in a
> half baked state, either clear them both or leave them both set.
>
> Additionally, add testing to verify the CPUID isn't in such a state.
>
> Aaron Lewis (3):
> KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
> KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
> KVM: selftests: Add XCR0 Test
>
> arch/x86/kvm/cpuid.c | 4 +
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/include/x86_64/processor.h | 19 +++
> tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +---
> .../selftests/kvm/x86_64/xcr0_cpuid_test.c | 134 ++++++++++++++++++
> 5 files changed, 161 insertions(+), 21 deletions(-)
> create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
>
> --
> 2.39.0.314.g84b9a713c41-goog
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA
2022-12-27 20:00 ` [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Jim Mattson
@ 2022-12-27 20:40 ` Aaron Lewis
0 siblings, 0 replies; 13+ messages in thread
From: Aaron Lewis @ 2022-12-27 20:40 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 8:01 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote:
> >
> > When running a SPR guest without AMX enabled
>
> Can you clarify what "without AMX enabled" means? Do you mean that
> userspace hasn't opted in to AMX via arch_prctl()?
>
Yes, exactly. I'll clarify in the follow-up.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
2022-12-27 19:56 ` Jim Mattson
@ 2022-12-27 20:43 ` Aaron Lewis
2022-12-27 21:14 ` Jim Mattson
0 siblings, 1 reply; 13+ messages in thread
From: Aaron Lewis @ 2022-12-27 20:43 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm, pbonzini, seanjc, like.xu.linux
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index 0b5bf013fcb8e..2d9910847786a 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> > u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > u64 permitted_xss = kvm_caps.supported_xss;
> >
> > + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) ||
> > + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA))
> > + permitted_xcr0 &= ~XFEATURE_MASK_XTILE;
> > +
> > entry->eax &= permitted_xcr0;
> > entry->ebx = xstate_required_size(permitted_xcr0, false);
> > entry->ecx = entry->ebx;
> > --
> > 2.39.0.314.g84b9a713c41-goog
> >
>
> Two questions:
>
> 1) Under what circumstances would this happen?
This would happen if userspace hasn't opted in to using AMX via arch_prctl().
> 2) Shouldn't we also clear XFEATURE_MASK_CFG if both bits are not set?
Both CFG and DATA are cleared with XFEATURE_MASK_XTILE. It defines both.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
2022-12-27 20:43 ` Aaron Lewis
@ 2022-12-27 21:14 ` Jim Mattson
2022-12-27 21:36 ` Jim Mattson
0 siblings, 1 reply; 13+ messages in thread
From: Jim Mattson @ 2022-12-27 21:14 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 12:44 PM Aaron Lewis <aaronlewis@google.com> wrote:
>
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index 0b5bf013fcb8e..2d9910847786a 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -977,6 +977,10 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
> > > u64 permitted_xcr0 = kvm_caps.supported_xcr0 & xstate_get_guest_group_perm();
> > > u64 permitted_xss = kvm_caps.supported_xss;
> > >
> > > + if (!(permitted_xcr0 & XFEATURE_MASK_XTILE_CFG) ||
> > > + !(permitted_xcr0 & XFEATURE_MASK_XTILE_DATA))
> > > + permitted_xcr0 &= ~XFEATURE_MASK_XTILE;
> > > +
> > > entry->eax &= permitted_xcr0;
> > > entry->ebx = xstate_required_size(permitted_xcr0, false);
> > > entry->ecx = entry->ebx;
> > > --
> > > 2.39.0.314.g84b9a713c41-goog
> > >
> >
> > Two questions:
> >
> > 1) Under what circumstances would this happen?
> This would happen if userspace hasn't opted in to using AMX via arch_prctl().
>
> > 2) Shouldn't we also clear XFEATURE_MASK_CFG if both bits are not set?
> Both CFG and DATA are cleared with XFEATURE_MASK_XTILE. It defines both.
Doh!
Reviewed-by: Jim Mattson <jmattson@google.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible
2022-12-27 18:37 ` [PATCH 2/3] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible Aaron Lewis
@ 2022-12-27 21:23 ` Jim Mattson
0 siblings, 0 replies; 13+ messages in thread
From: Jim Mattson @ 2022-12-27 21:23 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote:
>
> The instructions XGETBV and XSETBV are useful to other tests. Move
> them to processor.h to make them available to be used more broadly.
>
> No functional change intended.
>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
> .../selftests/kvm/include/x86_64/processor.h | 19 +++++++++++++++
> tools/testing/selftests/kvm/x86_64/amx_test.c | 24 +++----------------
> 2 files changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/include/x86_64/processor.h b/tools/testing/selftests/kvm/include/x86_64/processor.h
> index b1a31de7108ac..34957137be375 100644
> --- a/tools/testing/selftests/kvm/include/x86_64/processor.h
> +++ b/tools/testing/selftests/kvm/include/x86_64/processor.h
> @@ -492,6 +492,25 @@ static inline void set_cr4(uint64_t val)
> __asm__ __volatile__("mov %0, %%cr4" : : "r" (val) : "memory");
> }
>
> +static inline u64 xgetbv(u32 index)
> +{
> + u32 eax, edx;
> +
> + __asm__ __volatile__("xgetbv;"
> + : "=a" (eax), "=d" (edx)
> + : "c" (index));
> + return eax | ((u64)edx << 32);
> +}
> +
> +static inline void xsetbv(u32 index, u64 value)
> +{
> + u32 eax = value;
> + u32 edx = value >> 32;
> +
> + __asm__ __volatile__("xsetbv" :: "a" (eax), "d" (edx), "c" (index));
> +}
> +
> +
Not your change, but shouldn't both of these asm statements have
artificial "memory" clobbers, to prevent reordering?
Reviewed-by: Jim Mattson <jmattson@google.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear
2022-12-27 21:14 ` Jim Mattson
@ 2022-12-27 21:36 ` Jim Mattson
0 siblings, 0 replies; 13+ messages in thread
From: Jim Mattson @ 2022-12-27 21:36 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 1:14 PM Jim Mattson <jmattson@google.com> wrote:
> Reviewed-by: Jim Mattson <jmattson@google.com>
Could you change the shortlog to clarify that both feature bits are
cleared if either one is clear?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Add XCR0 Test
2022-12-27 18:37 ` [PATCH 3/3] KVM: selftests: Add XCR0 Test Aaron Lewis
@ 2022-12-27 22:07 ` Jim Mattson
2022-12-28 0:15 ` Aaron Lewis
0 siblings, 1 reply; 13+ messages in thread
From: Jim Mattson @ 2022-12-27 22:07 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, seanjc, like.xu.linux
On Tue, Dec 27, 2022 at 10:38 AM Aaron Lewis <aaronlewis@google.com> wrote:
>
> Test that the user xfeature bits, EDX:EAX of CPUID.(EAX=0DH,ECX=0),
> don't set up userspace for failure.
>
> Though it isn't architectural, test that the user xfeature bits aren't
> set in a half baked state that will cause a #GP if used when setting
> XCR0.
>
> Check that the rules for XCR0 described in the SDM vol 1, section
> 13.3 ENABLING THE XSAVE FEATURE SET AND XSAVE-ENABLED FEATURES, are
> followed for the xfeature bits too.
>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
> tools/testing/selftests/kvm/Makefile | 1 +
> .../selftests/kvm/x86_64/xcr0_cpuid_test.c | 134 ++++++++++++++++++
> 2 files changed, 135 insertions(+)
> create mode 100644 tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
>
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 1750f91dd9362..e2e56c82b8a90 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -104,6 +104,7 @@ TEST_GEN_PROGS_x86_64 += x86_64/vmx_tsc_adjust_test
> TEST_GEN_PROGS_x86_64 += x86_64/vmx_nested_tsc_scaling_test
> TEST_GEN_PROGS_x86_64 += x86_64/xapic_ipi_test
> TEST_GEN_PROGS_x86_64 += x86_64/xapic_state_test
> +TEST_GEN_PROGS_x86_64 += x86_64/xcr0_cpuid_test
> TEST_GEN_PROGS_x86_64 += x86_64/xss_msr_test
> TEST_GEN_PROGS_x86_64 += x86_64/debug_regs
> TEST_GEN_PROGS_x86_64 += x86_64/tsc_msrs_test
> diff --git a/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
> new file mode 100644
> index 0000000000000..644791ff5c48b
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/xcr0_cpuid_test.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * XCR0 cpuid test
> + *
> + * Copyright (C) 2022, Google LLC.
> + */
> +
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +
> +#include "test_util.h"
> +
> +#include "kvm_util.h"
> +#include "processor.h"
> +
> +#define XFEATURE_MASK_SSE (1ul << 1)
> +#define XFEATURE_MASK_YMM (1ul << 2)
> +#define XFEATURE_MASK_BNDREGS (1ul << 3)
> +#define XFEATURE_MASK_BNDCSR (1ul << 4)
> +#define XFEATURE_MASK_OPMASK (1ul << 5)
> +#define XFEATURE_MASK_ZMM_Hi256 (1ul << 6)
> +#define XFEATURE_MASK_Hi16_ZMM (1ul << 7)
> +#define XFEATURE_MASK_XTILECFG (1ul << 17)
> +#define XFEATURE_MASK_XTILEDATA (1ul << 18)
> +#define XFEATURE_MASK_XTILE (XFEATURE_MASK_XTILECFG | \
> + XFEATURE_MASK_XTILEDATA)
With XSETBV hoisted into processor.h, shouldn't these macros be more
widely available as well?
> +static uint64_t get_supported_user_xfeatures(void)
> +{
> + uint32_t a, b, c, d;
> +
> + cpuid(0xd, &a, &b, &c, &d);
> +
> + return a | ((uint64_t)d << 32);
> +}
> +
> +static void guest_code(void)
> +{
> + uint64_t xcr0_rest;
> + uint64_t supported_xcr0;
> + uint64_t xfeature_mask;
> + uint64_t supported_state;
> +
> + set_cr4(get_cr4() | X86_CR4_OSXSAVE);
> +
> + xcr0_rest = xgetbv(0);
> + supported_xcr0 = get_supported_user_xfeatures();
> +
> + GUEST_ASSERT(xcr0_rest == 1ul);
> +
> + /* Check AVX */
> + xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
> + supported_state = supported_xcr0 & xfeature_mask;
> + GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM);
> +
> + /* Check MPX */
> + xfeature_mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> + supported_state = supported_xcr0 & xfeature_mask;
> + GUEST_ASSERT((supported_state == xfeature_mask) ||
> + (supported_state == 0ul));
> +
> + /* Check AVX-512 */
> + xfeature_mask = XFEATURE_MASK_OPMASK |
> + XFEATURE_MASK_ZMM_Hi256 |
> + XFEATURE_MASK_Hi16_ZMM;
> + supported_state = supported_xcr0 & xfeature_mask;
> + GUEST_ASSERT((supported_state == xfeature_mask) ||
> + (supported_state == 0ul));
> +
> + /* Check AMX */
> + xfeature_mask = XFEATURE_MASK_XTILE;
> + supported_state = supported_xcr0 & xfeature_mask;
> + GUEST_ASSERT((supported_state == xfeature_mask) ||
> + (supported_state == 0ul));
In this series, you've added code to __do_cpuid_func() to ensure that
XFEATURE_MASK_XTILECFG and XFEATURE_MASK_XTILEDATA can't be set unless
the other is set. Do we need to do something similar for AVX-512 and
MPX?
> + GUEST_SYNC(0);
> +
> + xsetbv(0, supported_xcr0);
> +
> + GUEST_DONE();
> +}
> +
> +static void guest_gp_handler(struct ex_regs *regs)
> +{
> + GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0.");
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + struct kvm_vcpu *vcpu;
> + struct kvm_run *run;
> + struct kvm_vm *vm;
> + struct ucall uc;
> +
> + TEST_REQUIRE(kvm_cpu_has(X86_FEATURE_XSAVE));
> +
> + /* Tell stdout not to buffer its content */
> + setbuf(stdout, NULL);
> +
> + vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> + run = vcpu->run;
> +
> + vm_init_descriptor_tables(vm);
> + vcpu_init_descriptor_tables(vcpu);
> +
> + while (1) {
> + vcpu_run(vcpu);
> +
> + TEST_ASSERT(run->exit_reason == KVM_EXIT_IO,
> + "Unexpected exit reason: %u (%s),\n",
> + run->exit_reason,
> + exit_reason_str(run->exit_reason));
> +
> + switch (get_ucall(vcpu, &uc)) {
> + case UCALL_SYNC:
> + vm_install_exception_handler(vm, GP_VECTOR,
> + guest_gp_handler);
> + break;
> + case UCALL_ABORT:
> + REPORT_GUEST_ASSERT(uc);
> + break;
> + case UCALL_DONE:
> + goto done;
> + default:
> + TEST_FAIL("Unknown ucall %lu", uc.cmd);
> + }
> + }
> +
> +done:
> + kvm_vm_free(vm);
> + return 0;
> +}
> --
> 2.39.0.314.g84b9a713c41-goog
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] KVM: selftests: Add XCR0 Test
2022-12-27 22:07 ` Jim Mattson
@ 2022-12-28 0:15 ` Aaron Lewis
0 siblings, 0 replies; 13+ messages in thread
From: Aaron Lewis @ 2022-12-28 0:15 UTC (permalink / raw)
To: Jim Mattson; +Cc: kvm, pbonzini, seanjc, like.xu.linux
> > +#include "kvm_util.h"
> > +#include "processor.h"
> > +
> > +#define XFEATURE_MASK_SSE (1ul << 1)
> > +#define XFEATURE_MASK_YMM (1ul << 2)
> > +#define XFEATURE_MASK_BNDREGS (1ul << 3)
> > +#define XFEATURE_MASK_BNDCSR (1ul << 4)
> > +#define XFEATURE_MASK_OPMASK (1ul << 5)
> > +#define XFEATURE_MASK_ZMM_Hi256 (1ul << 6)
> > +#define XFEATURE_MASK_Hi16_ZMM (1ul << 7)
> > +#define XFEATURE_MASK_XTILECFG (1ul << 17)
> > +#define XFEATURE_MASK_XTILEDATA (1ul << 18)
> > +#define XFEATURE_MASK_XTILE (XFEATURE_MASK_XTILECFG | \
> > + XFEATURE_MASK_XTILEDATA)
>
> With XSETBV hoisted into processor.h, shouldn't these macros be more
> widely available as well?
Sure, I'll hoist them up there too.
>
> > +static uint64_t get_supported_user_xfeatures(void)
> > +{
> > + uint32_t a, b, c, d;
> > +
> > + cpuid(0xd, &a, &b, &c, &d);
> > +
> > + return a | ((uint64_t)d << 32);
> > +}
> > +
> > + GUEST_ASSERT(xcr0_rest == 1ul);
> > +
> > + /* Check AVX */
> > + xfeature_mask = XFEATURE_MASK_SSE | XFEATURE_MASK_YMM;
> > + supported_state = supported_xcr0 & xfeature_mask;
> > + GUEST_ASSERT(supported_state != XFEATURE_MASK_YMM);
> > +
> > + /* Check MPX */
> > + xfeature_mask = XFEATURE_MASK_BNDREGS | XFEATURE_MASK_BNDCSR;
> > + supported_state = supported_xcr0 & xfeature_mask;
> > + GUEST_ASSERT((supported_state == xfeature_mask) ||
> > + (supported_state == 0ul));
> > +
> > + /* Check AVX-512 */
> > + xfeature_mask = XFEATURE_MASK_OPMASK |
> > + XFEATURE_MASK_ZMM_Hi256 |
> > + XFEATURE_MASK_Hi16_ZMM;
> > + supported_state = supported_xcr0 & xfeature_mask;
> > + GUEST_ASSERT((supported_state == xfeature_mask) ||
> > + (supported_state == 0ul));
> > +
> > + /* Check AMX */
> > + xfeature_mask = XFEATURE_MASK_XTILE;
> > + supported_state = supported_xcr0 & xfeature_mask;
> > + GUEST_ASSERT((supported_state == xfeature_mask) ||
> > + (supported_state == 0ul));
>
> In this series, you've added code to __do_cpuid_func() to ensure that
> XFEATURE_MASK_XTILECFG and XFEATURE_MASK_XTILEDATA can't be set unless
> the other is set. Do we need to do something similar for AVX-512 and
> MPX?
That does sound like a natural extension of this. I can add support
for that in v2.
>
> > + GUEST_SYNC(0);
> > +
> > + xsetbv(0, supported_xcr0);
> > +
> > + GUEST_DONE();
> > +}
> > +
> > +static void guest_gp_handler(struct ex_regs *regs)
> > +{
> > + GUEST_ASSERT(!"Failed to set the supported xfeature bits in XCR0.");
> > +}
> > +
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2022-12-28 0:15 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-27 18:37 [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Aaron Lewis
2022-12-27 18:37 ` [PATCH 1/3] KVM: x86: Clear XTILE_CFG if XTILE_DATA is clear Aaron Lewis
2022-12-27 19:56 ` Jim Mattson
2022-12-27 20:43 ` Aaron Lewis
2022-12-27 21:14 ` Jim Mattson
2022-12-27 21:36 ` Jim Mattson
2022-12-27 18:37 ` [PATCH 2/3] KVM: selftests: Hoist XGETBV and XSETBV to make them more accessible Aaron Lewis
2022-12-27 21:23 ` Jim Mattson
2022-12-27 18:37 ` [PATCH 3/3] KVM: selftests: Add XCR0 Test Aaron Lewis
2022-12-27 22:07 ` Jim Mattson
2022-12-28 0:15 ` Aaron Lewis
2022-12-27 20:00 ` [PATCH 0/3] Clean up AMX cpuid bits XTILE_CFG and XTILE_DATA Jim Mattson
2022-12-27 20:40 ` Aaron Lewis
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.