Re: [PATCH v3 2/2] selftests: kvm: Allows userspace to handle emulation errors.

[Aaron Lewis <aaronlewis@google.com>]

> > +static void process_exit_on_emulation_error(struct kvm_vm *vm)
> > +{
> > +       struct kvm_run *run = vcpu_state(vm, VCPU_ID);
> > +       struct kvm_regs regs;
> > +       uint8_t *insn_bytes;
> > +       uint8_t insn_size;
> > +       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);
> > +
> > +       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), encountered (0x%x /%u)",
> > +                                   insn_bytes[0], (insn_size >= 2) ? GET_REG(insn_bytes[1]) : 0);
>
> If you don't get 'flds', you shouldn't assume that the second byte is
> the modr/m byte. Even if it is, the reg field may not be part of the
> opcode.
>
> > +                       vcpu_regs_get(vm, VCPU_ID, &regs);
> > +                       regs.rip += (uintptr_t)(&fld_end) - (uintptr_t)(&fld_start);
>
> A general purpose hypervisor wouldn't normally have access to these
> labels, so you should really determine the length of the instruction
> by decoding, *and* ensure that kvm gave you sufficient instruction
> bytes. For instance, if the addressing mode involves a SIB byte and a
> 32-bit displacement, you would need kvm to give you at least 7 bytes.
> Speaking of sufficient bytes, it would be nice to see your test
> exercise the case where kvm's in-kernel emulator can't actually fetch
> the full 15 bytes.
>
> Can you comment on what else would have to be done to actually emulate
> this instruction in userspace?
>

Along with doing a more thorough job decoding this instruction we
should really have a way to convert the effective address to a linear
address.  This can be tricky to do, so I'd suggest letting the kernel
do it (because it already knows how).  I could create an ioctl that
resolves the effective address and returns either the corresponding
linear address or an exception in the event it fails to linearize the
address.  In the case we have here in this test I'd expect an
exception in the form of a #PF(RSVD).  I was thinking this could be
done as a followup to this change.

> > +                       vcpu_regs_set(vm, VCPU_ID, &regs);
> > +               }
> > +       }
> > +}
> > +
> > +static void do_guest_assert(struct kvm_vm *vm, struct ucall *uc)
> > +{
> > +       TEST_FAIL("%s at %s:%ld", (const char *)uc->args[0], __FILE__,
> > +                 uc->args[1]);
> > +}
> > +