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

[Elliot Berman <quic_eberman@quicinc.com>]

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
>