diff mbox series

[2/4] KVM: selftests: x86: Add check of IA32_XFD in amx_test

Message ID 20230110185823.1856951-3-mizhang@google.com (mailing list archive)
State New, archived
Headers show
Series Add extra checkings to amx_test | expand

Commit Message

Mingwei Zhang Jan. 10, 2023, 6:58 p.m. UTC
When #NM is triggered, the handler needs to ensure the exception is
triggered by AMX by checking IA32_XFD_ERR and not because of CR0.TS[bit 3]
is 1. Note that the value of IA32_XFD_ERR comes from "the logical AND of
the IA32_XFD MSR and the bitmap corresponding to the state components
required by the faulting instruction." (Intel SDM vol 1. Section 13.14)

Add the missing check of CR0.TS before checking the value of IA32_XFD_ERR.
In addition, add an extra check to IA32_XFD to ensure the behavior is
consistent with the AMX archtecture. In addition, repeat the checks across
context switch to ensure the values of IA32_XFD and IA32_XFD_ERR are well
preserved.

Signed-off-by: Mingwei Zhang <mizhang@google.com>
---
 tools/testing/selftests/kvm/x86_64/amx_test.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Sean Christopherson Feb. 8, 2023, 1:17 a.m. UTC | #1
On Tue, Jan 10, 2023, Mingwei Zhang wrote:
> When #NM is triggered, the handler needs to ensure the exception is

State what the patch does (and explain why), don't say ABC needs/should do XYZ.
The #NM handler doesn't _need_ to ensure the #NM wasn't due to CR0.TS

> triggered by AMX by checking IA32_XFD_ERR and not because of CR0.TS[bit 3]

CR0.TS is a single bit, using square braces makes it look like an index into CR0.TS.
I would drop the "bit 3" part altogether, it's not relevant

> is 1. Note that the value of IA32_XFD_ERR comes from "the logical AND of
> the IA32_XFD MSR and the bitmap corresponding to the state components
> required by the faulting instruction." (Intel SDM vol 1. Section 13.14)
> 
> Add the missing check of CR0.TS before checking the value of IA32_XFD_ERR.
> In addition, add an extra check to IA32_XFD to ensure the behavior is
> consistent with the AMX archtecture. In addition, repeat the checks across
> context switch to ensure the values of IA32_XFD and IA32_XFD_ERR are well
> preserved.

Split the MSR_IA32_XFD checks to a separate patch.  Or I guess given the shortlog
is about IA32_XFD, split the CR0.TS check to a separate patch.

> 
> Signed-off-by: Mingwei Zhang <mizhang@google.com>
> ---
>  tools/testing/selftests/kvm/x86_64/amx_test.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
> index 16533949a189..b2369f956fea 100644
> --- a/tools/testing/selftests/kvm/x86_64/amx_test.c
> +++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
> @@ -226,9 +226,12 @@ void guest_nm_handler(struct ex_regs *regs)
>  {
>  	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
>  	GUEST_SYNC(7);
> +	GUEST_ASSERT((get_cr0() & X86_CR0_TS) == 0);

	GUEST_ASSERT(!(get_cr0() & X86_CR0_TS));

>  	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> +	GUEST_ASSERT((rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);

Isn't this just

	GUEST_ASSERT(rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA);

or am I horribly misreading the code?

>  	GUEST_SYNC(8);
>  	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
> +	GUEST_ASSERT((rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);

Same here.

>  	/* Clear xfd_err */
>  	wrmsr(MSR_IA32_XFD_ERR, 0);
>  	/* xfd=0, enable amx */
> -- 
> 2.39.0.314.g84b9a713c41-goog
>
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/x86_64/amx_test.c b/tools/testing/selftests/kvm/x86_64/amx_test.c
index 16533949a189..b2369f956fea 100644
--- a/tools/testing/selftests/kvm/x86_64/amx_test.c
+++ b/tools/testing/selftests/kvm/x86_64/amx_test.c
@@ -226,9 +226,12 @@  void guest_nm_handler(struct ex_regs *regs)
 {
 	/* Check if #NM is triggered by XFEATURE_MASK_XTILEDATA */
 	GUEST_SYNC(7);
+	GUEST_ASSERT((get_cr0() & X86_CR0_TS) == 0);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT((rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);
 	GUEST_SYNC(8);
 	GUEST_ASSERT(rdmsr(MSR_IA32_XFD_ERR) == XFEATURE_MASK_XTILEDATA);
+	GUEST_ASSERT((rdmsr(MSR_IA32_XFD) & XFEATURE_MASK_XTILEDATA) == XFEATURE_MASK_XTILEDATA);
 	/* Clear xfd_err */
 	wrmsr(MSR_IA32_XFD_ERR, 0);
 	/* xfd=0, enable amx */