On Wed, May 03, 2023 at 06:17:58PM +0100, Robin Murphy wrote:
> 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.
Yeah, I like that direction
> 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)
Can we leave this till when we get there? I think it makes sense to
add an args struct and the first one can contain the quirk field.
The trouble with this approach is we still have to get the alloc to
reject inputs it doesn't understand.
> 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.
I think the places it needs to touch is slowly shrinking :)
But it would be great to get the bus stuff going, we can organize the
series in whatever order, but I think the "Consolidate the error
handling around device attachment" series is good now so we should get
it in Joerg's tree and start there.
> > @@ -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?
Oops, yeah, will fix
> cleaner. And it makes my iommu_group_first_device() helper look even more
> appealing, if I dare say so myself :)
It is a good idea, but I seem to recall the count of users has reduced
after my last two series
> > @@ -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?
It is deliberate for now, if it included FQ it would cause a bunch of
ARM64 drivers to switch to lazy mode. I'll add a comment.
I have drafted a followup series that removes all the
DMA/DMA_FQ/UNMANAGED checks from the remaining 6 drivers. I did this
by adding an op flag 'prefer to use FQ' and made the core code drive
the FQ decision from ops.
But that is another series for another day..
> 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 :/
I wouldn't say that. This converts 14 of the drivers and leaves 6. The
remaing 6 are more complicated and have to go in other series. eg I
have to reorganize DMA_FQ first.
Regardless, the point is not to be universal but to have a straight
forward sign off that these 14 drivers have been audited and they
don't have any policy logic in alloc_domain any more. Incremental
progress..
> > 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 :/
Yeah.. I can write something else here
> 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.
It is hard to keep the details out of the drivers if the argument to
ops->alloc_domain() encodes the details we want to keep opaque :)
So I have been taking the approach of just removing the
IOMMU_DOMAIN_xx text and related checks from the drivers as it becomes
possible. This series is the first wack and does every driver except
those that have dynamic identity domains.
DMA_FQ is the logical next step on this thread
> 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.
My general feeling is that we can remove them completely. At least DMA
and DMA_FQ I think can be removed totally with some reorganizing.
PAGING can become maybe (bool)domain->ops->iova_to_phys with some small
work on the remaining 6 drivers.
So we might just get down to SVA, IDENTITY and everything else
Jason
