| 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
| ||
|
Message-ID: <b205bcfb-9972-4e03-9994-62419117d76f@intel.com>
Date: Wed, 5 Jun 2024 14:23:56 +0800
From: Yi Liu <yi.l.liu@...el.com>
To: Baolu Lu <baolu.lu@...ux.intel.com>, Joerg Roedel <joro@...tes.org>, "Will
Deacon" <will@...nel.org>, Robin Murphy <robin.murphy@....com>, "Jason
Gunthorpe" <jgg@...pe.ca>, Kevin Tian <kevin.tian@...el.com>
CC: David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, "Kalle
Valo" <kvalo@...nel.org>, Bjorn Andersson <andersson@...nel.org>, "Mathieu
Poirier" <mathieu.poirier@...aro.org>, Alex Williamson
<alex.williamson@...hat.com>, <mst@...hat.com>, Jason Wang
<jasowang@...hat.com>, Thierry Reding <thierry.reding@...il.com>, "Jonathan
Hunter" <jonathanh@...dia.com>, Mikko Perttunen <mperttunen@...dia.com>,
<iommu@...ts.linux.dev>, <dri-devel@...ts.freedesktop.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 01/22] iommu: Add iommu_user_domain_alloc() interface
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@...ux.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
Powered by blists - more mailing lists