Re: [PATCH V7 6/9] iommufd: pfn reader for file mappings

[Steven Sistare <steven.sistare@oracle.com>]

On 11/7/2024 5:00 AM, Alexey Kardashevskiy wrote:
> 
> 
> On 7/11/24 00:19, Steven Sistare wrote:
>> On 11/5/2024 10:18 PM, Alexey Kardashevskiy wrote:
>>> On 31/10/24 14:34, Alexey Kardashevskiy wrote:
>>>> On 26/10/24 00:11, Steve Sistare wrote:
>>>>> Extend pfn_reader_user to pin file mappings, by calling memfd_pin_folios.
>>>>> Repin at small page granularity, and fill the batch from folios. Expand
>>>>> folios to upages for the iopt_pages_fill path.
>>>>>
>>>>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
>>>>> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
>>>>> Reviewed-by: Kevin Tian <kevin.tian@intel.com>
>>>>> ---
>>>>>   drivers/iommu/iommufd/io_pagetable.h |   5 ++
>>>>>   drivers/iommu/iommufd/pages.c        | 128 ++++++++++++++++++++++++++++++-----
>>>>>   2 files changed, 116 insertions(+), 17 deletions(-)
>>>>>
>>>>> diff --git a/drivers/iommu/iommufd/io_pagetable.h b/drivers/iommu/iommufd/io_pagetable.h
>>>>> index 8e48266..5ac4eed 100644
>>>>> --- a/drivers/iommu/iommufd/io_pagetable.h
>>>>> +++ b/drivers/iommu/iommufd/io_pagetable.h
>>>>> @@ -177,6 +177,7 @@ enum {
>>>>>   enum iopt_address_type {
>>>>>       IOPT_ADDRESS_USER = 0,
>>>>> +    IOPT_ADDRESS_FILE = 1,
>>>>>   };
>>>>>   /*
>>>>> @@ -202,6 +203,10 @@ struct iopt_pages {
>>>>>       enum iopt_address_type type;
>>>>>       union {
>>>>>           void __user *uptr;        /* IOPT_ADDRESS_USER */
>>>>> +        struct {            /* IOPT_ADDRESS_FILE */
>>>>> +            struct file *file;
>>>>> +            unsigned long start;
>>>>> +        };
>>>>>       };
>>>>>       bool writable:1;
>>>>>       u8 account_mode;
>>>>> diff --git a/drivers/iommu/iommufd/pages.c b/drivers/iommu/iommufd/pages.c
>>>>> index aa79504..5f371fa 100644
>>>>> --- a/drivers/iommu/iommufd/pages.c
>>>>> +++ b/drivers/iommu/iommufd/pages.c
>>>>> @@ -752,19 +752,32 @@ struct pfn_reader_user {
>>>>>        * neither
>>>>>        */
>>>>>       int locked;
>>>>> +
>>>>> +    /* The following are only valid if file != NULL. */
>>>>
>>>>
>>>> Are "struct page **upages" and "size_t upages_len" still valid in this case? The code (kind of) suggests they are not... Thanks,
>>>
>>>
>>> Ping?
>>
>> Sorry, I don't recall seeing your first email, but others asked this same question
>> and I answered.  upages and upages_len are still used when file!=NULL for mediated
>> device access, to point to the pages array returned to the kernel client. 
> 
> This would make a nice addition to the commit log :)
> 
>> Maybe
>> "u" for user is not a good description in that case, but changing that and still
>> using "u"pages elsewhere would complicate the code for little gain in clarity IMO.
>>
>>> Also does "user" in function names mean "userspace addresses" (as opposite to "file"? 
>>
>> Yes, except for mdev as noted above.
>>
>>> If so, the patchset makes many names misleading then. Thanks,
>>
>> The new stuff I added is prefixed with file.  I left the existing upages names
>> as is.  That made sense to me, and made the maintainers happy enough to accept it :)
> 
> Fair enough :)
> 
> I was initially pointed at this patchset as an example of how my guest_memfd pinning of private memory (CoCo VM) should look like. And so far it's been good except the actual pinning part - memfd_pin_folios() - expects the file to be shmem or hugetlbfs and guest_memfd is neither (well, last couple of months). Is there any plan to add those? If none, I will then, just wanted to check. At the moment I do what KVM does which is calling filemap_grab_folio(). Thanks,

I have no plans to implement guest_memfd pinning.  Not sure about other folks.

- Steve