Robin Murphy
2022-Sep-08 10:25 UTC
[PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
On 2022-09-08 01:43, Jason Gunthorpe wrote:> On Wed, Sep 07, 2022 at 08:41:13PM +0100, Robin Murphy wrote: > >>>> 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.. >> >> Who said IOMMU instance? > > Ah, I completely misunderstood what 'dev->iommu' was referring too, OK > I see. > >> Again, not what I was suggesting. In fact the nature of iommu_attach_group() >> already rules out bogus devices getting this far, so all a driver currently >> has to worry about is compatibility of a device that it definitely probed >> with a domain that it definitely allocated. Therefore, from a caller's point >> of view, if attaching to an existing domain returns -EINVAL, try another >> domain; multiple different existing domains can be tried, and may also >> return -EINVAL for the same or different reasons; the final attempt is to >> allocate a fresh domain and attach to that, which should always be nominally >> valid and *never* return -EINVAL. If any attempt returns any other error, >> bail out down the usual "this should have worked but something went wrong" >> path. Even if any driver did have a nonsensical "nothing went wrong, I just >> can't attach my device to any of my domains" case, I don't think it would >> really need distinguishing from any other general error anyway. > > The algorithm you described is exactly what this series does, it just > used EMEDIUMTYPE instead of EINVAL. Changing it to EINVAL is not a > fundamental problem, just a bit more work. > > Looking at Nicolin's series there is a bunch of existing errnos that > would still need converting, ie EXDEV, EBUSY, EOPNOTSUPP, EFAULT, and > ENXIO are all returned as codes for 'domain incompatible with device' > in various drivers. So the patch would still look much the same, just > changing them to EINVAL instead of EMEDIUMTYPE. > > That leaves the question of the remaining EINVAL's that Nicolin did > not convert to EMEDIUMTYPE. > > eg in the AMD driver: > > if (!check_device(dev)) > return -EINVAL; > > iommu = rlookup_amd_iommu(dev); > if (!iommu) > return -EINVAL; > > These are all cases of 'something is really wrong with the device or > iommu, everything will fail'. Other drivers are using ENODEV for this > already, so we'd probably have an additional patch changing various > places like that to ENODEV. > > This mixture of error codes is the basic reason why a new code was > used, because none of the existing codes are used with any > consistency. > > But OK, I'm on board, lets use more common errnos with specific > meaning, that can be documented in a comment someplace: > ENOMEM - out of memory > ENODEV - no domain can attach, device or iommu is messed up > EINVAL - the domain is incompatible with the device > <others> - Same behavior as ENODEV, use is discouraged. > > I think achieving consistency of error codes is a generally desirable > goal, it makes the error code actually useful. > > Joerg this is a good bit of work, will you be OK with it? > >> Thus as long as we can maintain that basic guarantee that attaching >> a group to a newly allocated domain can only ever fail for resource >> allocation reasons and not some spurious "incompatibility", then we >> don't need any obscure trickery, and a single, clear, error code is >> in fact enough to say all that needs to be said. > > As above, this is not the case, drivers do seem to have error paths > that are unconditional on the domain. Perhaps they are just protective > assertions and never happen.Right, that's the gist of what I was getting at - I think it's worth putting in the effort to audit and fix the drivers so that that *can* be the case, then we can have a meaningful error API with standard codes effectively for free, rather than just sighing at the existing mess and building a slightly esoteric special case on top. Case in point, the AMD checks quoted above are pointless, since it checks the same things in ->probe_device, and if that fails then the device won't get a group so there's no way for it to even reach ->attach_dev any more. I'm sure there's a *lot* of cruft that can be cleared out now that per-device and per-domain ops give us this kind of inherent robustness. Cheers, Robin.> Regardless, it doesn't matter. If they return ENODEV or EINVAL the > VFIO side algorithm will continue to work fine, it just does alot more > work if EINVAL is permanently returned. > > Thanks, > Jason