Re: [PATCH 09/16] KVM: x86/mmu: Move private vs. shared check above slot validity checks

From: "Huang, Kai" <kai.huang@intel.com>
To: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, <kvm@vger.kernel.org>,
	<linux-kernel@vger.kernel.org>, Yan Zhao <yan.y.zhao@intel.com>,
	"Isaku Yamahata" <isaku.yamahata@intel.com>,
	Michael Roth <michael.roth@amd.com>,
	Yu Zhang <yu.c.zhang@linux.intel.com>,
	Chao Peng <chao.p.peng@linux.intel.com>,
	Fuad Tabba <tabba@google.com>,
	David Matlack <dmatlack@google.com>
Subject: Re: [PATCH 09/16] KVM: x86/mmu: Move private vs. shared check above slot validity checks
Date: Thu, 7 Mar 2024 13:28:44 +1300	[thread overview]
Message-ID: <4af321d2-79bc-4245-b701-815be16e5ecc@intel.com> (raw)
In-Reply-To: <ZekBBxiackL1dRTg@google.com>

On 7/03/2024 12:49 pm, Sean Christopherson wrote:
> On Thu, Mar 07, 2024, Kai Huang wrote:
>>
>>
>> On 6/03/2024 3:02 pm, Sean Christopherson wrote:
>>> On Wed, Mar 06, 2024, Kai Huang wrote:
>>>>
>>>>
>>>> On 6/03/2024 1:38 pm, Sean Christopherson wrote:
>>>>> On Wed, Mar 06, 2024, Kai Huang wrote:
>>>>>>
>>>>>>
>>>>>> On 28/02/2024 3:41 pm, Sean Christopherson wrote:
>>>>>>> Prioritize private vs. shared gfn attribute checks above slot validity
>>>>>>> checks to ensure a consistent userspace ABI.  E.g. as is, KVM will exit to
>>>>>>> userspace if there is no memslot, but emulate accesses to the APIC access
>>>>>>> page even if the attributes mismatch.
>>>>>>
>>>>>> IMHO, it would be helpful to explicitly say that, in the later case (emulate
>>>>>> APIC access page) we still want to report MEMORY_FAULT error first (so that
>>>>>> userspace can have chance to fixup, IIUC) instead of emulating directly,
>>>>>> which will unlikely work.
>>>>>
>>>>> Hmm, it's not so much that emulating directly won't work, it's that KVM would be
>>>>> violating its ABI.  Emulating APIC accesses after userspace converted the APIC
>>>>> gfn to private would still work (I think), but KVM's ABI is that emulated MMIO
>>>>> is shared-only.
>>>>
>>>> But for (at least) TDX guest I recall we _CAN_ allow guest's MMIO to be
>>>> mapped as private, right?  The guest is supposed to get a #VE anyway?
>>>
>>> Not really.  KVM can't _map_ emulated MMIO as private memory, because S-EPT
>>> entries can only point at convertible memory.
>>
>> Right.  I was talking about the MMIO mapping in the guest, which can be
>> private I suppose.
>>
>>> KVM _could_ emulate in response to a !PRESENT EPT violation, but KVM is not
>>> going to do that.
>>>
>>> https://lore.kernel.org/all/ZcUO5sFEAIH68JIA@google.com
>>>
>>
>> Right agreed KVM shouldn't "emulate" !PRESENT fault.
> 
> One clarification.  KVM *does* emulate !PRESENT faults.  And that's not optional,
> as caching MMIO info in SPTEs is purely an optimization and isn't possible on all
> CPUs, e.g. AMD CPUs with MAXPHYADDR=52 don't have reserved bits to set.

Sorry I forgot to add "private".

> 
> My point above was specifically about emulating *private* !PRESENT faults as MMIO.
> 
>> I am talking about this -- for TDX guest, if I recall correctly, for guest's
>> MMIO gfn KVM still needs to get the EPT violation for the _first_ access, in
>> which KVM can configure the EPT entry to make sure "suppress #VE" bit is
>> cleared so the later accesses can trigger #VE directly.
> 
> That's totally fine, so long as the access is shared, not private.

OK as you already replied in the later patch.

> 
>> I suppose this is still the way we want to implement?
>>
>> I am afraid for this case, we will still see the MMIO GFN as private, while
>> by default I believe the "guest memory attributes" for that MMIO GFN should
>> be shared?
> 
> No, the guest should know it's (emulated) MMIO and access the gfn as shared.  That
> might generate a !PRESENT fault, but that's again a-ok.

Ditto.

> 
>> AFAICT, it seems the "guest memory attributes" code doesn't check whether a
>> GFN is MMIO or truly RAM.
> 
> That's userspace's responsibility, not KVM's responsibility.  And if userspace
> doesn't proactively make emulated MMIO regions shared, then the memory_fault exit
> that results from this patch will give userspace the hook it needs to convert the
> gfn to shared on-demand.

I mean whether it's better to just make kvm_mem_is_private() check 
whether the given GFN has slot, and always return "shared" if it doesn't.

In kvm_vm_set_mem_attributes() we also ignore NULL-slot GFNs.

(APIC access page is a special case that needs to handle.)

Allowing userspace to maintain whether MMIO GFN is shared or private 
doesn't make a lot sense because that doesn't help a lot due to the MMIO 
mapping is actually controlled by the guest kernel itself.

The (buggy) guest may still generate private !PRESNET faults, and KVM 
can still return to userspace with MEMORY_FAULT, but userspace can just 
kill the VM if the faulting address is MMIO.

But this will complicate the code so I guess may not worth to do..

Thanks for your time. :-)