Re: [PATCH v7 4/7] KVM: guest_memfd: Folio sharing states and functions that manage their transition

[Sean Christopherson <seanjc@google.com>]

On Thu, Apr 03, 2025, Fuad Tabba wrote:
> Hi Sean,
> 
> On Thu, 3 Apr 2025 at 01:19, Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Mar 28, 2025, Fuad Tabba wrote:
> > > @@ -389,22 +381,211 @@ static void kvm_gmem_init_mount(void)
> > >  }
> > >
> > >  #ifdef CONFIG_KVM_GMEM_SHARED_MEM
> > > -static bool kvm_gmem_offset_is_shared(struct file *file, pgoff_t index)
> > > +/*
> > > + * An enum of the valid folio sharing states:
> > > + * Bit 0: set if not shared with the guest (guest cannot fault it in)
> > > + * Bit 1: set if not shared with the host (host cannot fault it in)
> > > + */
> > > +enum folio_shareability {
> > > +     KVM_GMEM_ALL_SHARED     = 0b00, /* Shared with the host and the guest. */
> > > +     KVM_GMEM_GUEST_SHARED   = 0b10, /* Shared only with the guest. */
> > > +     KVM_GMEM_NONE_SHARED    = 0b11, /* Not shared, transient state. */
> >
> > Absolutely not.  The proper way to define bitmasks is to use BIT(xxx).  Based on
> > past discussions, I suspect you went this route so that the most common value
> > is '0' to avoid extra, but that should be an implementation detail buried deep
> > in the low level xarray handling, not a
> >
> > The name is also bizarre and confusing.  To map memory into the guest as private,
> > it needs to be in KVM_GMEM_GUEST_SHARED.  That's completely unworkable.
> > Of course, it's not at all obvious that you're actually trying to create a bitmask.
> > The above looks like an inverted bitmask, but then it's used as if the values don't
> > matter.
> >
> >         return (r == KVM_GMEM_ALL_SHARED || r == KVM_GMEM_GUEST_SHARED);
> 
> Ack.
> 
> > Given that I can't think of a sane use case for allowing guest_memfd to be mapped
> > into the host but not the guest (modulo temporary demand paging scenarios), I
> > think all we need is:
> >
> >         KVM_GMEM_SHARED           = BIT(0),
> >         KVM_GMEM_INVALID          = BIT(1),
> 
> We need the third state for the transient case, i.e., when a page is
> transitioning from being shared with the host to going back to
> private, in order to ensure that neither the guest nor the host can
> install a mapping/fault it in. But I see your point.

That's KVM_GMEM_INVALID.  Translating to what you had:

  KVM_GMEM_ALL_SHARED   = KVM_GMEM_SHARED
  KVM_GMEM_GUEST_SHARED = 0
  KVM_GMEM_NONE_SHARED  = KVM_GMEM_INVALID