Hi Gavin,
On Fri, 24 Jan 2025 at 05:32, Gavin Shan <gshan@xxxxxxxxxx> wrote:
>
> Hi Fuad,
>
> On 1/18/25 2:29 AM, Fuad Tabba wrote:
> > From: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> >
> > Track whether guest_memfd memory can be mapped within the inode,
> > since it is property of the guest_memfd's memory contents.
> >
> > The guest_memfd PRIVATE memory attribute is not used for two
> > reasons. First because it reflects the userspace expectation for
> > that memory location, and therefore can be toggled by userspace.
> > The second is, although each guest_memfd file has a 1:1 binding
> > with a KVM instance, the plan is to allow multiple files per
> > inode, e.g. to allow intra-host migration to a new KVM instance,
> > without destroying guest_memfd.
> >
> > Signed-off-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> > Co-developed-by: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> > Signed-off-by: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> > Co-developed-by: Fuad Tabba <tabba@xxxxxxxxxx>
> > Signed-off-by: Fuad Tabba <tabba@xxxxxxxxxx>
> > ---
> > virt/kvm/guest_memfd.c | 56 ++++++++++++++++++++++++++++++++++++++----
> > 1 file changed, 51 insertions(+), 5 deletions(-)
> >
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 6453658d2650..0a7b6cf8bd8f 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -18,6 +18,17 @@ struct kvm_gmem {
> > struct list_head entry;
> > };
> >
> > +struct kvm_gmem_inode_private {
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > + struct xarray mappable_offsets;
> > +#endif
> > +};
> > +
> > +static struct kvm_gmem_inode_private *kvm_gmem_private(struct inode *inode)
> > +{
> > + return inode->i_mapping->i_private_data;
> > +}
> > +
> > /**
> > * folio_file_pfn - like folio_file_page, but return a pfn.
> > * @folio: The folio which contains this index.
> > @@ -312,8 +323,28 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
> > return gfn - slot->base_gfn + slot->gmem.pgoff;
> > }
> >
> > +static void kvm_gmem_evict_inode(struct inode *inode)
> > +{
> > + struct kvm_gmem_inode_private *private = kvm_gmem_private(inode);
> > +
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > + /*
> > + * .evict_inode can be called before private data is set up if there are
> > + * issues during inode creation.
> > + */
> > + if (private)
> > + xa_destroy(&private->mappable_offsets);
> > +#endif
> > +
> > + truncate_inode_pages_final(inode->i_mapping);
> > +
> > + kfree(private);
> > + clear_inode(inode);
> > +}
> > +
> > static const struct super_operations kvm_gmem_super_operations = {
> > - .statfs = simple_statfs,
> > + .statfs = simple_statfs,
> > + .evict_inode = kvm_gmem_evict_inode,
> > };
> >
>
> As I understood, ->destroy_inode() may be more suitable place where the xarray is
> released. ->evict_inode() usually detach the inode from the existing struct, to make
> it offline. ->destroy_inode() is actually the place where the associated resource
> (memory) is relased.
>
> Another benefit with ->destroy_inode() is we're not concerned to truncate_inode_pages_final()
> and clear_inode().
I see. I'll give this a try.
>
> > static int kvm_gmem_init_fs_context(struct fs_context *fc)
> > @@ -440,6 +471,7 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> > loff_t size, u64 flags)
> > {
> > const struct qstr qname = QSTR_INIT(name, strlen(name));
> > + struct kvm_gmem_inode_private *private;
> > struct inode *inode;
> > int err;
> >
> > @@ -448,10 +480,19 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> > return inode;
> >
> > err = security_inode_init_security_anon(inode, &qname, NULL);
> > - if (err) {
> > - iput(inode);
> > - return ERR_PTR(err);
> > - }
> > + if (err)
> > + goto out;
> > +
> > + err = -ENOMEM;
> > + private = kzalloc(sizeof(*private), GFP_KERNEL);
> > + if (!private)
> > + goto out;
> > +
> > +#ifdef CONFIG_KVM_GMEM_MAPPABLE
> > + xa_init(&private->mappable_offsets);
> > +#endif
> > +
> > + inode->i_mapping->i_private_data = private;
> >
>
> The whole block of code needs to be guarded by CONFIG_KVM_GMEM_MAPPABLE because
> kzalloc(sizeof(...)) is translated to kzalloc(0) when CONFIG_KVM_GMEM_MAPPABLE
> is disabled, and kzalloc() will always fail. It will lead to unusable guest-memfd
> if CONFIG_KVM_GMEM_MAPPABLE is disabled.
Good point, thanks for pointing this out.
Cheers,
/fuad
> > inode->i_private = (void *)(unsigned long)flags;
> > inode->i_op = &kvm_gmem_iops;
> > @@ -464,6 +505,11 @@ static struct inode *kvm_gmem_inode_make_secure_inode(const char *name,
> > WARN_ON_ONCE(!mapping_unevictable(inode->i_mapping));
> >
> > return inode;
> > +
> > +out:
> > + iput(inode);
> > +
> > + return ERR_PTR(err);
> > }
> >
> > static struct file *kvm_gmem_inode_create_getfile(void *priv, loff_t size,
>
> Thanks,
> Gavin
>
