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

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Sep 07, 2022 at 03:23:09PM +0100, Robin Murphy wrote:
> On 2022-09-07 14:47, Jason Gunthorpe wrote:
> > On Wed, Sep 07, 2022 at 02:41:54PM +0200, Joerg Roedel wrote:
> > > On Mon, Aug 15, 2022 at 11:14:33AM -0700, Nicolin Chen wrote:
> > > > 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.
> > > 
> > > I am not a fan of re-using EMEDIUMTYPE or any other special value. What
> > > is needed here in EINVAL, but with a way to tell the caller which of the
> > > function parameters is actually invalid.
> > 
> > Using errnos to indicate the nature of failure is a well established
> > unix practice, it is why we have hundreds of error codes and don't
> > just return -EINVAL for everything.
> > 
> > What don't you like about it?
> > 
> > Would you be happier if we wrote it like
> > 
> >   #define IOMMU_EINCOMPATIBLE_DEVICE xx
> > 
> > Which tells "which of the function parameters is actually invalid" ?
> 
> FWIW, we're now very close to being able to validate dev->iommu against
> where the domain came from in core code, and so short-circuit ->attach_dev
> entirely if they don't match. 

I don't think this is a long term direction. We have systems now with
a number of SMMU blocks and we really are going to see a need that
they share the iommu_domains so we don't have unncessary overheads
from duplicated io page table memory.

So ultimately I'd expect to pass the iommu_domain to the driver and
the driver will decide if the page table memory it represents is
compatible or not. Restricting to only the same iommu instance isn't
good..

> At that point -EINVAL at the driver callback level could be assumed
> to refer to the domain argument, while anything else could be taken
> as something going unexpectedly wrong when the attach may otherwise
> have worked. I've forgotten if we actually had a valid case anywhere
> for "this is my device but even if you retry with a different domain
> it's still never going to work", but I think we wouldn't actually
> need that anyway - it should be clear enough to a caller that if
> attaching to an existing domain fails, then allocating a fresh
> domain and attaching also fails, that's the point to give up.

The point was to have clear error handling, we either have permenent
errors or 'this domain will never work with this device error'.

If we treat all error as temporary and just retry randomly it can
create a mess. For instance we might fail to attach to a perfectly
compatible domain due to ENOMEM or something and then go on to
successfully a create a new 2nd domain, just due to races.

We can certainly code the try everything then allocate scheme, it is
just much more fragile than having definitive error codes.

Jason