On Tue, Sep 27, 2022, Emanuele Giuseppe Esposito wrote: > > Am 26/09/2022 um 23:28 schrieb Sean Christopherson: > > On Mon, Sep 26, 2022, David Hildenbrand wrote: > >> As Sean said "This is an awful lot of a complexity to take on for something > >> that appears to be solvable in userspace." > > > > And if the userspace solution is unpalatable for whatever reason, I'd like to > > understand exactly what KVM behavior is problematic for userspace. E.g. the > > above RHBZ bug should no longer be an issue as the buggy commit has since been > > reverted. > > It still is because I can reproduce the bug, as also pointed out in > multiple comments below. You can reproduce _a_ bug, but it's obviously not the original bug, because the last comment says: Second, indeed the patch was reverted and somehow accepted without generating too much noise: ... The underlying issue of course as we both know is still there. You might have luck reproducing it with this bug https://bugzilla.redhat.com/show_bug.cgi?id=1855298 But for me it looks like it is 'working' as well, so you might have to write a unit test to trigger the issue. > > If the issue is KVM doing something nonsensical on a code fetch to MMIO, then I'd > > much rather fix _that_ bug and improve KVM's user exit ABI to let userspace handle > > the race _if_ userspace chooses not to pause vCPUs. > > > > Also on the BZ they all seem (Paolo included) to agree that the issue is > non-atomic memslots update. Yes, non-atomic memslot likely results in the guest fetching from a GPA without a memslot. I'm asking for an explanation of exactly what happens when that occurs, because it should be possible to adjust KVM and/or QEMU to play nice with the fetch, e.g. to resume the guest until the new memslot is installed, in which case an atomic update isn't needed. I assume the issue is that KVM exits with KVM_EXIT_INTERNAL_ERROR because the guest is running at CPL=0, and QEMU kills the guest in response. If that's correct, then that problem can be solved by exiting to userspace with KVM_EXIT_MMIO instead of KVM_EXIT_INTERNAL_ERROR so that userspace can do something sane in response to the MMIO code fetch. I'm pretty sure this patch will Just Work for QEMU, because QEMU simply resumes the vCPU if mmio.len==0. It's a bit of a hack, but I don't think it violates KVM's ABI in any way, and it can even become "official" behavior since KVM x86 doesn't otherwise exit with mmio.len==0. Compile tested only... --- From: Sean Christopherson <seanjc@xxxxxxxxxx> Date: Tue, 27 Sep 2022 08:16:03 -0700 Subject: [PATCH] KVM: x86: Exit to userspace with zero-length MMIO "read" on MMIO fetch Exit to userspace with KVM_EXIT_MMIO if emulation fails due to not being able to fetch instruction bytes, e.g. if the resolved GPA isn't backed by a memslot. If userspace is manipulating memslots without pausing vCPUs, e.g. to emulate BIOS relocation, then a vCPU may fetch while there is no valid memslot installed. Depending on guest context, KVM will either exit to userspace with KVM_EXIT_INTERNAL_ERROR (L1, CPL=0) or simply resume the guest (L2 or CPL>0), neither of which is desirable as exiting with "emulation error" effectively kills the VM, and resuming the guest doesn't provide userspace an opportunity to react the to fetch. Use "mmio.len == 0" to indicate "fetch". This is a bit of a hack, but there is no other way to communicate "fetch" to userspace without defining an entirely new exit reason, e.g. "mmio.is_write" is a boolean and not a flag, and there is no known use case for actually supporting code fetches from MMIO, i.e. there's no need to allow userspace to fill in the instruction bytes. Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx> --- arch/x86/kvm/emulate.c | 2 ++ arch/x86/kvm/kvm_emulate.h | 1 + arch/x86/kvm/x86.c | 9 ++++++++- 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c index f092c54d1a2f..e141238d93b0 100644 --- a/arch/x86/kvm/emulate.c +++ b/arch/x86/kvm/emulate.c @@ -5353,6 +5353,8 @@ int x86_decode_insn(struct x86_emulate_ctxt *ctxt, void *insn, int insn_len, int done: if (rc == X86EMUL_PROPAGATE_FAULT) ctxt->have_exception = true; + if (rc == X86EMUL_IO_NEEDED) + return EMULATION_IO_FETCH; return (rc != X86EMUL_CONTINUE) ? EMULATION_FAILED : EMULATION_OK; } diff --git a/arch/x86/kvm/kvm_emulate.h b/arch/x86/kvm/kvm_emulate.h index 89246446d6aa..3cb2e321fcd2 100644 --- a/arch/x86/kvm/kvm_emulate.h +++ b/arch/x86/kvm/kvm_emulate.h @@ -516,6 +516,7 @@ bool x86_page_table_writing_insn(struct x86_emulate_ctxt *ctxt); #define EMULATION_OK 0 #define EMULATION_RESTART 1 #define EMULATION_INTERCEPTED 2 +#define EMULATION_IO_FETCH 3 void init_decode_cache(struct x86_emulate_ctxt *ctxt); int x86_emulate_insn(struct x86_emulate_ctxt *ctxt); int emulator_task_switch(struct x86_emulate_ctxt *ctxt, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index aa5ab0c620de..7eb72694c601 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7129,8 +7129,13 @@ static int kvm_fetch_guest_virt(struct x86_emulate_ctxt *ctxt, bytes = (unsigned)PAGE_SIZE - offset; ret = kvm_vcpu_read_guest_page(vcpu, gpa >> PAGE_SHIFT, val, offset, bytes); - if (unlikely(ret < 0)) + if (unlikely(ret < 0)) { + vcpu->run->mmio.phys_addr = gpa; + vcpu->run->mmio.len = 0; + vcpu->run->mmio.is_write = 0; + vcpu->run->exit_reason = KVM_EXIT_MMIO; return X86EMUL_IO_NEEDED; + } return X86EMUL_CONTINUE; } @@ -8665,6 +8670,8 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, r = x86_decode_emulated_instruction(vcpu, emulation_type, insn, insn_len); if (r != EMULATION_OK) { + if (r == EMULATION_IO_FETCH) + return 0; if ((emulation_type & EMULTYPE_TRAP_UD) || (emulation_type & EMULTYPE_TRAP_UD_FORCED)) { kvm_queue_exception(vcpu, UD_VECTOR); base-commit: 39d9b48cc777bdf6d67d01ed24f1f89b13f5fbb2 --
