Fuad Tabba <tabba@xxxxxxxxxx> writes:
> 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.
>
While working on 1G page support (old revision at [1]), I was looking at
this.
Using .destroy_inode to clean up private->mappable_offsets should work
fine, and I agree this should be refactored to use .destroy_inode
instead. Thanks for pointing this out!
FWIW, for 1G page support, the truncation process has to be overridden
to, so the .evict_inode override will have to come back.
>>
>> > 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
>>
[1] https://lore.kernel.org/all/cover.1726009989.git.ackerleytng@xxxxxxxxxx/T/
