On Thu, Apr 03, 2025, Fuad Tabba wrote:
> Hi Sean,
>
> On Thu, 3 Apr 2025 at 01:19, Sean Christopherson <seanjc@xxxxxxxxxx> 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
