Re: [PATCH v3 02/11] KVM: guest_memfd: Handle final folio_put() of guest_memfd pages

[Vlastimil Babka <vbabka@suse.cz>]

On 2/17/25 11:12, Fuad Tabba wrote:
> Hi Vlastimil,
> 
> On Mon, 17 Feb 2025 at 09:49, Vlastimil Babka <vbabka@suse.cz> wrote:
>>
>> On 2/11/25 13:11, Fuad Tabba wrote:
>> > Before transitioning a guest_memfd folio to unshared, thereby
>> > disallowing access by the host and allowing the hypervisor to
>> > transition its view of the guest page as private, we need to be
>> > sure that the host doesn't have any references to the folio.
>> >
>> > This patch introduces a new type for guest_memfd folios, which
>> > isn't activated in this series but is here as a placeholder and
>> > to facilitate the code in the next patch. This will be used in
>>
>> It's not clear to me how the code in the next page is facilitated as it
>> doesn't use any of this?
> 
> I'm sorry about that, I'm missing the word "series". i.e.,
> 
>> > This patch introduces a new type for guest_memfd folios, which
>> > isn't activated in this series but is here as a placeholder and
>> > to facilitate the code in the next patch *series*.
> 
> I'm referring to this series:
> https://lore.kernel.org/all/20250117163001.2326672-1-tabba@google.com/
> 
>> > the future to register a callback that informs the guest_memfd
>> > subsystem when the last reference is dropped, therefore knowing
>> > that the host doesn't have any remaining references.
>> >
>> > Signed-off-by: Fuad Tabba <tabba@google.com>
>> > ---
>> >  include/linux/kvm_host.h   |  9 +++++++++
>> >  include/linux/page-flags.h | 17 +++++++++++++++++
>> >  mm/debug.c                 |  1 +
>> >  mm/swap.c                  |  9 +++++++++
>> >  virt/kvm/guest_memfd.c     |  7 +++++++
>> >  5 files changed, 43 insertions(+)
>> >
>> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>> > index f34f4cfaa513..8b5f28f6efff 100644
>> > --- a/include/linux/kvm_host.h
>> > +++ b/include/linux/kvm_host.h
>> > @@ -2571,4 +2571,13 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
>> >                                   struct kvm_pre_fault_memory *range);
>> >  #endif
>> >
>> > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM
>> > +void kvm_gmem_handle_folio_put(struct folio *folio);
>> > +#else
>> > +static inline void kvm_gmem_handle_folio_put(struct folio *folio)
>> > +{
>> > +     WARN_ON_ONCE(1);
>> > +}
>>
>> Since the caller is guarded by CONFIG_KVM_GMEM_SHARED_MEM, do we need the
>> CONFIG_KVM_GMEM_SHARED_MEM=n variant at all?
> 
> No. I'll remove it.
> 
>> > +#endif
>> > +
>> >  #endif
>> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
>> > index 6dc2494bd002..734afda268ab 100644
>> > --- a/include/linux/page-flags.h
>> > +++ b/include/linux/page-flags.h
>> > @@ -933,6 +933,17 @@ enum pagetype {
>> >       PGTY_slab       = 0xf5,
>> >       PGTY_zsmalloc   = 0xf6,
>> >       PGTY_unaccepted = 0xf7,
>> > +     /*
>> > +      * guestmem folios are used to back VM memory as managed by guest_memfd.
>> > +      * Once the last reference is put, instead of freeing these folios back
>> > +      * to the page allocator, they are returned to guest_memfd.
>> > +      *
>> > +      * For now, guestmem will only be set on these folios as long as they
>> > +      * cannot be mapped to user space ("private state"), with the plan of
>>
>> Might be a bit misleading as I don't think it's set yet as of this series.
>> But I guess we can keep it to avoid another update later.
> 
> You're right, it's not in this series. But as you said, the idea is to
> have the least amount of churn in the core mm code.
> 
>> > +      * always setting that type once typed folios can be mapped to user
>> > +      * space cleanly.
>> > +      */
>> > +     PGTY_guestmem   = 0xf8,
>> >
>> >       PGTY_mapcount_underflow = 0xff
>> >  };
>> > @@ -1082,6 +1093,12 @@ FOLIO_TYPE_OPS(hugetlb, hugetlb)
>> >  FOLIO_TEST_FLAG_FALSE(hugetlb)
>> >  #endif
>> >
>> > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM
>> > +FOLIO_TYPE_OPS(guestmem, guestmem)
>> > +#else
>> > +FOLIO_TEST_FLAG_FALSE(guestmem)
>> > +#endif
>> > +
>> >  PAGE_TYPE_OPS(Zsmalloc, zsmalloc, zsmalloc)
>> >
>> >  /*
>> > diff --git a/mm/debug.c b/mm/debug.c
>> > index 8d2acf432385..08bc42c6cba8 100644
>> > --- a/mm/debug.c
>> > +++ b/mm/debug.c
>> > @@ -56,6 +56,7 @@ static const char *page_type_names[] = {
>> >       DEF_PAGETYPE_NAME(table),
>> >       DEF_PAGETYPE_NAME(buddy),
>> >       DEF_PAGETYPE_NAME(unaccepted),
>> > +     DEF_PAGETYPE_NAME(guestmem),
>> >  };
>> >
>> >  static const char *page_type_name(unsigned int page_type)
>> > diff --git a/mm/swap.c b/mm/swap.c
>> > index 47bc1bb919cc..241880a46358 100644
>> > --- a/mm/swap.c
>> > +++ b/mm/swap.c
>> > @@ -38,6 +38,10 @@
>> >  #include <linux/local_lock.h>
>> >  #include <linux/buffer_head.h>
>> >
>> > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM
>> > +#include <linux/kvm_host.h>
>> > +#endif
>>
>> Do we need to guard the include?
> 
> Yes, otherwise allnoconfig complains due to many of the x86 things it
> drags along if included but KVM isn't configured. I could put it in a
> different header that doesn't have this problem, but I couldn't think
> of a better header for this.

Ok, you can add
Acked-by: Vlastimil Babka <vbabka@suse.cz>