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?
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).
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?
-- Kind regards, Ivan Orlov