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

[Nicolin Chen]

Hi Kevin,

On Wed, Jun 08, 2022 at 07:49:10AM +0000, Tian, Kevin wrote:
> External email: Use caution opening links or attachments
> 
> 
> > From: Nicolin Chen
> > Sent: Monday, June 6, 2022 2:19 PM
> >
> > Cases like VFIO wish to attach a device to an existing domain that was
> > not allocated specifically from the device. This raises a condition
> > where the IOMMU driver can fail the domain attach because the domain and
> > device are incompatible with each other.
> >
> > This is a soft failure that can be resolved by using a different domain.
> >
> > Provide a dedicated errno from the IOMMU driver during attach that the
> > reason attached failed is because of domain incompatability. EMEDIUMTYPE
> > is chosen because it is never used within the iommu subsystem today and
> > evokes a sense that the 'medium' aka the domain is incompatible.
> >
> > VFIO can use this to know attach is a soft failure and it should continue
> > searching. Otherwise the attach will be a hard failure and VFIO will
> > return the code to userspace.
> >
> > Update all drivers to return EMEDIUMTYPE in their failure paths that are
> > related to domain incompatability.
> 
> Seems not all drivers are converted, e.g.:

Thank you for going through all of them!

> mtk_iommu_v1_attach_device():
>         /* Only allow the domain created internally. */
>         mtk_mapping = data->mapping;
>         if (mtk_mapping->domain != domain)
>                 return 0;
> ** the current code sounds incorrect which should return an error

I agree.
 
> s390_iommu_attach_device():
>         /* Allow only devices with identical DMA range limits */
>         } else if (domain->geometry.aperture_start != zdev->start_dma ||
>                 domain->geometry.aperture_end != zdev->end_dma) {
>                 rc = -EINVAL;
>
> sprd_iommu_attach_device():
>         if (dom->sdev) {
>                 pr_err("There's already a device attached to this domain.\n");
>                 return -EINVAL;
>         }
> 
> 
> gart_iommu_attach_dev():
>         if (gart->active_domain && gart->active_domain != domain) {
>                 ret = -EBUSY;

Will add these.

> arm_smmu_attach_dev():
>         if (!fwspec || fwspec->ops != &arm_smmu_ops) {
>                 dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n");
>                 return -ENXIO;
>         }
> **probably this check can be covered by next patch which moves bus ops
> check into iommu core?

I was thinking that it could be covered. Yet, we are about to drop
that ops check, as Robin pointed out that we don't need that ops
sanity for we don't have mixed-driver systems yet. So perhaps this
would be a different ops check and should return -EMEDIUMTYPE too,
I think.

Thanks
Nic