Message-ID: <20231002155330.lguyhqgy64rhko4p@amd.com>
Date:   Mon, 2 Oct 2023 10:53:30 -0500
From:   Michael Roth <michael.roth@....com>
To:     Sean Christopherson <seanjc@...gle.com>
CC:     Paolo Bonzini <pbonzini@...hat.com>, <kvm@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        Binbin Wu <binbin.wu@...ux.intel.com>
Subject: Re: [PATCH 06/13] KVM: Disallow hugepages for incompatible gmem
 bindings, but let 'em succeed

On Thu, Sep 28, 2023 at 11:31:51AM -0700, Sean Christopherson wrote:
> On Fri, Sep 22, 2023, Michael Roth wrote:
> > On Thu, Sep 21, 2023 at 01:33:23PM -0700, Sean Christopherson wrote:
> > > +	/*
> > > +	 * For simplicity, allow mapping a hugepage if and only if the entire
> > > +	 * binding is compatible, i.e. don't bother supporting mapping interior
> > > +	 * sub-ranges with hugepages (unless userspace comes up with a *really*
> > > +	 * strong use case for needing hugepages within unaligned bindings).
> > > +	 */
> > > +	if (!IS_ALIGNED(slot->gmem.pgoff, 1ull << *max_order) ||
> > > +	    !IS_ALIGNED(slot->npages, 1ull << *max_order))
> > > +		*max_order = 0;
> > 
> > Thanks for working this in. Unfortunately on x86 the bulk of guest memory
> > ends up getting slotted directly above legacy regions at GFN 0x100, 
> 
> Can you provide an example?  I'm struggling to understand what the layout actually
> is.  I don't think it changes the story for the kernel, but it sounds like there
> might be room for optimization in QEMU?  Or more likely, I just don't understand
> what you're saying :-)

Here's one example, which seems to be fairly normal for an x86 boot:

  kvm_set_user_memory AddrSpace#0 Slot#0 flags=0x4 gpa=0x0 size=0x80000000 ua=0x7f24afc00000 ret=0 restricted_fd=19 restricted_offset=0x0
  ^ QEMU creates Slot 0 for all of main guest RAM
  kvm_set_user_memory AddrSpace#0 Slot#0 flags=0x0 gpa=0x0 size=0x0 ua=0x7f24afc00000 ret=0 restricted_fd=19 restricted_offset=0x0
  kvm_set_user_memory AddrSpace#0 Slot#0 flags=0x4 gpa=0x0 size=0xc0000 ua=0x7f24afc00000 ret=0 restricted_fd=19 restricted_offset=0x0
  kvm_set_user_memory AddrSpace#0 Slot#3 flags=0x6 gpa=0xc0000 size=0x20000 ua=0x7f2575000000 ret=0 restricted_fd=33 restricted_offset=0x0
  kvm_set_user_memory AddrSpace#0 Slot#4 flags=0x6 gpa=0xe0000 size=0x20000 ua=0x7f2575400000 ret=0 restricted_fd=31 restricted_offset=0x0
  ^ legacy regions are created and mapped on top of GPA ranges [0xc0000:0xe0000) and [0xe0000:0x100000)
  kvm_set_user_memory AddrSpace#0 Slot#5 flags=0x4 gpa=0x100000 size=0x7ff00000 ua=0x7f24afd00000 ret=0 restricted_fd=19 restricted_offset=0x100000
  ^ QEMU divides Slot 0 into Slot 0 at [0x0:0xc0000) and Slot 5 at [0x100000:0x80000000)
    Both Slots still share the same backing memory allocation, so same gmem
    fd 19 is used,but Slot 5 is assigned to offset 0x100000, whih is not
    2M-aligned

I tried messing with QEMU handling to pad out guest_memfd offsets to 2MB
boundaries but then the inode size needs to be enlarged to account for it
and things get a bit messy. Not sure if there are alternative approaches
that can be taken from userspace, but with normal malloc()'d or mmap()'d
backing memory the kernel can still allocate a 2MB backing page for the
[0x0:0x200000) range and I think KVM still handles that when setting up
NPT of sub-ranges so there might not be much room for further optimization
there.

> 
> > so the associated slot still ends failing these alignment checks if it tries
> > to match the gmemfd offsets up with the shared RAM/memfd offsets.
> > 
> > I tried to work around it in userspace by padding the gmemfd offset of
> > each slot to the next 2M boundary, but that also requires dynamically
> > growing the gmemfd inode to account for the padding of each new slot and
> > it gets ugly enough that I'm not sure it's any better than your
> > suggested alternative of using a unique gmemfd for each slot.
> > 
> > But what if we relax the check to simply make sure that any large folio
> > must is fully-contained by the range of the slot is bound to? It *seems*
> > like that would still avoid stuff like mapping 2M pages in the NPT (or
> > setting up 2M RMP table entries) that aren't fully contained by a slot
> > while still allowing the bulk of guest memory to get mapped as 2M. Are
> > there other edge cases to consider?
> > 
> > The following seems to work for a basic 16GB SNP guest at least:
> > 
> > diff --git a/virt/kvm/guest_mem.c b/virt/kvm/guest_mem.c
> > index 9109bf5751ee..e73128d4ebc2 100644
> > --- a/virt/kvm/guest_mem.c
> > +++ b/virt/kvm/guest_mem.c
> > @@ -618,6 +618,7 @@ int __kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> >                        gfn_t gfn, kvm_pfn_t *pfn, int *max_order, bool prep)
> >  {
> >         pgoff_t index = gfn - slot->base_gfn + slot->gmem.pgoff;
> > +       pgoff_t huge_index;
> >         struct kvm_gmem *gmem;
> >         struct folio *folio;
> >         struct page *page;
> > @@ -662,9 +663,12 @@ int __kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> >          * sub-ranges with hugepages (unless userspace comes up with a *really*
> >          * strong use case for needing hugepages within unaligned bindings).
> >          */
> > -       if (!IS_ALIGNED(slot->gmem.pgoff, 1ull << *max_order) ||
> > -           !IS_ALIGNED(slot->npages, 1ull << *max_order))
> > +       huge_index = round_down(index, 1ull << *max_order);
> 
> Why not use ALIGN() here?  The size is obviously a power-of-2.  Or is my math
> even worse than I thought?

I actually only originally used round_down() because kvm_gmem_get_huge_folio()
was taking that approach, but I still ended up using ALIGN() below so sorry
if the inconsistency caused any confusion. I switched to using ALIGN() above
and it works fine.

> 
> > +       if (huge_index < ALIGN(slot->gmem.pgoff, 1ull << *max_order) ||
> > +           huge_index + (1ull << *max_order) > slot->gmem.pgoff + slot->npages) {
> 
> Argh, I keep forgetting that the MMU is responsible for handling misaligned gfns.
> Yeah, this looks right.
> 
> Can you post this as a proper patch, on top of my fixes?  And without the pr_debug().
> That'll be the easiest for me to apply+squash when the time comes.

Sure, I submitted a revised patch on top of kvm-x86:

  https://lore.kernel.org/lkml/20231002133342.195882-1-michael.roth@amd.com/T/#u

I ran into a separate issue trying to test it and submitted a patch for that
here:

  https://lore.kernel.org/lkml/20231002133230.195738-1-michael.roth@amd.com/T/#u

-Mike

> 
> Thanks much!

Powered by blists - more mailing lists