Re: [PATCH v11 12/26] gunyah: vm_mgr: Add/remove user memory regions

[Elliot Berman]

On 3/31/2023 7:26 AM, Alex Elder wrote:
> On 3/3/23 7:06 PM, Elliot Berman wrote:
> > +
> > +    mem_entries = kcalloc(mapping->npages, sizeof(*mem_entries), 
> > GFP_KERNEL);
> > +    if (!mem_entries) {
> > +        ret = -ENOMEM;
> > +        goto reclaim;
> > +    }
> > +
> > +    /* reduce number of entries by combining contiguous pages into 
> > single memory entry */
>
> Are you sure you need to do this?  I.e., does pin_user_pages_fast()
> already take care of consolidating these pages?

pin_user_pages_fast wouldn't consolidate the page entries. There's a 
speedup in sharing memory when pages are contiguous since less 
information needs to be transmitted to Gunyah describing the memory.

> > +    prev_page = page_to_phys(mapping->pages[0]);
> > +    mem_entries[0].ipa_base = cpu_to_le64(prev_page);
> > +    entry_size = PAGE_SIZE;
> > +    for (i = 1, j = 0; i < mapping->npages; i++) {
> > +        curr_page = page_to_phys(mapping->pages[i]);
>
> I think you can actually use the page frame numbers
> here instead of the addresses.  If they are consecutive,
> they are contiguous.  See pages_are_mergeable() for an
> example of that.  Using PFNs might simplify this code.

It did, thanks for the suggestion!

> > +        if (curr_page - prev_page == PAGE_SIZE) {
> > +            entry_size += PAGE_SIZE;
> > +        } else {
> > +            mem_entries[j].size = cpu_to_le64(entry_size);
> > +            j++;
> > +            mem_entries[j].ipa_base = cpu_to_le64(curr_page);
> > +            entry_size = PAGE_SIZE;
> > +        }
> > +
> > +        prev_page = curr_page;
> > +    }
> > +    mem_entries[j].size = cpu_to_le64(entry_size);
>
> It might be messier, but it seems like you could scan the pages to
> see how many you'll need (after combining), then allocate the array
> of mem entries based on that.  That is, do that rather than allocating,
> filling, then duplicating and freeing.
>
>      count = 1;
>      curr_page = mapping->pages[0];
>      for (i = 1; i < mapping->npages; i++) {
>          next_page = mapping->pages[i];
>          if (page_to_pfn(next_page) !=
>                  page_to_pfn(curr_page) + 1)
>              count++;
>          curr_page = next_page;
>      }
>      parcel->n_mem_entries = count;
>      parcel->mem_entries = kcalloc(count, ...);
>      /* Then fill them up */
>
> (Not tested, but you get the idea.)

It wasn't too messy IMO, I think this ended up simplifying the loop.

> > +
> > +    parcel->n_mem_entries = j + 1;
> > +    parcel->mem_entries = kmemdup(mem_entries, sizeof(*mem_entries) * 
> > parcel->n_mem_entries,
> > +                    GFP_KERNEL);
> > +    kfree(mem_entries);
> > +    if (!parcel->mem_entries) {
> > +        ret = -ENOMEM;
> > +        goto reclaim;
> > +    }
> > +
> > +    mutex_unlock(&ghvm->mm_lock);
> > +    return 0;
> > +reclaim:
> > +    gh_vm_mem_reclaim(ghvm, mapping);
> > +free_mapping:
> > +    kfree(mapping);
> > +    mutex_unlock(&ghvm->mm_lock);
> > +    return ret;
> > +}
> > +
> > +int gh_vm_mem_free(struct gh_vm *ghvm, u32 label)
> > +{
> > +    struct gh_vm_mem *mapping;
> > +    int ret;
> > +
> > +    ret = mutex_lock_interruptible(&ghvm->mm_lock);
> > +    if (ret)
> > +        return ret;
> > +
> > +    mapping = __gh_vm_mem_find_by_label(ghvm, label);
> > +    if (!mapping)
> > +        goto out;
> > +
> > +    gh_vm_mem_reclaim(ghvm, mapping);
> > +    kfree(mapping);
> > +out:
> > +    mutex_unlock(&ghvm->mm_lock);
> > +    return ret;
> > +}
> > diff --git a/include/uapi/linux/gunyah.h b/include/uapi/linux/gunyah.h
> > index 10ba32d2b0a6..a19207e3e065 100644
> > --- a/include/uapi/linux/gunyah.h
> > +++ b/include/uapi/linux/gunyah.h
> > @@ -20,4 +20,33 @@
> >    */
> >   #define GH_CREATE_VM            _IO(GH_IOCTL_TYPE, 0x0) /* Returns a 
> > Gunyah VM fd */
> > +/*
> > + * ioctls for VM fds
> > + */
> > +
>
> I think you should define the following three values in an enum.
>
> > +#define GH_MEM_ALLOW_READ    (1UL << 0)
> > +#define GH_MEM_ALLOW_WRITE    (1UL << 1)
> > +#define GH_MEM_ALLOW_EXEC    (1UL << 2)
> > +
> > +/**
> > + * struct gh_userspace_memory_region - Userspace memory descripion 
> > for GH_VM_SET_USER_MEM_REGION
> > + * @label: Unique identifer to the region.
>
> Unique with respect to what?  I think it's unique among memory
> regions defined within a VM.  And I think it's arbitrary and
> defined by the caller (right?).
>
> > + * @flags: Flags for memory parcel behavior
> > + * @guest_phys_addr: Location of the memory region in guest's memory 
> > space (page-aligned)
> > + * @memory_size: Size of the region (page-aligned)
> > + * @userspace_addr: Location of the memory region in caller 
> > (userspace)'s memory
> > + *
> > + * See Documentation/virt/gunyah/vm-manager.rst for further details.
> > + */
> > +struct gh_userspace_memory_region {
> > +    __u32 label;
> > +    __u32 flags;
>
> Add a comment to indicate what types of values "flags" can have.
> Maybe "flags" should be called "perms" or something?

Added documentation for the valid values of flags. I'm anticipating 
needing to add other flag values beyond permission bits.

> > +    __u64 guest_phys_addr;
> > +    __u64 memory_size;
> > +    __u64 userspace_addr;
>
> Why isn't userspace_addr just a (void *)?  That would be a more natural
> thing to pass to the kernel.  Is it to avoid 32-bit/64-bit pointer
> differences in the API?

Yes, to avoid 32-bit/64-bit pointer differences in API.

> > +};
> > +
> > +#define GH_VM_SET_USER_MEM_REGION    _IOW(GH_IOCTL_TYPE, 0x1, \
> > +                        struct gh_userspace_memory_region)
> > +
>
> I think it's nicer to group the definitions of these IOCTL values.
> Then in the struct definitions that follow, you can add comment that
> indicates which IOCTL the struct is used for.
>
> >   #endif