Received-SPF: pass (google.com: domain of linux-kernel+bounces-201800-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <b205bcfb-9972-4e03-9994-62419117d76f@intel.com>
Date: Wed, 5 Jun 2024 14:23:56 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 01/22] iommu: Add iommu_user_domain_alloc() interface
To: Baolu Lu <baolu.lu@linux.intel.com>, Joerg Roedel <joro@8bytes.org>, "Will
 Deacon" <will@kernel.org>, Robin Murphy <robin.murphy@arm.com>, "Jason
 Gunthorpe" <jgg@ziepe.ca>, Kevin Tian <kevin.tian@intel.com>
CC: David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, "Kalle
 Valo" <kvalo@kernel.org>, Bjorn Andersson <andersson@kernel.org>, "Mathieu
 Poirier" <mathieu.poirier@linaro.org>, Alex Williamson
	<alex.williamson@redhat.com>, <mst@redhat.com>, Jason Wang
	<jasowang@redhat.com>, Thierry Reding <thierry.reding@gmail.com>, "Jonathan
 Hunter" <jonathanh@nvidia.com>, Mikko Perttunen <mperttunen@nvidia.com>,
	<iommu@lists.linux.dev>, <dri-devel@lists.freedesktop.org>,
	<linux-kernel@vger.kernel.org>
References: <20240604015134.164206-1-baolu.lu@linux.intel.com>
 <20240604015134.164206-2-baolu.lu@linux.intel.com>
 <445fae9f-ea1e-4864-9f0e-f348c51146a1@intel.com>
 <91a42f46-060a-4430-a10a-7bfded67c4a9@linux.intel.com>
Content-Language: en-US
From: Yi Liu <yi.l.liu@intel.com>
In-Reply-To: <91a42f46-060a-4430-a10a-7bfded67c4a9@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NzVmZlVTU09GZjBXdis4K0VWaTNhYWF1RUplVUtwTVdIUHh5Z0xYYjdLMFk3?=
 =?utf-8?B?ekFQVlV3MElOTUYwSzFvcGYyQnVpbW5aYnlUYlFCY2tyUnhHNjhMZG1GMFEz?=
 =?utf-8?B?akEvLzNaeU0vM284MWxJNWh3T0s4YmlWZFJaaG42azFKanp4dE5mWkF0UjhX?=
 =?utf-8?B?ZEJpUCtNeGk4VTl3dDVHUmxUbWJxTXNDZk1GWGVxNUllTDIrWjVuOGp2elJE?=
 =?utf-8?B?VG9QQWZ4Y2NYL0t1bXNrUytRTmVXQ3hZMFJSc0ZaMUdOSmdNU2JGOVJUd0JT?=
 =?utf-8?B?TW1Fd1FmSFFCdWIxem1sTG9WeGdBMHJiUlFCUjc1NUxNVkJaTE9QeWtqWkRS?=
 =?utf-8?B?THREbGFqNVVuQ2JuN3hCVmdDeEpaaXBVL0pIMDlEWlltWitoNjJROHBwYzho?=
 =?utf-8?B?aGQ5cFBZQ2V6UEU4cHF4dlA0QjRJYzJlblRvZUc0MS9ZTVhoM01rZWJhTVVD?=
 =?utf-8?B?NEthMnRoK1h2OWRqdzdNSGhtMm9FUnJERU1lUWdUcEdYMEM2TDd0OERXMjRh?=
 =?utf-8?B?RjF4RlM4bmJiMWpsUFFHNE1vcWJyS1lmSmlLYzBJNnBJT2h5RnUrZ255YW41?=
 =?utf-8?B?ZFcwaEFyOVl1MDBDblhYRUkwaGY2Nk1meFg3Mm02SXlDY212N28ySmlwNFc1?=
 =?utf-8?B?SUNCSFRraXNRb0xUNTBrZzd6YXNLUlZBSXYxcmt1UU9mNlEyU3lidlZGaDJS?=
 =?utf-8?B?dTdRbjBEdFc1MjVuMWo2dyt3SzJMNllVM2Npa0NlcWxWN25KamNuZVd2Z0Nm?=
 =?utf-8?B?RS9OZElxUnNWWmxKVnpWWTVPWlpXV1BUYXJzemxNd0pSKzliR2J6aWF5ZURQ?=
 =?utf-8?B?ekRkSHpuTWkrckpiZVp1MnAybnJFVlFTbEUvNkQ0eDhxbk1FVXpHdUdKK1g3?=
 =?utf-8?B?ZXNlVCtsRDV4cGY1YVJHdDZneEhUNXVkMGNmSWZnTDN1WHAxa3lTN2N4eWNn?=
 =?utf-8?B?YzQ5SjM1cVg5ZmgyZldWWEwyRFJDVzNMUmk5eWRNTFZDWHh4SWxmZ1dZOFpE?=
 =?utf-8?B?dDR0RVNsT29mUlZEc040QWtmeGROUW5UczJFdGxDM2pVenJyMi9TczVxaXps?=
 =?utf-8?B?VUFacDAyZnZiRjA3Y2RkWVRiU0xwaTdhZllOYzNVWVplRG5rTDZjekxKNlA0?=
 =?utf-8?B?MDdOUjhHbjlrR3RtekZPQXNKaCtTMVQrMTRQd1JuMHlIYlRCK0dnbmp4Q0Nx?=
 =?utf-8?B?d0xOc3NoQlBOaVVnc0NMMlIrT0JkZmtYbzhoenNrdEExMDhUNDRXU2w4T3Z3?=
 =?utf-8?B?cXIzNFppVmRjY1VaMFhoOFFGbkMwVy9tUWpBeVMwbU9vZ2dadmZkQzFyVlZw?=
 =?utf-8?B?ZmM0S3R1cnJmNVJDUzFPWHllTDd2WTBlbGt4cm53a1ZYVDNnUFdNK2d3a293?=
 =?utf-8?B?Mmp0SjNvVE84b3l3Y0FvZ0E1TTFWL2RGTXNGWFd6SElvQ3M2ckl0MWUzaXVh?=
 =?utf-8?B?eUJwdlR0ZktVcGowRGtxNzV0ZFY2cDFpQjFEclZ2QldLRWlvY2o1QklRSlFP?=
 =?utf-8?B?N3VHS0xuVitnY1RYVUNsM1M5VmpGK1pmOWtDZFpTQ29pZzlnQXlKRlhmcVFq?=
 =?utf-8?B?S3Fud2h4b2IxNzhYMWI2bDdSV3dlczlaYUVBR1Ztc09sQTlsK3FVVzl2QURq?=
 =?utf-8?B?UVBrbHhTYzMwaFJDZ1VlTTRzTHJzNm45TXNzWFphSkhHSkJMajVvMUhSL0p6?=
 =?utf-8?B?b2xhak9GVVVlQWFxVmRlWmFIUFY5cDRVWEV2dVhpR0ExaUxNeVFrSzdVaEI3?=
 =?utf-8?B?SHIrc0tJNHhHWTFCSFZFSGIyVjNwMmozSkhDWCtsN0U0T1pXaWdoWERTM2JT?=
 =?utf-8?B?TU9BZ0Z0am82dWVBTE1ndVgraTdZaXd1VWlqSnprZHdPUjZ6enRYcGlVZ0tI?=
 =?utf-8?B?MHlLMHNacEsrMUYwN2ZtU0xqM0U3eXlTdU11SEFmSUx5b1h4b3R3Wk8xZHZ1?=
 =?utf-8?B?LzRoN1g3QnR3MVhvemVmTDVzazZyWVcrd245bjVlVHk2ZlN1aXZBMEdMaU5D?=
 =?utf-8?B?TlZveHBJamN4THVyelVZVWhqQ2swSjVpNjk3eFFaMDJlWExyRktYeVhOaUs1?=
 =?utf-8?B?VGNJeEVmYXAyV2RjTEJTK2RNWEpDNEdsT1MybnV3UTg3VnBXOUhHdEdkUGdT?=
 =?utf-8?Q?SJ3OTv/gPGYnv9khpv8s6fpn6?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a2bf3d0c-a711-4d61-c698-08dc85278826
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB7529.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 Jun 2024 06:20:01.6772
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Jff8HfHOYDCc49ShrrPFeqcJcaIJabOLkBzND+PYQ91CGZQRgB6Cz/poQRYEzPS/O7dNanSUuTBCgr2VU7a5WQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB6994
X-OriginatorOrg: intel.com

On 2024/6/5 10:00, Baolu Lu wrote:
> On 6/4/24 4:03 PM, Yi Liu wrote:
>> On 2024/6/4 09:51, Lu Baolu wrote:
>>> Commit <909f4abd1097> ("iommu: Add new iommu op to create domains owned
>>> by userspace") added a dedicated iommu op to allocate a user domain.
>>> While IOMMUFD has already made use of this callback, other frameworks
>>> like vfio/type1 and vDPA still use the paging domain allocation interface.
>>>
>>> Add a new interface named iommu_user_domain_alloc(), which indicates the
>>> allocation of a domain for device DMA managed by user space driver. All
>>> device passthrough frameworks could use this interface for their domain
>>> allocation.
>>>
>>> Although it is expected that all iommu drivers could implement their own
>>> domain_alloc_user ops, most drivers haven't implemented it yet. Rollback
>>> to the paging domain allocation interface if the iommu driver hasn't
>>> implemented this op yet.
>>>
>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>> ---
>>>   include/linux/iommu.h |  6 ++++++
>>>   drivers/iommu/iommu.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 48 insertions(+)
>>>
>>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>>> index 7bc8dff7cf6d..6648b2415474 100644
>>> --- a/include/linux/iommu.h
>>> +++ b/include/linux/iommu.h
>>> @@ -780,6 +780,7 @@ extern bool iommu_present(const struct bus_type *bus);
>>>   extern bool device_iommu_capable(struct device *dev, enum iommu_cap cap);
>>>   extern bool iommu_group_has_isolated_msi(struct iommu_group *group);
>>>   extern struct iommu_domain *iommu_domain_alloc(const struct bus_type 
>>> *bus);
>>> +struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 
>>> flags);
>>>   extern void iommu_domain_free(struct iommu_domain *domain);
>>>   extern int iommu_attach_device(struct iommu_domain *domain,
>>>                      struct device *dev);
>>> @@ -1086,6 +1087,11 @@ static inline struct iommu_domain 
>>> *iommu_domain_alloc(const struct bus_type *bus
>>>       return NULL;
>>>   }
>>> +static inline struct iommu_domain *iommu_user_domain_alloc(struct 
>>> device *dev, u32 flags)
>>> +{
>>> +    return ERR_PTR(-ENODEV);
>>> +}
>>> +
>>>   static inline void iommu_domain_free(struct iommu_domain *domain)
>>>   {
>>>   }
>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>> index 9df7cc75c1bc..f1416892ef8e 100644
>>> --- a/drivers/iommu/iommu.c
>>> +++ b/drivers/iommu/iommu.c
>>> @@ -2032,6 +2032,48 @@ struct iommu_domain *iommu_domain_alloc(const 
>>> struct bus_type *bus)
>>>   }
>>>   EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>>> +/**
>>> + * iommu_user_domain_alloc() - Allocate a user domain
>>> + * @dev: device for which the domain is allocated
>>> + * @flags: iommufd_hwpt_alloc_flags defined in uapi/linux/iommufd.h
>>> + *
>>> + * Allocate a user domain which will be managed by a userspace driver. 
>>> Return
>>> + * allocated domain if successful, or a ERR pointer for failure.
>>
>> do you want to mention that this is for paging domain allocation?
> 
> You are worrying about its confusion with nesting domain allocation,
> right?

yes. As I replied in last version, user_domain is a bit confusing since
nested domain is also a user_domain. That's why we introduced the
alloc_domain_user op.

> My understanding is that if we want a common interface for
> nesting domain allocation, we should make it in another interface. Here,
> the user domain is a paging domain for GVA->HPA mapping, which is common
> across iommufd, vfio, and vdpa.

do you mean user_domain only includes paging domain?

>>
>>> + */
>>> +struct iommu_domain *iommu_user_domain_alloc(struct device *dev, u32 
>>> flags)
>>> +{
>>> +    struct iommu_domain *domain;
>>> +    const struct iommu_ops *ops;
>>> +
>>> +    if (!dev_has_iommu(dev))
>>> +        return ERR_PTR(-ENODEV);
>>> +
>>> +    ops = dev_iommu_ops(dev);
>>> +    if (ops->domain_alloc_user) {
>>> +        domain = ops->domain_alloc_user(dev, flags, NULL, NULL);
>>> +        if (IS_ERR(domain))
>>> +            return domain;
>>> +
>>> +        domain->type = IOMMU_DOMAIN_UNMANAGED;
>>> +        domain->owner = ops;
>>> +        domain->pgsize_bitmap = ops->pgsize_bitmap;
>>
>> this seems to break the iommufd selftest as the mock driver sets extra
>> bits in the domain->pgsize_bitmap in allocation. Override it may fail
>> something in the testing. you may need to check if domain->pgsize_bitmap
>> is set or use &=.
>>
>> static struct iommu_domain *mock_domain_alloc_paging(struct device *dev)
>> {
>>      struct mock_dev *mdev = container_of(dev, struct mock_dev, dev);
>>      struct mock_iommu_domain *mock;
>>
>>      mock = kzalloc(sizeof(*mock), GFP_KERNEL);
>>      if (!mock)
>>          return NULL;
>>      mock->domain.geometry.aperture_start = MOCK_APERTURE_START;
>>      mock->domain.geometry.aperture_end = MOCK_APERTURE_LAST;
>>      mock->domain.pgsize_bitmap = MOCK_IO_PAGE_SIZE;
>>      if (dev && mdev->flags & MOCK_FLAGS_DEVICE_HUGE_IOVA)
>>          mock->domain.pgsize_bitmap |= MOCK_HUGE_PAGE_SIZE;
>>      mock->domain.ops = mock_ops.default_domain_ops;
>>      mock->domain.type = IOMMU_DOMAIN_UNMANAGED;
>>      xa_init(&mock->pfns);
>>      return &mock->domain;
>> }
> 
> You are right. I should follow the code in __iommu_domain_alloc()
> 
>          /*
>           * If not already set, assume all sizes by default; the driver
>           * may override this later
>           */
>          if (!domain->pgsize_bitmap)
>                  domain->pgsize_bitmap = ops->pgsize_bitmap;
> 
> Does it work?

I think so. This should work.

-- 
Regards,
Yi Liu