Re: [PATCH kvm-unit-tests] memory: Skip tests for instructions that are absent

[Thomas Huth <thuth@xxxxxxxxxx>]

On 03/04/2023 18.30, Paolo Bonzini wrote:
> Checking that instructions are absent is broken when running with CPU
> models other than the bare metal processor's, because neither VMX nor SVM have
> intercept controls for the instructions.
>
> This can even happen with "-cpu max" when running under nested
> virtualization, which is the current situation in the Fedora KVM job
> on Cirrus-CI:
>
> FAIL: clflushopt (ABSENT)
> FAIL: clwb (ABSENT)
>
> In other words it looks like the features have been marked as disabled
> in the L0 host, while the hardware supports them.
>
> Reported-by: Thomas Huth <thuth@xxxxxxxxxx>
> Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> ---
>   x86/memory.c | 69 +++++++++++++++++++++++++++-------------------------
>   1 file changed, 36 insertions(+), 33 deletions(-)
>
> diff --git a/x86/memory.c b/x86/memory.c
> index 351e7c0..ffd4eeb 100644
> --- a/x86/memory.c
> +++ b/x86/memory.c
> @@ -25,53 +25,56 @@ static void handle_ud(struct ex_regs *regs)
>   
>   int main(int ac, char **av)
>   {
> -	int expected;
> -
>   	handle_exception(UD_VECTOR, handle_ud);
>   
>   	/* 3-byte instructions: */
>   	isize = 3;
>   
> -	expected = !this_cpu_has(X86_FEATURE_CLFLUSH); /* CLFLUSH */
> -	ud = 0;
> -	asm volatile("clflush (%0)" : : "b" (&target));
> -	report(ud == expected, "clflush (%s)", expected ? "ABSENT" : "present");
> +	if (this_cpu_has(X86_FEATURE_CLFLUSH)) { /* CLFLUSH */
> +		ud = 0;
> +		asm volatile("clflush (%0)" : : "b" (&target));
> +		report(!ud, "clflush");
> +	}
>   
> -	expected = !this_cpu_has(X86_FEATURE_XMM); /* SSE */
> -	ud = 0;
> -	asm volatile("sfence");
> -	report(ud == expected, "sfence (%s)", expected ? "ABSENT" : "present");
> +	if (this_cpu_has(X86_FEATURE_XMM)) { /* SSE */
> +		ud = 0;
> +		asm volatile("sfence");
> +		report(!ud, "sfence");
> +	}
>   
> -	expected = !this_cpu_has(X86_FEATURE_XMM2); /* SSE2 */
> -	ud = 0;
> -	asm volatile("lfence");
> -	report(ud == expected, "lfence (%s)", expected ? "ABSENT" : "present");
> +	if (this_cpu_has(X86_FEATURE_XMM2)) { /* SSE2 */
> +		ud = 0;
> +		asm volatile("lfence");
> +		report(!ud, "lfence");
>   
> -	ud = 0;
> -	asm volatile("mfence");
> -	report(ud == expected, "mfence (%s)", expected ? "ABSENT" : "present");
> +		ud = 0;
> +		asm volatile("mfence");
> +		report(!ud, "mfence");
> +	}
>   
>   	/* 4-byte instructions: */
>   	isize = 4;
>   
> -	expected = !this_cpu_has(X86_FEATURE_CLFLUSHOPT); /* CLFLUSHOPT */
> -	ud = 0;
> -	/* clflushopt (%rbx): */
> -	asm volatile(".byte 0x66, 0x0f, 0xae, 0x3b" : : "b" (&target));
> -	report(ud == expected, "clflushopt (%s)",
> -	       expected ? "ABSENT" : "present");
> +	if (this_cpu_has(X86_FEATURE_CLFLUSHOPT)) { /* CLFLUSHOPT */
> +		ud = 0;
> +		/* clflushopt (%rbx): */
> +		asm volatile(".byte 0x66, 0x0f, 0xae, 0x3b" : : "b" (&target));
> +		report(!ud, "clflushopt");
> +	}
>   
> -	expected = !this_cpu_has(X86_FEATURE_CLWB); /* CLWB */
> -	ud = 0;
> -	/* clwb (%rbx): */
> -	asm volatile(".byte 0x66, 0x0f, 0xae, 0x33" : : "b" (&target));
> -	report(ud == expected, "clwb (%s)", expected ? "ABSENT" : "present");
> +	if (this_cpu_has(X86_FEATURE_CLWB)) { /* CLWB */
> +		ud = 0;
> +		/* clwb (%rbx): */
> +		asm volatile(".byte 0x66, 0x0f, 0xae, 0x33" : : "b" (&target));
> +		report(!ud, "clwb");
> +	}
>   
> -	expected = !this_cpu_has(X86_FEATURE_PCOMMIT); /* PCOMMIT */
> -	ud = 0;
> -	/* pcommit: */
> -	asm volatile(".byte 0x66, 0x0f, 0xae, 0xf8");
> -	report(ud == expected, "pcommit (%s)", expected ? "ABSENT" : "present");
> +	if (this_cpu_has(X86_FEATURE_PCOMMIT)) { /* PCOMMIT */
> +		ud = 0;
> +		/* pcommit: */
> +		asm volatile(".byte 0x66, 0x0f, 0xae, 0xf8");
> +		report(!ud, "pcommit");
> +	}

Maybe add a

    } else {
       report_skip(...);
    }

after each block? ... so that it is more obvious in the output that 
something has not been executed?

Well, I don't care too much, so with or without that change:

Reviewed-by: Thomas Huth <thuth@xxxxxxxxxx>