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

From: Jason Gunthorpe <jgg@nvidia.com>
Sent: Thursday, September 8, 2022 8:43 AM

On Wed, Sep 07, 2022 at 08:41:13PM +0100, Robin Murphy wrote:

quoted

Again, not what I was suggesting. In fact the nature of

iommu_attach_group()

quoted

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

quoted

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"

quoted

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.

btw I saw the policy for -EBUSY is also not consistent in this series.

while it's correct to change -EBUSY to -EMEDIUMTYPE for omap, assuming
that retrying another fresh domain for the said device should work:

	if (omap_domain->dev) {
-		dev_err(dev, "iommu domain is already attached\n");
-		ret = -EBUSY;
+		ret = -EMEDIUMTYPE;
 		goto out;
 	}

the change in tegra-gart doesn't sound correct:

	if (gart->active_domain && gart->active_domain != domain) {
-		ret = -EBUSY;
+		ret = -EMEDIUMTYPE;

one device cannot be attached to two domains. This fact doesn't change
no matter how many domains are tried. In concept this check is
redundant and should have been done by iommu core, but obviously we
didn't pay attention to what -EBUSY actually represents in this path.

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.

There are also cases where common kAPIs are called in the attach
path which may return -EINVAL and random errno, e.g.:

omap_iommu_attach_dev()
  omap_iommu_attach()
    iommu_enable()
      pm_runtime_get_sync()
        __pm_runtime_resume()
          rpm_resume()
	if (dev->power.runtime_error) {
		retval = -EINVAL;
            
viommu_attach_dev()
  viommu_domain_finalise()
    ida_alloc_range()
	if ((int)min < 0)
		return -ENOSPC;

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?

quoted

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.

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.

I don't see an elegant option here.

If we care about accuracy of reporting incompatibility -EMEDIUMTYPE
is obviously a better option.

If we think attach_dev is a slow path and having unnecessary retries
doesn't hurt then -EINVAL sounds a simpler option. We probably can
just go using -EINVAL as retry indicator in vfio even w/o changing
iommu drivers at this point. Then improve them to use consistent
errno gradually and in a separate effort.

Thanks
Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel