Re: [PATCH 1/8] iommu: Decouple iommu_present() from bus ops

[Robin Murphy]

On 2023-01-26 13:13, Baolu Lu wrote:
> On 2023/1/20 3:18, Robin Murphy wrote:
> > Much as I'd like to remove iommu_present(), the final remaining users
> > are proving stubbornly difficult to clean up, so kick that can down
> > the road and just rework it to preserve the current behaviour without
> > depending on bus ops.
> >
> > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > ---
> >   drivers/iommu/iommu.c | 17 ++++++++++++++++-
> >   1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index b189ed345057..a77d58e1b976 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1871,9 +1871,24 @@ int bus_iommu_probe(struct bus_type *bus)
> >       return ret;
> >   }
> > +static int __iommu_present(struct device *dev, void *unused)
> > +{
> > +    return device_iommu_mapped(dev);
> > +}
>
> /**
>   * device_iommu_mapped - Returns true when the device DMA is translated
>   *                       by an IOMMU
>   * @dev: Device to perform the check on
>   */
> static inline bool device_iommu_mapped(struct device *dev)
> {
>          return (dev->iommu_group != NULL);
> }
>
> Perhaps device_iommu_mapped() should be improved. In some cases, the
> device has an iommu_group filled is not enough to indicate that the
> device has IOMMU hardware for DMA translation.
>
> For example, VFIO could allocate an iommu_group and add a device into
> the iommu_group even there's no IOMMU hardware in
> vfio_noiommu_group_alloc().
>
> Basically iommu_group_add_device() doesn't check the presence of an
> IOMMU.

/**
 * iommu_group_add_device [...]
 *
 * This function is called by an iommu driver [...]
 */

The "check" is inherent in the fact that it's been called at all. VFIO 
noiommu *is* an IOMMU driver in the sense that it provides a bare 
minimum of IOMMU API functionality (i.e. creating groups), sufficient to 
support (careful) usage by VFIO drivers. There would not seem to be a 
legitimate reason for some *other* driver to be specifically querying a 
device while it is already bound to a VFIO driver (and thus may have a 
noiommu group).

In terms of this patch, I'm confident that nobody is using VFIO noiommu 
on old Tegra SoCs; I'm even more confident that they wouldn't be doing 
it with platform devices; and I'm supremely confident that they're not 
loading the GPU drivers while already in the middle of using noiommu 
vfio_platform. Basically "not using VFIO noiommu" is one of the inherent 
platform-specific assumptions. If anyone else now ignores the first 
sentence of the documentation and tries to use iommu_present() somewhere 
that assumption might not hold, returning a meaningless wrong answer is 
the documented behaviour :)

Cheers,
Robin.

> > +
> > +/**
> > + * iommu_present() - make platform-specific assumptions about an IOMMU
> > + * @bus: bus to check
> > + *
> > + * Do not use this function. You want device_iommu_mapped() instead.
> > + *
> > + * Return: true if some IOMMU is present for some device on the given 
> > bus. In
> > + * general it may not be the only IOMMU, and it may not be for the 
> > device you
> > + * are ultimately interested in.
> > + */
> >   bool iommu_present(struct bus_type *bus)
> >   {
> > -    return bus->iommu_ops != NULL;
> > +    return bus_for_each_dev(bus, NULL, NULL, __iommu_present) > 0;
> >   }
> >   EXPORT_SYMBOL_GPL(iommu_present);
>
> --
> Best regards,
> baolu