Re: [PATCH 2/3] KVM: vmx, svm, mmu: Process MMIO during event delivery

[Sean Christopherson <seanjc@google.com>]

On Wed, Oct 16, 2024, Ivan Orlov wrote:
> On 10/12/24 01:05, Sean Christopherson wrote:
> > 
> > > +			 * without VMM intervention, so return a corresponding internal error
> > > +			 * instead (otherwise, vCPU will fall into infinite loop trying to
> > > +			 * deliver the event again and again).
> > > +			 */
> > > +			if (error_code & PFERR_EVT_DELIVERY) {
> > 
> > Hmm, I'm 99% certain handling error in this location is wrong, and I'm also pretty
> > sure it's unnecessary.  Or rather, the synthetic error code is unnecessary.
> > 
> > It's wrong because this path specifically handles "cached" MMIO, i.e. emulated
> > MMIO that is triggered by a special MMIO SPTE.  KVM should punt to userspace on
> > _any_ MMIO emulation.  KVM has gotten away with the flaw because SVM is completely
> > broken, and VMX can always generate reserved EPTEs.  But with SVM, on CPUs with
> > MAXPHYADDR=52, KVM can't generate a reserved #PF, i.e. can't do cached MMIO, and
> > so I'm pretty sure your test would fail on those CPUs since they'll never come
> > down this path.
> > 
> 
> Ah, alright, I see... Probably, I need to test the next version with
> enable_mmio_caching=false as well.
> 
> > Heh, though I bet the introduction of RET_PF_WRITE_PROTECTED has regressed
> > shadow paging on CPUs with PA52.
> 
> Is it because it doesn't process write-protected gfn correctly if it is in
> MMIO range when mmio caching is disabled?

Ignore this, I was thinking lack of cached MMIO SPTEs would result in no
EPT Misconfig, but it's shadow paging, there is no EPT.

> > Anyways, the synthetic PFERR flag is unnecessary because the information is readily
> > available to {vmx,svm}_check_emulate_instruction().  Ha!  And EMULTYPE_WRITE_PF_TO_SP
> > means vendor code can even precisely identify MMIO.
> 
> Hmm, do you mean EMULTYPE_PF? It looks like EMULTYPE_WRITE_PF_TO_SP has
> nothing to do with MMIO...

Nope.  Well, both.  EMULTYPE_PF is set if *any* page fault triggers emulation.
EMULTYPE_WRITE_PF_TO_SP is set if emulation was triggered by a write protection
violation due to write-tracking, and write-tracking requires an underlying memslot.
I.e. if EMULTYPE_WRITE_PF_TO_SP is set, then emulation *can't* be for emulated
MMIO.

> I thought about processing the error in check_emulate_instruction as it
> seems logical, however I hadn't found a way to detect MMIO without page
> walking on SVM. I'll validate that EMULTYPE_PF gets set in all of the MMIO
> cases and move the handling into this function in V2 if it works.
> 
> > I think another X86EMUL_* return type is needed, but that's better than a synthetic
> > #PF error code flag.
> > 
> 
> If I understand correctly, you suggest returning this new
> X86EMUL_<something> code from {svm,vmx}_check_emulate_instruction and
> process it in the common code, right? I agree that it's much better than
> handling the error in MMU code. We are gonna return this return type from
> vendor code and handle it in the common code this way, which is neat!

Yep, exactly.

> > Ugh, and the manual call to vmx_check_emulate_instruction() in handle_ept_misconfig()
> > is similarly flawed, though encountering that is even more contrived as that only
> > affects accesses from SGX enclaves.
> > 
> > Hmm, and looking at all of this, SVM doesn't take advantage of KVM_FAST_MMIO_BUS.
> > Unless I'm forgetting some fundamental incompatibility, SVM can do fast MMIO so
> > long as next_rip is valid.
> > 
> > Anyways, no need to deal with vmx_check_emulate_instruction() or fast MMIO, I'll
> > tackle that in a separate series.  But for this series, please do the EPT misconfig
> > in a separate patch from fixing SVM.  E.g. extract the helper, convert VMX to the
> > new flow, and then teach SVM to do the same.
> > 
> 
> Hmm, implementing KVM_FAST_MMIO_BUS for SVM sounds like an interesting thing
> to do, please let me know if I could help. By the way, why can't we move the
> call to kvm_io_bus_write into the common code (e.g. MMU)? It would remove
> the need of implementing KVM_FAST_MMIO_BUS specifically for each vendor.

That would work too, but vendor code needs to be aware of "fast" MMIO no matter
what, because there are vendor specific conditions that make fast MMIO impossible
(KVM needs the CPU to provide the instruction length, otherwise KVM needs to
emulate the instruction in order to decode it, which makes fast MMIO not-fast).

> > >   		gpa_t gpa = vmcs_read64(GUEST_PHYSICAL_ADDRESS);
> > > -		bool is_mmio = exit_reason.basic == EXIT_REASON_EPT_MISCONFIG;
> > > -
> > 
> > Blank newline after variable declarations.
> > 
> > > -		kvm_prepare_ev_delivery_failure_exit(vcpu, gpa, is_mmio);
> > > +		kvm_prepare_ev_delivery_failure_exit(vcpu, gpa, false);
> > >   		return 0;
> > >   	}
> > 
> > All in all, I think this is the basic gist?  Definitely feel free to propose a
> > better name than X86EMUL_UNHANDLEABLE_VECTORING.
> > 
> 
> It sounds OK, but maybe something more precise would work, like
> X86EMUL_VECTORING_IO_NEEDED (by analogy with X86EMUL_IO_NEEDED)?

Hmm, I definitely want X86EMUL_UNHANDLEABLE_XXX, so that it's super clear that
emulation failed.  Maybe X86EMUL_UNHANDLEABLE_IO_VECTORING?  Sadly, I think we
need VECTORING (or similar) in the name, because it will be morphed to
KVM_INTERNAL_ERROR_DELIVERY_EV.  E.g. simply X86EMUL_UNHANDLEABLE_IO would be
nice, but is wildly misleading as there are other scenarios where KVM can't
handled emulated MMIO during instruction emulation.  :-/