diff mbox series

[v3,04/10] KVM: selftests: Move flds instruction emulation failure handling to header

Message ID 20221031180045.3581757-5-dmatlack@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: selftests: Fix and clean up emulator_error_test | expand

Commit Message

David Matlack Oct. 31, 2022, 6 p.m. UTC 
Move the flds instruction emulation failure handling code to a header
so it can be re-used in an upcoming test.

No functional change intended.

Signed-off-by: David Matlack <dmatlack@google.com>
---
 .../selftests/kvm/x86_64/flds_emulation.h     | 59 +++++++++++++++++++
 .../smaller_maxphyaddr_emulation_test.c       | 45 ++------------
 2 files changed, 64 insertions(+), 40 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/x86_64/flds_emulation.h

Comments

Sean Christopherson Oct. 31, 2022, 6:28 p.m. UTC | #1 
On Mon, Oct 31, 2022, David Matlack wrote:
> Move the flds instruction emulation failure handling code to a header
> so it can be re-used in an upcoming test.
> 
> No functional change intended.
> 
> Signed-off-by: David Matlack <dmatlack@google.com>
> ---
>  .../selftests/kvm/x86_64/flds_emulation.h     | 59 +++++++++++++++++++
>  .../smaller_maxphyaddr_emulation_test.c       | 45 ++------------
>  2 files changed, 64 insertions(+), 40 deletions(-)
>  create mode 100644 tools/testing/selftests/kvm/x86_64/flds_emulation.h
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/flds_emulation.h b/tools/testing/selftests/kvm/x86_64/flds_emulation.h
> new file mode 100644
> index 000000000000..be0b4e0dd722
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/x86_64/flds_emulation.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef SELFTEST_KVM_FLDS_EMULATION_H
> +#define SELFTEST_KVM_FLDS_EMULATION_H
> +
> +#include "kvm_util.h"
> +
> +#define FLDS_MEM_EAX ".byte 0xd9, 0x00"
> +
> +/*
> + * flds is an instruction that the KVM instruction emulator is known not to
> + * support. This can be used in guest code along with a mechanism to force
> + * KVM to emulate the instruction (e.g. by providing an MMIO address) to
> + * exercise emulation failures.
> + */
> +static inline void flds(uint64_t address)
> +{
> +	__asm__ __volatile__(FLDS_MEM_EAX :: "a"(address));
> +}
> +
> +static inline void assert_exit_for_flds_emulation_failure(struct kvm_vcpu *vcpu)

I think it makes sense to keeping the bundling of the assert+skip.  As written,
the last test doesn't need to skip, but that may not always hold true, e.g. if
the test adds more stages to verify KVM handles page splits correctly, and even
when a skip is required, it does no harm.  I can't think of a scenario where a
test would want an FLDS emulation error but wouldn't want to skip the instruction,
e.g. injecting a fault from userspace is largely an orthogonal test.

Maybe this as a helper name?  I don't think it's necessary to include "assert"
anywhere in the name, the idea being that "emulated" provides a hint that it's a
non-trivial helper.  

  static inline void skip_emulated_flds(struct kvm_vcpu *vcpu)

or skip_emulated_flds_instruction() if we're concerned that it might not be obvious
"flds" is an instruction mnemonic.
David Matlack Nov. 2, 2022, 6:17 p.m. UTC | #2 
On Mon, Oct 31, 2022 at 11:28 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Mon, Oct 31, 2022, David Matlack wrote:
> > +
> > +static inline void assert_exit_for_flds_emulation_failure(struct kvm_vcpu *vcpu)
>
> I think it makes sense to keeping the bundling of the assert+skip.  As written,
> the last test doesn't need to skip, but that may not always hold true, e.g. if
> the test adds more stages to verify KVM handles page splits correctly, and even
> when a skip is required, it does no harm.  I can't think of a scenario where a
> test would want an FLDS emulation error but wouldn't want to skip the instruction,
> e.g. injecting a fault from userspace is largely an orthogonal test.
>
> Maybe this as a helper name?  I don't think it's necessary to include "assert"
> anywhere in the name, the idea being that "emulated" provides a hint that it's a
> non-trivial helper.
>
>   static inline void skip_emulated_flds(struct kvm_vcpu *vcpu)
>
> or skip_emulated_flds_instruction() if we're concerned that it might not be obvious
> "flds" is an instruction mnemonic.

I kept them separate for readability, but otherwise I have no argument
against bundling. I find skip_emulated*() somewhat misleading since
flds is not actually emulated (successfully). I'm trending toward
something like handle_flds_emulation_failure_exit() for v4.
Sean Christopherson Nov. 2, 2022, 7:03 p.m. UTC | #3 
On Wed, Nov 02, 2022, David Matlack wrote:
> On Mon, Oct 31, 2022 at 11:28 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Mon, Oct 31, 2022, David Matlack wrote:
> > > +
> > > +static inline void assert_exit_for_flds_emulation_failure(struct kvm_vcpu *vcpu)
> >
> > I think it makes sense to keeping the bundling of the assert+skip.  As written,
> > the last test doesn't need to skip, but that may not always hold true, e.g. if
> > the test adds more stages to verify KVM handles page splits correctly, and even
> > when a skip is required, it does no harm.  I can't think of a scenario where a
> > test would want an FLDS emulation error but wouldn't want to skip the instruction,
> > e.g. injecting a fault from userspace is largely an orthogonal test.
> >
> > Maybe this as a helper name?  I don't think it's necessary to include "assert"
> > anywhere in the name, the idea being that "emulated" provides a hint that it's a
> > non-trivial helper.
> >
> >   static inline void skip_emulated_flds(struct kvm_vcpu *vcpu)
> >
> > or skip_emulated_flds_instruction() if we're concerned that it might not be obvious
> > "flds" is an instruction mnemonic.
> 
> I kept them separate for readability,

Ha, and of course I found assert_exit_for_flds_emulation_failure() hard to read :-)

> but otherwise I have no argument against bundling. I find skip_emulated*()
> somewhat misleading since flds is not actually emulated (successfully). I'm
> trending toward something like handle_flds_emulation_failure_exit() for v4.

How about "skip_flds_on_emulation_failure_exit()"?  "handle" is quite generic and
doesn't provide any hints as to what the function actually does under the hood.
David Matlack Nov. 2, 2022, 10:02 p.m. UTC | #4 
On Wed, Nov 2, 2022 at 12:03 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Wed, Nov 02, 2022, David Matlack wrote:
> > On Mon, Oct 31, 2022 at 11:28 AM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Mon, Oct 31, 2022, David Matlack wrote:
> > > > +
> > > > +static inline void assert_exit_for_flds_emulation_failure(struct kvm_vcpu *vcpu)
> > >
> > > I think it makes sense to keeping the bundling of the assert+skip.  As written,
> > > the last test doesn't need to skip, but that may not always hold true, e.g. if
> > > the test adds more stages to verify KVM handles page splits correctly, and even
> > > when a skip is required, it does no harm.  I can't think of a scenario where a
> > > test would want an FLDS emulation error but wouldn't want to skip the instruction,
> > > e.g. injecting a fault from userspace is largely an orthogonal test.
> > >
> > > Maybe this as a helper name?  I don't think it's necessary to include "assert"
> > > anywhere in the name, the idea being that "emulated" provides a hint that it's a
> > > non-trivial helper.
> > >
> > >   static inline void skip_emulated_flds(struct kvm_vcpu *vcpu)
> > >
> > > or skip_emulated_flds_instruction() if we're concerned that it might not be obvious
> > > "flds" is an instruction mnemonic.
> >
> > I kept them separate for readability,
>
> Ha, and of course I found assert_exit_for_flds_emulation_failure() hard to read :-)
>
> > but otherwise I have no argument against bundling. I find skip_emulated*()
> > somewhat misleading since flds is not actually emulated (successfully). I'm
> > trending toward something like handle_flds_emulation_failure_exit() for v4.
>
> How about "skip_flds_on_emulation_failure_exit()"?  "handle" is quite generic and
> doesn't provide any hints as to what the function actually does under the hood.

LGTM. Paolo can you apply the name change when queueing v4 (assuming
there are no other comments)? If not I'd be happy to send a v5.
diff mbox series

Patch

diff --git a/tools/testing/selftests/kvm/x86_64/flds_emulation.h b/tools/testing/selftests/kvm/x86_64/flds_emulation.h
new file mode 100644
index 000000000000..be0b4e0dd722
--- /dev/null
+++ b/tools/testing/selftests/kvm/x86_64/flds_emulation.h
@@ -0,0 +1,59 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+#ifndef SELFTEST_KVM_FLDS_EMULATION_H
+#define SELFTEST_KVM_FLDS_EMULATION_H
+
+#include "kvm_util.h"
+
+#define FLDS_MEM_EAX ".byte 0xd9, 0x00"
+
+/*
+ * flds is an instruction that the KVM instruction emulator is known not to
+ * support. This can be used in guest code along with a mechanism to force
+ * KVM to emulate the instruction (e.g. by providing an MMIO address) to
+ * exercise emulation failures.
+ */
+static inline void flds(uint64_t address)
+{
+	__asm__ __volatile__(FLDS_MEM_EAX :: "a"(address));
+}
+
+static inline void assert_exit_for_flds_emulation_failure(struct kvm_vcpu *vcpu)
+{
+	struct kvm_run *run = vcpu->run;
+	uint8_t *insn_bytes;
+	uint64_t flags;
+
+	TEST_ASSERT(run->exit_reason == KVM_EXIT_INTERNAL_ERROR,
+		    "Unexpected exit reason: %u (%s)",
+		    run->exit_reason,
+		    exit_reason_str(run->exit_reason));
+
+	TEST_ASSERT(run->emulation_failure.suberror == KVM_INTERNAL_ERROR_EMULATION,
+		    "Unexpected suberror: %u",
+		    run->emulation_failure.suberror);
+
+	flags = run->emulation_failure.flags;
+	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
+		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
+		    "run->emulation_failure is missing instruction bytes");
+
+	TEST_ASSERT(run->emulation_failure.insn_size >= 2,
+		    "Expected a 2-byte opcode for 'flds', got %d bytes",
+		    run->emulation_failure.insn_size);
+
+	insn_bytes = run->emulation_failure.insn_bytes;
+	TEST_ASSERT(insn_bytes[0] == 0xd9 && insn_bytes[1] == 0,
+		    "Expected 'flds [eax]', opcode '0xd9 0x00', got opcode 0x%02x 0x%02x\n",
+		    insn_bytes[0], insn_bytes[1]);
+}
+
+static inline void skip_flds_instruction(struct kvm_vcpu *vcpu)
+{
+	struct kvm_regs regs;
+
+	vcpu_regs_get(vcpu, &regs);
+	regs.rip += 2;
+	vcpu_regs_set(vcpu, &regs);
+}
+
+#endif /* !SELFTEST_KVM_FLDS_EMULATION_H */
diff --git a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
index f9fdf365dff7..f438a98e8bb7 100644
--- a/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
+++ b/tools/testing/selftests/kvm/x86_64/smaller_maxphyaddr_emulation_test.c
@@ -8,6 +8,8 @@ 
 
 #define _GNU_SOURCE /* for program_invocation_short_name */
 
+#include "flds_emulation.h"
+
 #include "test_util.h"
 #include "kvm_util.h"
 #include "vmx.h"
@@ -19,50 +21,12 @@ 
 #define MEM_REGION_SLOT	10
 #define MEM_REGION_SIZE PAGE_SIZE
 
-#define FLDS_MEM_EAX ".byte 0xd9, 0x00"
-
 static void guest_code(void)
 {
-	__asm__ __volatile__(FLDS_MEM_EAX :: "a"(MEM_REGION_GVA));
-
+	flds(MEM_REGION_GVA);
 	GUEST_DONE();
 }
 
-static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu)
-{
-	struct kvm_run *run = vcpu->run;
-	struct kvm_regs regs;
-	uint8_t *insn_bytes;
-	uint64_t flags;
-
-	TEST_ASSERT(run->exit_reason == KVM_EXIT_INTERNAL_ERROR,
-		    "Unexpected exit reason: %u (%s)",
-		    run->exit_reason,
-		    exit_reason_str(run->exit_reason));
-
-	TEST_ASSERT(run->emulation_failure.suberror == KVM_INTERNAL_ERROR_EMULATION,
-		    "Unexpected suberror: %u",
-		    run->emulation_failure.suberror);
-
-	flags = run->emulation_failure.flags;
-	TEST_ASSERT(run->emulation_failure.ndata >= 3 &&
-		    flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES,
-		    "run->emulation_failure is missing instruction bytes");
-
-	TEST_ASSERT(run->emulation_failure.insn_size >= 2,
-		    "Expected a 2-byte opcode for 'flds', got %d bytes",
-		    run->emulation_failure.insn_size);
-
-	insn_bytes = run->emulation_failure.insn_bytes;
-	TEST_ASSERT(insn_bytes[0] == 0xd9 && insn_bytes[1] == 0,
-		    "Expected 'flds [eax]', opcode '0xd9 0x00', got opcode 0x%02x 0x%02x\n",
-		    insn_bytes[0], insn_bytes[1]);
-
-	vcpu_regs_get(vcpu, &regs);
-	regs.rip += 2;
-	vcpu_regs_set(vcpu, &regs);
-}
-
 int main(int argc, char *argv[])
 {
 	struct kvm_vcpu *vcpu;
@@ -97,7 +61,8 @@  int main(int argc, char *argv[])
 	vm_set_page_table_entry(vm, vcpu, MEM_REGION_GVA, pte | (1ull << 36));
 
 	vcpu_run(vcpu);
-	process_exit_on_emulation_error(vcpu);
+	assert_exit_for_flds_emulation_failure(vcpu);
+	skip_flds_instruction(vcpu);
 	vcpu_run(vcpu);
 	ASSERT_EQ(get_ucall(vcpu, NULL), UCALL_DONE);