| Message ID | 20221018214612.3445074-3-dmatlack@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: selftests: Fix and clean up emulator_error_test | expand |
On Tue, Oct 18, 2022, David Matlack wrote: > Explicitly require instruction bytes to be available in > run->emulation_failure by asserting that they are present. Note that the > test already requires the instruction bytes to be present because that's > the only way the test will advance the RIP past the flds and get to > GUEST_DONE(). > > Signed-off-by: David Matlack <dmatlack@google.com> > --- > .../smaller_maxphyaddr_emulation_test.c | 47 +++++++++---------- > 1 file changed, 23 insertions(+), 24 deletions(-) > > 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 6ed996988a5a..c5353ad0e06d 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 > @@ -65,30 +65,29 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu) > "Unexpected suberror: %u", > run->emulation_failure.suberror); > > - if (run->emulation_failure.ndata >= 1) { > - flags = run->emulation_failure.flags; > - if ((flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES) && > - run->emulation_failure.ndata >= 3) { > - insn_size = run->emulation_failure.insn_size; > - insn_bytes = run->emulation_failure.insn_bytes; > - > - TEST_ASSERT(insn_size <= 15 && insn_size > 0, > - "Unexpected instruction size: %u", > - insn_size); > - > - TEST_ASSERT(is_flds(insn_bytes, insn_size), > - "Unexpected instruction. Expected 'flds' (0xd9 /0)"); > - > - /* > - * If is_flds() succeeded then the instruction bytes > - * contained an flds instruction that is 2-bytes in > - * length (ie: no prefix, no SIB, no displacement). > - */ > - vcpu_regs_get(vcpu, ®s); > - regs.rip += 2; > - vcpu_regs_set(vcpu, ®s); > - } > - } > + 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"); > + > + insn_size = run->emulation_failure.insn_size; > + insn_bytes = run->emulation_failure.insn_bytes; > + > + TEST_ASSERT(insn_size <= 15 && insn_size > 0, > + "Unexpected instruction size: %u", > + insn_size); Unnecessary newline, insn_size fits comfortably on the line above. > + > + TEST_ASSERT(is_flds(insn_bytes, insn_size), > + "Unexpected instruction. Expected 'flds' (0xd9 /0)"); > + > + /* > + * If is_flds() succeeded then the instruction bytes contained an flds > + * instruction that is 2-bytes in length (ie: no prefix, no SIB, no > + * displacement). > + */ > + vcpu_regs_get(vcpu, ®s); > + regs.rip += 2; This whole sequence is silly. Assert that size > 0 but < 15, then assert that it's >= 2, then skip exactly two bytes and effective "assert" that it's '2. And while I appreciate the ModR/M decoding, IMO it does more harm than good. If someone can follow the ModR/M decoding, they can figure out a hardcode opcode. E.g. IMO this is much simpler and will be easier to debug. #define FLDS_MEM_EAX ".byte 0xd9, 0x00" static void guest_code(void) { __asm__ __volatile__(FLDS_MEM_EAX :: "a"(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%x 0x%x\n", insn_bytes[0], insn_bytes[1]); vcpu_regs_get(vcpu, ®s); regs.rip += 2; vcpu_regs_set(vcpu, ®s); }
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 6ed996988a5a..c5353ad0e06d 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 @@ -65,30 +65,29 @@ static void process_exit_on_emulation_error(struct kvm_vcpu *vcpu) "Unexpected suberror: %u", run->emulation_failure.suberror); - if (run->emulation_failure.ndata >= 1) { - flags = run->emulation_failure.flags; - if ((flags & KVM_INTERNAL_ERROR_EMULATION_FLAG_INSTRUCTION_BYTES) && - run->emulation_failure.ndata >= 3) { - insn_size = run->emulation_failure.insn_size; - insn_bytes = run->emulation_failure.insn_bytes; - - TEST_ASSERT(insn_size <= 15 && insn_size > 0, - "Unexpected instruction size: %u", - insn_size); - - TEST_ASSERT(is_flds(insn_bytes, insn_size), - "Unexpected instruction. Expected 'flds' (0xd9 /0)"); - - /* - * If is_flds() succeeded then the instruction bytes - * contained an flds instruction that is 2-bytes in - * length (ie: no prefix, no SIB, no displacement). - */ - vcpu_regs_get(vcpu, ®s); - regs.rip += 2; - vcpu_regs_set(vcpu, ®s); - } - } + 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"); + + insn_size = run->emulation_failure.insn_size; + insn_bytes = run->emulation_failure.insn_bytes; + + TEST_ASSERT(insn_size <= 15 && insn_size > 0, + "Unexpected instruction size: %u", + insn_size); + + TEST_ASSERT(is_flds(insn_bytes, insn_size), + "Unexpected instruction. Expected 'flds' (0xd9 /0)"); + + /* + * If is_flds() succeeded then the instruction bytes contained an flds + * instruction that is 2-bytes in length (ie: no prefix, no SIB, no + * displacement). + */ + vcpu_regs_get(vcpu, ®s); + regs.rip += 2; + vcpu_regs_set(vcpu, ®s); } static void do_guest_assert(struct ucall *uc)
Explicitly require instruction bytes to be available in run->emulation_failure by asserting that they are present. Note that the test already requires the instruction bytes to be present because that's the only way the test will advance the RIP past the flds and get to GUEST_DONE(). Signed-off-by: David Matlack <dmatlack@google.com> --- .../smaller_maxphyaddr_emulation_test.c | 47 +++++++++---------- 1 file changed, 23 insertions(+), 24 deletions(-)