Re: [kvm-unit-tests PATCH] x86/emulator64: Extend non-canonical memory access tests with CR2 coverage

[Sean Christopherson <seanjc@xxxxxxxxxx>]

On Thu, Jun 12, 2025, Mathias Krause wrote:
> Extend the non-canonical memory access tests to verify CR2 stays
> unchanged.
> 
> There's currently a bug in QEMU/TCG that breaks that assumption.
> 
> Link: https://gitlab.com/qemu-project/qemu/-/issues/928
> Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx>
> ---
>  x86/emulator64.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/x86/emulator64.c b/x86/emulator64.c
> index 5d1bb0f06d4f..abef2bda29f1 100644
> --- a/x86/emulator64.c
> +++ b/x86/emulator64.c
> @@ -325,16 +325,39 @@ static void test_mmx_movq_mf(uint64_t *mem)
>  	report(exception_vector() == MF_VECTOR, "movq mmx generates #MF");
>  }
>  
> +#define CR2_REF_VALUE	0xdecafbadUL
> +
> +static void setup_cr2(void)
> +{
> +	write_cr2(CR2_REF_VALUE);
> +}
> +
> +static void check_cr2(void)
> +{
> +	unsigned long cr2 = read_cr2();
> +
> +	if (cr2 == CR2_REF_VALUE) {
> +		report(true, "CR2 unchanged");
> +	} else {
> +		report(false, "CR2 changed from %#lx to %#lx", CR2_REF_VALUE, cr2);
> +		setup_cr2();

Writing CR2 isn't expensive in the grand scheme, so rather than conditionally
re-write CR2, I think it makes sense to write CR2 at the start of every testcase,
and then just do "report(cr2 == CR2_REF_VALUE".

> +	}
> +}
> +
>  static void test_jmp_noncanonical(uint64_t *mem)
>  {
> +	setup_cr2();
>  	*mem = NONCANONICAL;
>  	asm volatile (ASM_TRY("1f") "jmp *%0; 1:" : : "m"(*mem));
>  	report(exception_vector() == GP_VECTOR,
>  	       "jump to non-canonical address");
> +	check_cr2();
>  }
>  
>  static void test_reg_noncanonical(void)
>  {
> +	setup_cr2();
> +
>  	/* RAX based, should #GP(0) */
>  	asm volatile(ASM_TRY("1f") "orq $0, (%[noncanonical]); 1:"
>  		     : : [noncanonical]"a"(NONCANONICAL));
> @@ -342,6 +365,7 @@ static void test_reg_noncanonical(void)
>  	       "non-canonical memory access, should %s(0), got %s(%u)",
>  	       exception_mnemonic(GP_VECTOR),
>  	       exception_mnemonic(exception_vector()), exception_error_code());
> +	check_cr2();

And then rather than add more copy+paste, what if we add a macro to handle the
checks?  Then the CR2 validation can slot in nicely (and maybe someday the macro
could be used outside of the x86/emulator64.c).

Attached patches yield:	

#define CR2_REF_VALUE	0xdecafbadUL

#define ASM_TRY_NONCANONICAL(insn, inputs, access, ex_vector)			\
do {										\
	unsigned int vector, ec;						\
										\
	write_cr2(CR2_REF_VALUE);						\
										\
	asm volatile(ASM_TRY("1f") insn "; 1:" :: inputs);			\
										\
	vector = exception_vector();						\
	ec = exception_error_code();						\
										\
	report(vector == ex_vector && !ec,					\
	      "non-canonical " access ", should %s(0), got %s(%u)",		\
	      exception_mnemonic(ex_vector), exception_mnemonic(vector), ec);	\
										\
	if (vector != PF_VECTOR) {						\
		unsigned long cr2  = read_cr2();				\
										\
		report(cr2 == CR2_REF_VALUE,					\
		       "Wanted CR2 '0x%lx', got '0x%lx", CR2_REF_VALUE, cr2);	\
	}									\
} while (0)

static void test_jmp_noncanonical(uint64_t *mem)
{
	*mem = NONCANONICAL;

	ASM_TRY_NONCANONICAL("jmp *%0", "m"(*mem), "jmp", GP_VECTOR);
}

static void test_reg_noncanonical(void)
{
	/* RAX based, should #GP(0) */
	ASM_TRY_NONCANONICAL("orq $0, (%[nc])", [nc]"a"(NONCANONICAL),
			     "memory access", GP_VECTOR);

	/* RSP based, should #SS(0) */
	ASM_TRY_NONCANONICAL("orq $0, (%%rsp,%[nc],1)", [nc]"r"(NONCANONICAL),
			     "rsp-based access", SS_VECTOR);

	/* RBP based, should #SS(0) */
	ASM_TRY_NONCANONICAL("orq $0, (%%rbp,%[nc],1)", [nc]"r"(NONCANONICAL),
			     "rbp-based access", SS_VECTOR);
}