| Message ID | 20230403163046.387460-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] memory: Skip tests for instructions that are absent | expand |
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@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > 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@redhat.com>
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"); + } return report_summary(); }
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@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- x86/memory.c | 69 +++++++++++++++++++++++++++------------------------- 1 file changed, 36 insertions(+), 33 deletions(-)