Re: [PATCH v4 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group

[Robin Murphy]

On 01/07/2022 5:43 pm, Nicolin Chen wrote:
> On Fri, Jul 01, 2022 at 11:21:48AM +0100, Robin Murphy wrote:
>
> > > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > index 2ed3594f384e..072cac5ab5a4 100644
> > > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > > @@ -1135,10 +1135,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> > >        struct arm_smmu_device *smmu;
> > >        int ret;
> > >
> > > -     if (!fwspec || fwspec->ops != &arm_smmu_ops) {
> > > -             dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
> > > -             return -ENXIO;
> > > -     }
> > > +     if (!fwspec || fwspec->ops != &arm_smmu_ops)
> > > +             return -EMEDIUMTYPE;
> >
> > This is the wrong check, you want the "if (smmu_domain->smmu != smmu)"
> > condition further down. If this one fails it's effectively because the
> > device doesn't have an IOMMU at all, and similar to patch #3 it will be
>
> Thanks for the review! I will fix that. The "on the same bus" is
> quite eye-catching.
>
> > removed once the core code takes over properly (I even have both those
> > patches written now!)
>
> Actually in my v1 the proposal for ops check returned -EMEDIUMTYPE
> also upon an ops mismatch, treating that too as an incompatibility.
> Do you mean that we should have fine-grained it further?

On second look, I think this particular check was already entirely 
redundant by the time I made the fwspec conversion to it, oh well. Since 
it remains harmless for the time being, let's just ignore it entirely 
until we can confidently say goodbye to the whole lot[1].

I don't think there's any need to differentiate an instance mismatch 
from a driver mismatch, once the latter becomes realistically possible, 
mostly due to iommu_domain_alloc() also having to become device-aware to 
know which driver to allocate from. Thus as far as a user is concerned, 
if attaching a device to an existing domain fails with -EMEDIUMTYPE, 
allocating a new domain using the given device, and attaching to that, 
can be expected to succeed, regardless of why the original attempt was 
rejected. In fact even in the theoretical different-driver-per-bus model 
the same principle still holds up.

Thanks,
Robin.

[1] 
https://gitlab.arm.com/linux-arm/linux-rm/-/commit/aa4accfa4a10e92daad0d51095918e8a89014393