On Thu, Dec 12, 2024, Ivan Orlov wrote:
> On Wed, Dec 11, 2024 at 05:01:07PM -0800, Sean Christopherson wrote:
> > > Hm, by the way, what is the desired behaviour if EMULTYPE_ALLOW_RETRY_PF is
> > > set? Is it correct that we return an internal error if it is set during
> > > vectoring? Or KVM may try to unprotect the page and re-execute?
> >
> > Heh, it's sneaky, but EMULTYPE_ALLOW_RETRY_PF can be set if and only if
> > RET_PF_WRITE_PROTECTED is set. Hmm, that makes me think we should do the below
> > (EMULTYPE_WRITE_PF_TO_SP was a recent addition).
> >
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 2e713480933a..de5f6985d123 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9077,7 +9077,7 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
> >
> > if ((emulation_type & EMULTYPE_ALLOW_RETRY_PF) &&
> > (WARN_ON_ONCE(is_guest_mode(vcpu)) ||
> > - WARN_ON_ONCE(!(emulation_type & EMULTYPE_PF))))
> > + WARN_ON_ONCE(!(emulation_type & EMULTYPE_WRITE_PF_TO_SP))))
> > emulation_type &= ~EMULTYPE_ALLOW_RETRY_PF;
> >
>
> What if we are handling a write to write-protected page, but not a write to
> the page table? We still can retry after unprotecting the page, so
> EMULTYPE_ALLOW_RETRY_PF should be enabled, right?
Gah, I got my enums mixed up. I conflated RET_PF_WRITE_PROTECTED with
EMULTYPE_WRITE_PF_TO_SP. Ignore the above.
FWIW, KVM _can't_ unprotect and retry in the EMULTYPE_WRITE_PF_TO_SP case. From
kvm_unprotect_and_retry_on_failure():
/*
* If the failed instruction faulted on an access to page tables that
* are used to translate any part of the instruction, KVM can't resolve
* the issue by unprotecting the gfn, as zapping the shadow page will
* result in the instruction taking a !PRESENT page fault and thus put
* the vCPU into an infinite loop of page faults. E.g. KVM will create
* a SPTE and write-protect the gfn to resolve the !PRESENT fault, and
* then zap the SPTE to unprotect the gfn, and then do it all over
* again. Report the error to userspace.
*/
if (emulation_type & EMULTYPE_WRITE_PF_TO_SP)
return false;
> > r = kvm_check_emulate_insn(vcpu, emulation_type, insn, insn_len);
> >
> > That said, let me get back to you on this when my brain is less tired. I'm not
> > sure emulating when an exit occurred during event delivery is _ever_ correct.
> >
>
> I believe we can re-execute the instruction if exit happened during vectoring
> due to exception (and if the address is not MMIO, e.g. when it's a write to
> write-protected page, for instance when stack points to it).
Unprotect and re-execute is fine, what I'm worried about is *successfully*
emulating the instruction. E.g.
1. CPU executes instruction X and hits a #GP.
2. While vectoring the #GP, a shadow #PF is taken.
3. On VM-Exit, KVM re-injects the #GP (see __vmx_complete_interrupts()).
4. KVM emulates because of the write-protected page.
5. KVM "successfully" emulates and also detects the #GP
6. KVM synthesizes a #GP, and because the vCPU already has injected #GP,
incorrectly escalates to a #DF.
The above is a bit contrived, but I think it could happen if the guest reused a
page that _was_ a page table, for a vCPU's kernel stack.
> KVM unprotects the page, executes the instruction one more time and
> (probably) gets this exception once again (but the page is already
> unprotected, so vectoring succeeds without vmexit). If not
> (e.g. exception conditions are not met anymore), guest shouldn't really
> care and it can continue execution.
>
> However, I'm not sure what happens if vectoring is caused by external
> interrupt: if we unprotect the page and re-execute the instruction,
> will IRQ be delivered nonetheless, or it will be lost as irq is
> already in ISR? Do we need to re-inject it in such a case?
In all cases, the event that was being vectored is re-injected. Restarting from
scratch would be a bug. E.g. if the cause of initial exception was "fixed", say
because the initial exception was #BP, and the guest finished patching out the INT3,
then restarting would execute the _new_ instruction, and the INT3 would be lost.
In most cases, the guest would never notice, but it's still undesriable for KVM
to effectively rewrite history.
As far as unprotect+retry being viable, I think we're on the same page. What I'm
getting at is that I think KVM should never allow emulating on #PF when the #PF
occurred while vectoring. E.g. this:
static inline bool kvm_can_emulate_event_vectoring(int emul_type)
{
return !(emul_type & EMULTYPE_PF);
}
and then I believe this? Where this diff can be a separate prep patch (though I'm
pretty sure it's technically pointless without the vectoring angle, because shadow
#PF can't coincide with any of the failure paths for kvm_check_emulate_insn()).
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 07c6f1d5323d..63361b2da450 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9107,6 +9107,10 @@ int x86_emulate_instruction(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
if (r == X86EMUL_RETRY_INSTR || r == X86EMUL_PROPAGATE_FAULT)
return 1;
+ if (kvm_unprotect_and_retry_on_failure(vcpu, cr2_or_gpa,
+ emulation_type))
+ return 1;
+
if (r == X86EMUL_UNHANDLEABLE_VECTORING_IO) {
kvm_prepare_event_vectoring_exit(vcpu, cr2_or_gpa);
return 0;
