linux-kernel - Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b05e5856-a9f3-a7b5-32d5-bf3a9e0f2bfa@nvidia.com>
Date:   Wed, 26 Oct 2016 20:36:01 +0530
From:   Kirti Wankhede <kwankhede@...dia.com>
To:     "Tian, Kevin" <kevin.tian@...el.com>,
        Alex Williamson <alex.williamson@...hat.com>
CC:     "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "kraxel@...hat.com" <kraxel@...hat.com>,
        "cjia@...dia.com" <cjia@...dia.com>,
        "qemu-devel@...gnu.org" <qemu-devel@...gnu.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "Song, Jike" <jike.song@...el.com>,
        "bjsdjshi@...ux.vnet.ibm.com" <bjsdjshi@...ux.vnet.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v9 04/12] vfio iommu: Add support for mediated devices



On 10/26/2016 12:49 PM, Tian, Kevin wrote:
>> From: Alex Williamson [mailto:alex.williamson@...hat.com]
>> Sent: Monday, October 24, 2016 10:32 AM
>>
>>>>> -static long vfio_unpin_pages(unsigned long pfn, long npage,
>>>>> -			     int prot, bool do_accounting)
>>>>> +static long __vfio_unpin_pages_remote(struct vfio_iommu *iommu,
>>>>> +				      unsigned long pfn, long npage, int prot,
>>>>> +				      bool do_accounting)
>>>>
>>>> Have you noticed that it's kind of confusing that
>>>> __vfio_{un}pin_pages_remote() uses current, which does a
>>>> get_user_pages_fast() while "local" uses a provided task_struct and
>>>> uses get_user_pages_*remote*()?  And also what was effectively local
>>>> (ie. we're pinning for our own use here) is now "remote" and pinning
>>>> for a remote, vendor driver consumer, is now "local".  It's not very
>>>> intuitive.
>>>>
> 
> I questioned this confusing naming in v8 too...
> 

I do tried to address your concerns on v8.

>>>
>>> 'local' in local_domain was suggested to describe the domain for local
>>> page tracking. Earlier suggestions to have 'mdev' or 'noimmu' in this
>>> name were discarded. May be we should revisit what the name should be.
>>> Any suggestion?
>>>
>>> For local_domain, to pin pages, flow is:
>>>
>>> for local_domain
>>>     |- vfio_pin_pages()
>>>         |- vfio_iommu_type1_pin_pages()
>>>             |- __vfio_pin_page_local()
>>>                 |-  vaddr_get_pfn(task->mm)
>>>                     |- get_user_pages_remote()
>>>
>>> __vfio_pin_page_local() --> get_user_pages_remote()
>>
>>
>> In vfio.c we have the concept of an external user, perhaps that could
>> be continued here.  An mdev driver would be an external, or remote
>> pinning.
>>
> 
> I prefer to use remote here. It's aligned with underlying mm operations
> 

Using 'remote' in this case is also confusing since it is already used
in this file. I liked Alex's suggestion to use external and I'll have
those changed in next version of patch set.

Kirti