Re: [RFC PATCH v4 07/10] KVM: x86: Add gmem hook for initializing private memory

[Michael Roth]

On Fri, Jul 21, 2023 at 07:25:58AM -0700, Sean Christopherson wrote:
> On Thu, Jul 20, 2023, isaku.yamahata@xxxxxxxxx wrote:
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index a73ddb43a2cf..35bb14363828 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4352,6 +4352,7 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
> >  				   struct kvm_page_fault *fault)
> >  {
> >  	int max_order, r;
> > +	u8 max_level;
> >  
> >  	if (!kvm_slot_can_be_private(fault->slot))
> >  		return kvm_do_memory_fault_exit(vcpu, fault);
> > @@ -4361,8 +4362,15 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
> >  	if (r)
> >  		return r;
> >  
> > -	fault->max_level = min(kvm_max_level_for_order(max_order),
> > -			       fault->max_level);
> > +	max_level = kvm_max_level_for_order(max_order);
> > +	r = static_call(kvm_x86_gmem_prepare)(vcpu->kvm, fault->slot, fault->pfn,
> > +					      fault->gfn, &max_level);
> 
> I think KVM should hook gmem itself, i.e. convert pages to private when they're
> allocated, and then back to shared when they're freed.  I don't like having
> asymmetric APIs (convert on fault instead of allocate, but then convert back on
> free instead of on zap), and hooking the page fault path also violates the approach
> of tying the RMP/HKID to the physical allocation.

I'd originally added an arch hook in kvm_gmem_get_pfn() to handle
RMP/HKID when the folio is allocated:

  https://github.com/mdroth/linux/commit/adf78d224126f31e9096b80be21619e1ba447304

Based on your response it seemed like there was a preference to either:

  a) do the RMP/HKID update via KVM MMU prior to mapping into the guest
  b) implement an ioctl() to let userspace pre-allocate/pre-prepare and
     do the RMP updates in advance.

  https://lore.kernel.org/lkml/20230522135036.wnvsmryhkvstwvw2@xxxxxxx/

So this patch basically implements suggestion a), or at least my
understanding of it. Is the original patch that does it via
kvm_gmem_get_pfn() more in line with what you're suggesting here, or
were you still thinking of an ioctl? Or some other place in the code?
Keep in mind the GPA is needed to do the RMP updates so that prevents
us from hooking into the more obvious place like kvm_gmem_get_folio().

> 
> Practically speaking, hooking the fault path will result in undesirable behavior.
> Just because KVM *maps* at 4KiB granularity doesn't mean the RMP must be assigned
> at 4KiB granularity, e.g. if userspace chooses to *not* PUNCH_HOLE when the guest
> shares a single 4KiB page in a 2MiB chunk.   Dirty logging is another case where
> the RMP can stay at 2MiB.  Or as a very silly example, imagine userspace pulls a
> stupid and covers a single 2MiB chunk of gmem with 512 memslots.

Unfortunately I don't think things aren't quite that flexible with SNP. If
RMP entry is 2MB, and you map a sub-page as 4K in the NPT, you'll immediately
get a PFERR_GUEST_SIZEM on the first access (presumably when the guest tries
to PVALIDATE it before use). The RMP fault handler will then subsequently
need to PSMASH the 2MB entry into 4K before that guest can access it. So you
get an extra page fault for every 2MB page that's mapped this way.
(APM Volume 2, Section 15.36.10).

That might not be a big deal for guests that are at least somewhat optimized
to make use of 2MB pages, but another situation is:

  - gmem allocates 2MB page
  - guest issues PVALIDATE on 2MB page
  - guest later converts a subpage to shared but doesn't holepunch
  - SNP host code issues PSMASH to split 2MB RMP mapping to 4K
  - KVM MMU splits NPT mapping to 4K
  - guest converts that shared page back to private

At this point there are no mixed attributes, and KVM would normally
allow for 2MB NPT mappings again, but this is actually not allowed
because the RMP table mappings are validated/4K and cannot be promoted on
the hypervisor, so the NPT mappings must still be limited to 4K to
match this, otherwise we hit the reverse of the PFERR_GUEST_SIZEM
scenario above, where the NPT mapping level is *larger* than the RMP
entry level. Unfortunately that does not result in a PFERR_GUEST_SIZEM
where we can fix things up in response, but instead it's a general
RMP fault that would be tricky to distinguish from an RMP fault
resulting from an implicit page conversion or some other guest weirdness
without doing RMP table checks every time we get a general RMP fault.

So for all intents and purposes it does sort of end up being the case
that the mapping size and RMP entry size are sort of intertwined and
can't totally be decoupled, and if you don't take that into account
when updating the RMP entry, you'll have to do some extra PSMASH's
in response to PFERR_GUEST_SIZEM RMP faults later.

> 
> That likely means KVM will need an additional hook to clamp the max_level at the
> RMP, but that shouldn't be a big deal, e.g. if we convert on allocation, then KVM
> should be able to blindly do the conversion because it would be a kernel bug if
> the page is already assigned to an ASID in the RMP, i.e. the additional hook
> shouldn't incur an extra RMP lookup.

Yah we'd still need a hook in roughly this same place for clamping
max_level. Previous versions of SNP hypervisor patches all had a separate
hook for handling these cases, but since the work of updating the RMP table
prior to mapping isn't too dissimilar from the work of determining max
mapping size, I combined both of them into the kvm_x86_gmem_prepare()
hook/implementation.

But I don't see any major issue with moving RMPUPDATE handling to an
allocation-time hook. As mentioned above we'd get additional
PFERR_GUEST_SIZEM faults by not taking MMU mapping level into account, but
I previously had it implemented similarly via a hook in kvm_gmem_get_pfn()
(because we need the GPA) and didn't notice anything major. But I'm not
sure exactly where you're suggesting we do it now, so could use some
clarify on that if kvm_gmem_get_pfn() isn't what you had in mind.

WRT avoiding the RMP lookup, I'd tried __filemap_get_folio() without
FGP_CREAT flag as a way to check whether the folio was already allocated
or not, but it seemed to result in a lockup, and didn't look like it
would be any better performance-wise than just doing the RMP lookup
anyway (which is just a normal memory read), so that's the route I ended
up going.

-Mike