Re: [PATCH] iommu/arm-smmu-v3: Fail aliasing StreamIDs more gracefully

[Pranjal Shrivastava <praan@google.com>]

On Fri, Apr 11, 2025 at 12:21:22PM -0300, Jason Gunthorpe wrote:
> On Fri, Apr 11, 2025 at 03:09:14PM +0100, Robin Murphy wrote:
> > We've never supported StreamID aliasing between devices, and as such
> > they will never have had functioning DMA, but this is not fatal to the
> > SMMU itself. Although aliasing between hard-wired platform device
> > StreamIDs would tend to raise questions about the whole system, in
> > practice it's far more likely to occur relatively innocently due to
> > legacy PCI bridges, where the underlying StreamID mappings are still
> > perfectly reasonable.
> > 
> > As such, return a more benign -ENODEV when failing probe for such an
> > unsupported device (and log a more obvious error message), so that it
> > doesn't break the entire SMMU probe now that bus_iommu_probe() runs in
> > the right order and can propagate that error back. The end result is
> > still that the device doesn't get an IOMMU group and probably won't
> > work, same as before.
> > 
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index b4c21aaed126..c06459f7077b 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -3400,9 +3400,9 @@ static int arm_smmu_insert_master(struct arm_smmu_device *smmu,
> >  		/* Insert into SID tree */
> >  		if (rb_find_add(&new_stream->node, &smmu->streams,
> >  				arm_smmu_streams_cmp_node)) {
> > -			dev_warn(master->dev, "stream %u already in tree\n",
> > +			dev_warn(master->dev, "Aliasing StreamID 0x%x unsupported, expect DMA to be broken\n",
> >  				 sid);
> > -			ret = -EINVAL;
> > +			ret = -ENODEV;
> >  			break;
> 
> I don't think we should dillute the meaning of ENODEV here. It is
> supposed to mean that the driver does not recognize the device and
> this IOMMU instance doesn't translate for it.
> 
> That is different from the iommu instance owning the device but being
> unable to probe it for some reason. The failure recovery in the core
> code should be different.
> 
> IMHO it makes more sense to change this and related:
> 
> static int bus_iommu_probe(const struct bus_type *bus)
> {
> 	struct iommu_group *group, *next;
> 	LIST_HEAD(group_list);
> 	int ret;
> 
> 	ret = bus_for_each_dev(bus, NULL, &group_list, probe_iommu_group);
> 	if (ret)
> 		return ret;
> 
> So that recognized, but failed, devices are contained to only effect
> that device and do not fail the entire iommu or bus registration,
> which is more like how things used to work when the probe path would
> ignore the per-driver failure codes.
> 
> I can't think of a reason why we'd want any per-device failure to also
> abort the whole iommu registration??

I don't think this change would abort the iommu registration?
The probe_iommu_group would simply ignore the -ENODEV. Or are you
talking about a case where we don't return -ENODEV?

>
> It would be nice if the dev->iommu would record that the struct device
> is inoperable and then we can fail
> iommu_device_use_default_domain()/etc in a contained way.

Ohh okay.. you mean the dev->iommu_group == NULL may give an illusion to
<bus>_dma_configure that DMA is allowed? Even in that case the iommu 
would block the DMA right? And upon inspection of dmesg it would be
clear that something went wrong with the probe?


> 
> Jason
> 

Thanks,
Praan