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.
> As for optimizing xarray storage, assuming it's actually a bitmask, simply let
> KVM specify which bits to invert when storing/loading to/from the xarray so that
> KVM can optimize storage for the most common value (which is presumably
> KVM_GEM_SHARED on arm64?).
>
> If KVM_GMEM_SHARED is the desired "default", invert bit 0, otherwise dont. If
> for some reason we get to a state where the default value is multiple bits, the
> inversion trick still works. E.g. if KVM_GMEM_SHARED where a composite value,
> then invert bits 0 and 1. The polarity shenanigans should be easy to hide in two
> low level macros/helpers.
Ack.
> > +/*
> > + * Returns true if the folio is shared with the host and the guest.
>
> This is a superfluous comment. Simple predicates should be self-explanatory
> based on function name alone.
>
> > + *
> > + * Must be called with the offsets_lock lock held.
>
> Drop these types of comments and document through code, i.e. via lockdep
> assertions (which you already have).
Ack.
> > + */
> > +static bool kvm_gmem_offset_is_shared(struct inode *inode, pgoff_t index)
> > +{
> > + struct xarray *shared_offsets = &kvm_gmem_private(inode)->shared_offsets;
> > + rwlock_t *offsets_lock = &kvm_gmem_private(inode)->offsets_lock;
> > + unsigned long r;
> > +
> > + lockdep_assert_held(offsets_lock);
> >
> > - /* For now, VMs that support shared memory share all their memory. */
> > - return kvm_arch_gmem_supports_shared_mem(gmem->kvm);
> > + r = xa_to_value(xa_load(shared_offsets, index));
> > +
> > + return r == KVM_GMEM_ALL_SHARED;
> > +}
> > +
> > +/*
> > + * Returns true if the folio is shared with the guest (not transitioning).
> > + *
> > + * Must be called with the offsets_lock lock held.
>
> See above.
Ack.
> > static vm_fault_t kvm_gmem_fault(struct vm_fault *vmf)
>
> This should be something like kvm_gmem_fault_shared() make it abundantly clear
> what's being done. Because it too me a few looks to realize this is faulting
> memory into host userspace, not into the guest.
Ack.
Thanks!
/fuad
