Message-ID: <2b954265-9ec0-4ee6-99c8-6ac080687d02@gmail.com>
Date: Wed, 16 Oct 2024 23:53:50 +0100
From: Ivan Orlov <ivan.orlov0322@...il.com>
To: Sean Christopherson <seanjc@...gle.com>, Ivan Orlov <iorlov@...zon.com>
Cc: bp@...en8.de, dave.hansen@...ux.intel.com, mingo@...hat.com,
 pbonzini@...hat.com, shuah@...nel.org, tglx@...utronix.de, hpa@...or.com,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, x86@...nel.org, jalliste@...zon.com,
 nh-open-source@...zon.com, pdurrant@...zon.co.uk
Subject: Re: [PATCH 2/3] KVM: vmx, svm, mmu: Process MMIO during event
 delivery

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?

> 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...

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!

>>   
>> -	/*
>> -	 * Note:
>> -	 * Do not try to fix EXIT_REASON_EPT_MISCONFIG if it caused by
>> -	 * delivery event since it indicates guest is accessing MMIO.
>> -	 * The vm-exit can be triggered again after return to guest that
>> -	 * will cause infinite loop.
>> -	 */
>>   	if ((vectoring_info & VECTORING_INFO_VALID_MASK) &&
>>   	    (exit_reason.basic != EXIT_REASON_EXCEPTION_NMI &&
>>   	     exit_reason.basic != EXIT_REASON_EPT_VIOLATION &&
>>   	     exit_reason.basic != EXIT_REASON_PML_FULL &&
>>   	     exit_reason.basic != EXIT_REASON_APIC_ACCESS &&
>>   	     exit_reason.basic != EXIT_REASON_TASK_SWITCH &&
>> -	     exit_reason.basic != EXIT_REASON_NOTIFY)) {
>> +	     exit_reason.basic != EXIT_REASON_NOTIFY &&
>> +	     exit_reason.basic != EXIT_REASON_EPT_MISCONFIG)) {
> 
> Changing the behavior of EPT_MISCONFIG belongs in a separate commit.
> 

I will extract the vmx-specific changes into separate commit in V2, thanks!

> Huh, and that's technically a bug fix.  If userspace _creates_ a memslot, KVM
> doesn't eagerly zap MMIO SPTEs and instead relies on vcpu_match_mmio_gen() to
> force kvm_mmu_page_fault() down the actual page fault path.  If the guest somehow
> manages to generate an access to the new page while vectoring an event, KVM will
> spuriously exit to userspace instead of trying to fault-in the new page.
> 
> It's _ridiculously_ contrived, but technically a bug.
> 

That's amazing, I finally introduced an unintentional bugfix (usually 
it's other way around) :D

> 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.

>>   		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)?

Thanks a lot for the review.

-- 
Kind regards,
Ivan Orlov

Powered by blists - more mailing lists