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

From: Huang, Kai
Date: Wed Mar 06 2024 - 19:29:26 EST



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@xxxxxxxxxx


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