[PATCH 18/20] iommu: Add ops->domain_alloc_paging()
Robin Murphy
robin.murphy at arm.com
Wed May 3 10:17:58 PDT 2023
On 2023-05-01 19:03, Jason Gunthorpe wrote:
> This callback requests the driver to create only a __IOMMU_DOMAIN_PAGING
> domain, so it saves a few lines in a lot of drivers needlessly checking
> the type.
>
> More critically, this allows us to sweep out all the
> IOMMU_DOMAIN_UNMANAGED and IOMMU_DOMAIN_DMA checks from a lot of the
> drivers, simplifying what is going on in the code and ultimately removing
> the now-unused special cases in drivers where they did not support
> IOMMU_DOMAIN_DMA.
>
> domain_alloc_paging() should return a struct iommu_domain that is
> functionally compatible with ARM_DMA_USE_IOMMU, dma-iommu.c and iommufd.
>
> Be forwards looking and pass in a 'struct device *' argument. We can
> provide this when allocating the default_domain. No drivers will look at
> this.
As discussed previously, we're going to want a way for callers to pass
through various options as well, initially to replace
iommu_set_pgtable_quirks() at the very least. Maybe passing an
easily-extensible structure around is the even better option? (Wherein
we could even strictly enforce "no drivers will look at this" for the
moment by leaving it empty)
I'm hoping I'll get another version of my bus ops removal out this
cycle; there's obviously a lot of overlap here to figure out.
> Signed-off-by: Jason Gunthorpe <jgg at nvidia.com>
> ---
> drivers/iommu/iommu.c | 40 ++++++++++++++++++++++++----------------
> include/linux/iommu.h | 2 ++
> 2 files changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f20a031e2910b2..fee417df8f195d 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,6 +93,7 @@ static int iommu_bus_notifier(struct notifier_block *nb,
> unsigned long action, void *data);
> static void iommu_release_device(struct device *dev);
> static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> + struct device *dev,
> unsigned type);
> static int __iommu_attach_device(struct iommu_domain *domain,
> struct device *dev);
> @@ -1618,12 +1619,12 @@ static int iommu_get_def_domain_type(struct device *dev)
> }
>
> static struct iommu_domain *
> -__iommu_group_alloc_default_domain(struct bus_type *bus,
> +__iommu_group_alloc_default_domain(struct group_device *gdev,
> struct iommu_group *group, int req_type)
> {
> if (group->default_domain && group->default_domain->type == req_type)
> return group->default_domain;
> - return __iommu_domain_alloc(bus, req_type);
> + return __iommu_domain_alloc(gdev->dev->bus, gdev->dev, req_type);
> }
>
> /*
> @@ -1633,9 +1634,9 @@ __iommu_group_alloc_default_domain(struct bus_type *bus,
> static struct iommu_domain *
> iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
> {
> - struct bus_type *bus =
> - list_first_entry(&group->devices, struct group_device, list)
> - ->dev->bus;
> + struct group_device *gdev =
> + list_first_entry(&group->devices, struct group_device, list);
Eww, why pass around a group_device that nobody wants? Keeping the
single dereference of ->dev here, rather than the three above and below,
would be cleaner. And it makes my iommu_group_first_device() helper look
even more appealing, if I dare say so myself :)
> + const struct iommu_ops *ops = dev_iommu_ops(gdev->dev);
> struct iommu_domain *dom;
>
> lockdep_assert_held(&group->mutex);
> @@ -1645,14 +1646,15 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
> * domain. This should always be either an IDENTITY or PLATFORM domain.
> * Do not use in new drivers.
> */
> - if (bus->iommu_ops->default_domain) {
> + if (ops->default_domain) {
> if (req_type)
> return ERR_PTR(-EINVAL);
> - return bus->iommu_ops->default_domain;
> + return ops->default_domain;
> }
>
> if (req_type)
> - return __iommu_group_alloc_default_domain(bus, group, req_type);
> + return __iommu_group_alloc_default_domain(gdev, group,
> + req_type);
>
> /*
> * If ARM32 CONFIG_ARM_DMA_USE_IOMMU is enabled and the driver doesn't
> @@ -1665,18 +1667,19 @@ iommu_group_alloc_default_domain(struct iommu_group *group, int req_type)
> IS_ENABLED(CONFIG_IOMMU_DMA)));
>
> return __iommu_group_alloc_default_domain(
> - bus, group, IOMMU_DOMAIN_IDENTITY);
> + gdev, group, IOMMU_DOMAIN_IDENTITY);
> }
>
> /* The driver gave no guidance on what type to use, try the default */
> - dom = __iommu_group_alloc_default_domain(bus, group, iommu_def_domain_type);
> + dom = __iommu_group_alloc_default_domain(gdev, group,
> + iommu_def_domain_type);
> if (dom)
> return dom;
>
> /* Otherwise IDENTITY and DMA_FQ defaults will try DMA */
> if (iommu_def_domain_type == IOMMU_DOMAIN_DMA)
> return NULL;
> - dom = __iommu_group_alloc_default_domain(bus, group, IOMMU_DOMAIN_DMA);
> + dom = __iommu_group_alloc_default_domain(gdev, group, IOMMU_DOMAIN_DMA);
> if (!dom)
> return NULL;
>
> @@ -1930,6 +1933,7 @@ void iommu_set_fault_handler(struct iommu_domain *domain,
> EXPORT_SYMBOL_GPL(iommu_set_fault_handler);
>
> static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> + struct device *dev,
> unsigned type)
> {
> struct iommu_domain *domain;
> @@ -1940,7 +1944,11 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
> if (type == IOMMU_DOMAIN_IDENTITY && bus->iommu_ops->identity_domain)
> return bus->iommu_ops->identity_domain;
>
> - domain = bus->iommu_ops->domain_alloc(type);
> + if ((type == IOMMU_DOMAIN_UNMANAGED || type == IOMMU_DOMAIN_DMA) &&
Logically, "type & __IOMMU_DOMAIN_PAGING", otherwise we're already
missing IOMMU_DOMAIN_DMA_FQ. Except maybe that's deliberate? Not sure
how much I like the idea of a introducing new interface design that we
clearly can't even convert all the current drivers to as it stands :/
> + bus->iommu_ops->domain_alloc_paging)
> + domain = bus->iommu_ops->domain_alloc_paging(dev);
> + else
> + domain = bus->iommu_ops->domain_alloc(type);
> if (!domain)
> return NULL;
>
> @@ -1964,7 +1972,7 @@ static struct iommu_domain *__iommu_domain_alloc(struct bus_type *bus,
>
> struct iommu_domain *iommu_domain_alloc(struct bus_type *bus)
> {
> - return __iommu_domain_alloc(bus, IOMMU_DOMAIN_UNMANAGED);
> + return __iommu_domain_alloc(bus, NULL, IOMMU_DOMAIN_UNMANAGED);
> }
> EXPORT_SYMBOL_GPL(iommu_domain_alloc);
>
> @@ -3079,15 +3087,15 @@ static int __iommu_group_alloc_blocking_domain(struct iommu_group *group)
> if (group->blocking_domain)
> return 0;
>
> - group->blocking_domain =
> - __iommu_domain_alloc(dev->dev->bus, IOMMU_DOMAIN_BLOCKED);
> + group->blocking_domain = __iommu_domain_alloc(dev->dev->bus, dev->dev,
> + IOMMU_DOMAIN_BLOCKED);
> if (!group->blocking_domain) {
> /*
> * For drivers that do not yet understand IOMMU_DOMAIN_BLOCKED
> * create an empty domain instead.
> */
> group->blocking_domain = __iommu_domain_alloc(
> - dev->dev->bus, IOMMU_DOMAIN_UNMANAGED);
> + dev->dev->bus, dev->dev, IOMMU_DOMAIN_UNMANAGED);
> if (!group->blocking_domain)
> return -EINVAL;
> }
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index f6a28ab78e607e..cc9aff2d213eec 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -227,6 +227,7 @@ struct iommu_iotlb_gather {
> * struct iommu_ops - iommu ops and capabilities
> * @capable: check capability
> * @domain_alloc: allocate iommu domain
> + * @domain_alloc_paging: Allocate an IOMMU_DOMAIN_UNMANAGED
...except for the other types of paging-capable domain which also exist
and it also allocates :/
There remains good reason to keep track of the distinct subtypes of
paging domain within the IOMMU core (i.e. between iommu.c and
dma-iommu.c) even if drivers do finally get absolved of all the details.
Sure we could come up with any number of different ways to encode those
distinctions, but they wouldn't be objectively better than the domain
flags and types we already have, they'd just be different.
Thanks,
Robin.
> * @probe_device: Add device to iommu driver handling
> * @release_device: Remove device from iommu driver handling
> * @probe_finalize: Do final setup work after the device is added to an IOMMU
> @@ -258,6 +259,7 @@ struct iommu_ops {
>
> /* Domain allocation and freeing by the iommu driver */
> struct iommu_domain *(*domain_alloc)(unsigned iommu_domain_type);
> + struct iommu_domain *(*domain_alloc_paging)(struct device *dev);
>
> struct iommu_device *(*probe_device)(struct device *dev);
> void (*release_device)(struct device *dev);
More information about the Linux-rockchip
mailing list