| Message ID | 20220815181437.28127-2-nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Simplify vfio_iommu_type1 attach/detach routine | expand |
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. For that I prefer adding an additional pointer parameter to the attach functions in which the reason for the failure can be communicated up the chain. For the top-level iommu_attach_device() function I am okay with having a special version which has this additional paremter. Regards, Joerg
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" ? > For that I prefer adding an additional pointer parameter to the attach > functions in which the reason for the failure can be communicated up the > chain. That sounds like OS/2 :\ Jason
On Wed, Sep 07, 2022 at 10:47:39AM -0300, Jason Gunthorpe wrote: > Would you be happier if we wrote it like > > #define IOMMU_EINCOMPATIBLE_DEVICE xx > > Which tells "which of the function parameters is actually invalid" ? Having done some Rust hacking in the last months, I have to say I like to concept of error handling with Result<> there. Ideally we have a way to emulate that in our C code without having to change all callers. What I am proposing is a way this could be emulated here, but I am open to other suggestions. Still better than re-using random error codes for special purposes. Regards, Joerg
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. 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. Robin.
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
On Wed, Sep 07, 2022 at 04:06:29PM +0200, Joerg Roedel wrote: > On Wed, Sep 07, 2022 at 10:47:39AM -0300, Jason Gunthorpe wrote: > > Would you be happier if we wrote it like > > > > #define IOMMU_EINCOMPATIBLE_DEVICE xx > > > > Which tells "which of the function parameters is actually invalid" ? > > Having done some Rust hacking in the last months, I have to say I like > to concept of error handling with Result<> there. Ideally we have a way > to emulate that in our C code without having to change all callers. Sure, rust has all sorts of nice things. But the kernel doesn't follow rust idioms, and I don't think this is a great place to start experimenting with them. The unix/linux idiom is return an errno, and define what the errnos mean for your function. We have a long history of creatively applying the existing errnos, and sometimes we create new ones. We rarely return an errno and an additional error code because it doesn't fit the overall model, I can't return something like that through a system call, for instance. > What I am proposing is a way this could be emulated here, but I am open > to other suggestions. Still better than re-using random error codes for > special purposes. I think, in context of Linux as a project, it very much is worse to make up some rust-inspired error handing and discard the typical design patterns. Linux works because, for the most part, people follow similar design sensibilities throughout the tree. It has been 3 months since EMEDIUMTYPE was first proposed and 6 iterations of the series, don't you think it is a bit late in the game to try to experiment with rust error handling idioms? So, again, would you be happy with a simple #define IOMMU_EINCOMPATIBLE_DEVICE xx to make it less "re-using random error codes"? Thanks, Jason
On 2022-09-07 18:00, Jason Gunthorpe wrote: > 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.. Who said IOMMU instance? As a reminder, the patch I currently have[1] is matching the driver (via the device ops), which happens to be entirely compatible with drivers supporting cross-instance domains. Mostly because we already have drivers that support cross-instance domains and callers that use them. >> 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. 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. Once multiple drivers are in play, the only addition is that the "gatekeeper" check inside iommu_attach_group() may also return -EINVAL if the device is managed by a different driver, since that still fits the same "try again with a different domain" message to the caller. It's actually quite neat - basically the exact same thing we've tried to do with -EMEDIUMTYPE here, but more self-explanatory, since the fact is that a domain itself should never be invalid for attaching to via its own ops, and a group should never be inherently invalid for attaching to a suitable domain, it is only ever a particular combination of group (or device at the internal level) and domain that may not be valid together. 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. Whether iommu_attach_device() should also join the party and start rejecting non-singleton-group devices with a different error, or maintain its current behaviour since its legacy users already have their expectations set, is another matter in its own right. Cheers, Robin. [1] https://gitlab.arm.com/linux-arm/linux-rm/-/commit/683cdff1b2d4ae11f56e38d93b37e66e8c939fc9
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. 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
> 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: > > > 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. 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? > > > 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
> From: Tian, Kevin > Sent: Thursday, September 8, 2022 5:31 PM > > 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. > oops. Above is actually a right retry condition. gart is iommu instead of device. So in concept retrying gart->active_domain for the device could work. So please ignore this comment.
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
On Thu, Sep 08, 2022 at 09:30:57AM +0000, Tian, Kevin wrote: > > 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; Yes, this is was also on my mind with choosing an unpopular return code, it has a higher chance of not coming out of some other kernel API > 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. Given Joerg's objection I think we will do EINVAL and just live with the imperfection. It is not just slow path, but being inaccurate can mean extra domains are created when they were not needed. But I think we are getting into sufficiently unlikely territory that issue can be ignored to make progress. Jason
On Wed, Sep 07, 2022 at 02:10:33PM -0300, Jason Gunthorpe wrote: > Sure, rust has all sorts of nice things. But the kernel doesn't follow > rust idioms, and I don't think this is a great place to start > experimenting with them. It is actually a great place to start experimenting. The IOMMU interfaces are rather domain specific and if we get something wrong the damage is limited to a few callers. There are APIs much more exposed in the kernel which would be worse for that. But anyway, I am not insisting on it. > It has been 3 months since EMEDIUMTYPE was first proposed and 6 > iterations of the series, don't you think it is a bit late in the game > to try to experiment with rust error handling idioms? If I am not mistaken, I am the person who gets blamed when crappy IOMMU code is sent upstream. So it is also up to me to decide in which state and how close to merging a given patch series is an whether it is already 'late in the game'. > So, again, would you be happy with a simple > > #define IOMMU_EINCOMPATIBLE_DEVICE xx > > to make it less "re-using random error codes"? I am wondering if this can be solved by better defining what the return codes mean and adjust the call-back functions to match the definition. Something like: -ENODEV : Device not mapped my an IOMMU -EBUSY : Device attached and domain can not be changed -EINVAL : Device and domain are incompatible ... That would be much more intuitive than using something obscure like EMEDIUMTYPE. Regards, Joerg
On Thu, Sep 08, 2022 at 03:28:22PM +0200, Joerg Roedel wrote: > > It has been 3 months since EMEDIUMTYPE was first proposed and 6 > > iterations of the series, don't you think it is a bit late in the game > > to try to experiment with rust error handling idioms? > > If I am not mistaken, I am the person who gets blamed when crappy IOMMU > code is sent upstream. So it is also up to me to decide in which state > and how close to merging a given patch series is an whether it is > already 'late in the game'. I don't think the maintainer is the one who gets blamed. The community is responsible as a collective group for it's decisions. The maintainer is the leader of the community, responsible to foster it, and contributes their guidance, but doesn't bare an unlimited responsibility for what is merged. In a case like this I am the advocate, Nicolin wrote the patches, Kevin reviewed, Alex ack'd them - we as a group are ultimately responsible to repair, defend, or whatever is needed. > I am wondering if this can be solved by better defining what the return > codes mean and adjust the call-back functions to match the definition. > Something like: > > -ENODEV : Device not mapped my an IOMMU > -EBUSY : Device attached and domain can not be changed > -EINVAL : Device and domain are incompatible > ... Yes, this was gone over in a side thread the pros/cons, so lets do it. Nicolin will come with something along these lines. Thanks, Jason
On Thu, Sep 08, 2022 at 01:14:42PM -0300, Jason Gunthorpe wrote: > > I am wondering if this can be solved by better defining what the return > > codes mean and adjust the call-back functions to match the definition. > > Something like: > > > > -ENODEV : Device not mapped my an IOMMU > > -EBUSY : Device attached and domain can not be changed > > -EINVAL : Device and domain are incompatible > > ... > > Yes, this was gone over in a side thread the pros/cons, so lets do > it. Nicolin will come with something along these lines. I have started this effort by combining this list and the one from the side thread: @@ -266,6 +266,13 @@ struct iommu_ops { /** * struct iommu_domain_ops - domain specific operations * @attach_dev: attach an iommu domain to a device + * Rules of its return errno: + * ENOMEM - Out of memory + * EINVAL - Device and domain are incompatible + * EBUSY - Device is attached to a domain and cannot be changed + * ENODEV - Device or domain is messed up: device is not mapped + * to an IOMMU, no domain can attach, and etc. + * <others> - Same behavior as ENODEV, use is discouraged * @detach_dev: detach an iommu domain from a device * @map: map a physically contiguous memory region to an iommu domain * @map_pages: map a physically contiguous set of pages of the same size to I am now going through every single return value of ->attach_dev to make sure the list above applies. And I will also incorporate things like Robin's comments at the AMD IOMMU driver. And if the change occurs to be bigger, I guess that separating it to be an IOMMU series from this VFIO one might be better. Thanks Nic
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Friday, September 9, 2022 11:17 AM > > On Thu, Sep 08, 2022 at 01:14:42PM -0300, Jason Gunthorpe wrote: > > > > I am wondering if this can be solved by better defining what the return > > > codes mean and adjust the call-back functions to match the definition. > > > Something like: > > > > > > -ENODEV : Device not mapped my an IOMMU > > > -EBUSY : Device attached and domain can not be changed > > > -EINVAL : Device and domain are incompatible > > > ... > > > > Yes, this was gone over in a side thread the pros/cons, so lets do > > it. Nicolin will come with something along these lines. > > I have started this effort by combining this list and the one from > the side thread: > > @@ -266,6 +266,13 @@ struct iommu_ops { > /** > * struct iommu_domain_ops - domain specific operations > * @attach_dev: attach an iommu domain to a device > + * Rules of its return errno: > + * ENOMEM - Out of memory > + * EINVAL - Device and domain are incompatible > + * EBUSY - Device is attached to a domain and cannot be changed With this definition then probably @attach_dev should not return -EBUSY at all given it's already checked in the start of __iommu_attach_group(): if (group->domain && group->domain != group->default_domain && group->domain != group->blocking_domain) return -EBUSY; > + * ENODEV - Device or domain is messed up: device is not mapped > + * to an IOMMU, no domain can attach, and etc. if domain is messed up then should return -EINVAL given using another domain might just work. IMHO here -ENODEV should only cover device specific problems preventing this device from being attached to by any domain. > + * <others> - Same behavior as ENODEV, use is discouraged didn't get the "Same behavior" part. Does it suggest all other errnos should be converted to ENODEV? btw what about -ENOSPC? It's sane to allocate some resource in the attach path while the resource might be not available, e.g.: intel_iommu_attach_device() domain_add_dev_info() domain_attach_iommu(): int num, ret = -ENOSPC; ... ndomains = cap_ndoms(iommu->cap); num = find_first_zero_bit(iommu->domain_ids, ndomains); if (num >= ndomains) { pr_err("%s: No free domain ids\n", iommu->name); goto err_unlock; } As discussed in a side thread a note might be added to exempt calling kAPI outside of the iommu driver. > * @detach_dev: detach an iommu domain from a device > * @map: map a physically contiguous memory region to an iommu domain > * @map_pages: map a physically contiguous set of pages of the same size > to > > I am now going through every single return value of ->attach_dev to > make sure the list above applies. And I will also incorporate things > like Robin's comments at the AMD IOMMU driver. > > And if the change occurs to be bigger, I guess that separating it to > be an IOMMU series from this VFIO one might be better. > > Thanks > Nic
On Fri, Sep 09, 2022 at 05:00:16AM +0000, Tian, Kevin wrote: > > I have started this effort by combining this list and the one from > > the side thread: > > > > @@ -266,6 +266,13 @@ struct iommu_ops { > > /** > > * struct iommu_domain_ops - domain specific operations > > * @attach_dev: attach an iommu domain to a device > > + * Rules of its return errno: > > + * ENOMEM - Out of memory > > + * EINVAL - Device and domain are incompatible > > + * EBUSY - Device is attached to a domain and cannot be changed > > With this definition then probably @attach_dev should not return -EBUSY > at all given it's already checked in the start of __iommu_attach_group(): I think the EBUSY would be only for non-conforming drivers. The API semantic is you can always attach a new domain and replace an existing domain. So things like AMD's "can't do anything but idenitity on RID when PASID enabled" would be -EBUSY. Seems right that it should be rare though. > > + * ENODEV - Device or domain is messed up: device is not mapped > > + * to an IOMMU, no domain can attach, and etc. > > if domain is messed up then should return -EINVAL given using another domain > might just work. IMHO here -ENODEV should only cover device specific problems > preventing this device from being attached to by any domain. Agree > > + * <others> - Same behavior as ENODEV, use is discouraged > > didn't get the "Same behavior" part. Does it suggest all other errnos should > be converted to ENODEV? It says all other errnos should be treated as ENODEV by the caller but forwarded to userspace for further detail. > btw what about -ENOSPC? It's sane to allocate some resource in the attach > path while the resource might be not available, e.g.: Seems resaonable that it is similar to ENOMEM > As discussed in a side thread a note might be added to exempt calling > kAPI outside of the iommu driver. Sadly, not really.. The driver is responsible to santize this if it is relevant. It is the main downside of this approach. Jason
On Thu, Sep 08, 2022 at 09:08:38AM -0300, Jason Gunthorpe wrote: > On Thu, Sep 08, 2022 at 09:30:57AM +0000, Tian, Kevin wrote: > > 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; > Yes, this is was also on my mind with choosing an unpopular return > code, it has a higher chance of not coming out of some other kernel > API I wonder if it would be safe to just treat a pm_runtime_get_sync() failure as -ENODEV, since a PM resume() mostly occurs to the IOMMU that an IOMMU client/master device is behind, while an iommu_domain rarely intervenes.
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, September 9, 2022 8:08 PM > > > > As discussed in a side thread a note might be added to exempt calling > > kAPI outside of the iommu driver. > > Sadly, not really.. The driver is responsible to santize this if it is > relevant. It is the main downside of this approach. > Better provide a clarification on what sanitization means. e.g. I don't think we should change errno in those kAPIs to match the definition in iommu subsystem since e.g. -EINVAL really means different things in different context. So the sanitization in iommu driver is probably that: - If an external kAPI returns -EINVAL, convert it to -ENODEV given iommu domain is iommu internal object hence unlikely for external kAPIs to capture incompatibility issue between domain/device; - Otherwise just pass whatever returned to the caller, following the definition of "Same behavior as -ENODEV" above Thanks Kevin
> From: Nicolin Chen <nicolinc@nvidia.com> > Sent: Sunday, September 11, 2022 7:36 AM > > On Thu, Sep 08, 2022 at 09:08:38AM -0300, Jason Gunthorpe wrote: > > On Thu, Sep 08, 2022 at 09:30:57AM +0000, Tian, Kevin wrote: > > > > 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; > > > Yes, this is was also on my mind with choosing an unpopular return > > code, it has a higher chance of not coming out of some other kernel > > API > > I wonder if it would be safe to just treat a pm_runtime_get_sync() > failure as -ENODEV, since a PM resume() mostly occurs to the IOMMU > that an IOMMU client/master device is behind, while an iommu_domain > rarely intervenes. Yes, this is a condition preventing the device from being attached by a domain hence converting -EINVAL to -ENODEV probably makes sense. But as replied in another we might want to keep other errno's as is.
On Tue, Sep 13, 2022 at 02:22:17AM +0000, Tian, Kevin wrote: > External email: Use caution opening links or attachments > > > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Friday, September 9, 2022 8:08 PM > > > > > > > As discussed in a side thread a note might be added to exempt calling > > > kAPI outside of the iommu driver. > > > > Sadly, not really.. The driver is responsible to santize this if it is > > relevant. It is the main downside of this approach. > > > > Better provide a clarification on what sanitization means. > > e.g. I don't think we should change errno in those kAPIs to match the > definition in iommu subsystem since e.g. -EINVAL really means different > things in different context. > > So the sanitization in iommu driver is probably that: > > - If an external kAPI returns -EINVAL, convert it to -ENODEV given iommu > domain is iommu internal object hence unlikely for external kAPIs to > capture incompatibility issue between domain/device; > - Otherwise just pass whatever returned to the caller, following the definition > of "Same behavior as -ENODEV" above I added something similar. Thanks! diff --git a/include/linux/iommu.h b/include/linux/iommu.h index ea30f00dc145..190647d018f9 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -266,6 +266,17 @@ struct iommu_ops { /** * struct iommu_domain_ops - domain specific operations * @attach_dev: attach an iommu domain to a device + * Rules of its return errno: + * EINVAL - Exclusively, device and domain are incompatible. Must + * avoid kernel prints along with this errno. An EINVAL + * returned from a kAPI must be coverted to ENODEV if it + * is device specific, or to some other reasonable errno + * being listed below + * ENOMEM - Out of memory + * ENOSPC - No space left on device + * EBUSY - Device is attached to a domain and cannot be changed + * ENODEV - Device specific errors, not able to be attached + * <others> - Treated as ENODEV by the caller. Use is discouraged * @detach_dev: detach an iommu domain from a device * @map: map a physically contiguous memory region to an iommu domain * @map_pages: map a physically contiguous set of pages of the same size to
Hi Kevin, On Tue, Sep 13, 2022 at 02:24:27AM +0000, Tian, Kevin wrote: > > I wonder if it would be safe to just treat a pm_runtime_get_sync() > > failure as -ENODEV, since a PM resume() mostly occurs to the IOMMU > > that an IOMMU client/master device is behind, while an iommu_domain > > rarely intervenes. > > Yes, this is a condition preventing the device from being attached by > a domain hence converting -EINVAL to -ENODEV probably makes sense. > But as replied in another we might want to keep other errno's as is. Thanks for the reply and helps. I've sent a new IOMMU series: https://lore.kernel.org/linux-iommu/20220913082448.31120-1-nicolinc@nvidia.com/ Sorry I forgot to put you in CC, for I plainly copied the send list from the get_maintainer.pl script :( Hope you can still find these emails.
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 65b8e4fd8217..6a5bd0cfc06a 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -1755,7 +1755,7 @@ static int attach_device(struct device *dev, if (domain->flags & PD_IOMMUV2_MASK) { struct iommu_domain *def_domain = iommu_get_dma_domain(dev); - ret = -EINVAL; + ret = -EMEDIUMTYPE; if (def_domain->type != IOMMU_DOMAIN_IDENTITY) goto out; diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c index 1b1725759262..87f1829d24ea 100644 --- a/drivers/iommu/apple-dart.c +++ b/drivers/iommu/apple-dart.c @@ -495,10 +495,10 @@ static int apple_dart_attach_dev(struct iommu_domain *domain, if (cfg->stream_maps[0].dart->force_bypass && domain->type != IOMMU_DOMAIN_IDENTITY) - return -EINVAL; + return -EMEDIUMTYPE; if (!cfg->stream_maps[0].dart->supports_bypass && domain->type == IOMMU_DOMAIN_IDENTITY) - return -EINVAL; + return -EMEDIUMTYPE; ret = apple_dart_finalize_domain(domain, cfg); if (ret) diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index d32b02336411..a3717961d248 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2429,24 +2429,15 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) goto out_unlock; } } else if (smmu_domain->smmu != smmu) { - dev_err(dev, - "cannot attach to SMMU %s (upstream of %s)\n", - dev_name(smmu_domain->smmu->dev), - dev_name(smmu->dev)); - ret = -ENXIO; + ret = -EMEDIUMTYPE; goto out_unlock; } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && master->ssid_bits != smmu_domain->s1_cfg.s1cdmax) { - dev_err(dev, - "cannot attach to incompatible domain (%u SSID bits != %u)\n", - smmu_domain->s1_cfg.s1cdmax, master->ssid_bits); - ret = -EINVAL; + ret = -EMEDIUMTYPE; goto out_unlock; } else if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1 && smmu_domain->stall_enabled != master->stall_enabled) { - dev_err(dev, "cannot attach to stall-%s domain\n", - smmu_domain->stall_enabled ? "enabled" : "disabled"); - ret = -EINVAL; + ret = -EMEDIUMTYPE; goto out_unlock; } diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index dfa82df00342..26af6d923321 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1167,10 +1167,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) * different SMMUs. */ if (smmu_domain->smmu != smmu) { - dev_err(dev, - "cannot attach to SMMU %s whilst already attached to domain on SMMU %s\n", - dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev)); - ret = -EINVAL; + ret = -EMEDIUMTYPE; goto rpm_put; } diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c index 17235116d3bb..0daee6d8d993 100644 --- a/drivers/iommu/arm/arm-smmu/qcom_iommu.c +++ b/drivers/iommu/arm/arm-smmu/qcom_iommu.c @@ -381,13 +381,8 @@ static int qcom_iommu_attach_dev(struct iommu_domain *domain, struct device *dev * Sanity check the domain. We don't support domains across * different IOMMUs. */ - if (qcom_domain->iommu != qcom_iommu) { - dev_err(dev, "cannot attach to IOMMU %s while already " - "attached to domain on IOMMU %s\n", - dev_name(qcom_domain->iommu->dev), - dev_name(qcom_iommu->dev)); - return -EINVAL; - } + if (qcom_domain->iommu != qcom_iommu) + return -EMEDIUMTYPE; return 0; } diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index 7cca030a508e..560d2c3e9304 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4174,19 +4174,15 @@ static int prepare_domain_attach_device(struct iommu_domain *domain, return -ENODEV; if (dmar_domain->force_snooping && !ecap_sc_support(iommu->ecap)) - return -EOPNOTSUPP; + return -EMEDIUMTYPE; /* check if this iommu agaw is sufficient for max mapped address */ addr_width = agaw_to_width(iommu->agaw); if (addr_width > cap_mgaw(iommu->cap)) addr_width = cap_mgaw(iommu->cap); - if (dmar_domain->max_addr > (1LL << addr_width)) { - dev_err(dev, "%s: iommu width (%d) is not " - "sufficient for the mapped address (%llx)\n", - __func__, addr_width, dmar_domain->max_addr); - return -EFAULT; - } + if (dmar_domain->max_addr > (1LL << addr_width)) + return -EMEDIUMTYPE; dmar_domain->gaw = addr_width; /* diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 780fb7071577..d7416f6d9bab 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1975,6 +1975,20 @@ static int __iommu_attach_device(struct iommu_domain *domain, return ret; } +/** + * iommu_attach_device - Attach a device to an IOMMU domain + * @domain: IOMMU domain to attach + * @dev: Device that will be attached + * + * Returns 0 on success and error code on failure + * + * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and + * the device are incompatible in some way. This indicates that a caller should + * try another existing IOMMU domain or allocate a new one. And note that it's + * recommended to keep kernel print free when reporting -EMEDIUMTYPE error, as + * this function can be called to test compatibility with domains that will fail + * the test, which will result in a kernel log spam. + */ int iommu_attach_device(struct iommu_domain *domain, struct device *dev) { struct iommu_group *group; @@ -2101,6 +2115,20 @@ static int __iommu_attach_group(struct iommu_domain *domain, return ret; } +/** + * iommu_attach_group - Attach an IOMMU group to an IOMMU domain + * @domain: IOMMU domain to attach + * @group: IOMMU group that will be attached + * + * Returns 0 on success and error code on failure + * + * Specifically, -EMEDIUMTYPE is returned as a soft failure if the domain and + * the device are incompatible in some way. This indicates that a caller should + * try another existing IOMMU domain or allocate a new one. And note that it's + * recommended to keep kernel print free when reporting -EMEDIUMTYPE error, as + * this function can be called to test compatibility with domains that will fail + * the test, which will result in a kernel log spam. + */ int iommu_attach_group(struct iommu_domain *domain, struct iommu_group *group) { int ret; diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c index 1d42084d0276..0103480648cb 100644 --- a/drivers/iommu/ipmmu-vmsa.c +++ b/drivers/iommu/ipmmu-vmsa.c @@ -628,9 +628,7 @@ static int ipmmu_attach_device(struct iommu_domain *io_domain, * Something is wrong, we can't attach two devices using * different IOMMUs to the same domain. */ - dev_err(dev, "Can't attach IPMMU %s to domain on IPMMU %s\n", - dev_name(mmu->dev), dev_name(domain->mmu->dev)); - ret = -EINVAL; + ret = -EMEDIUMTYPE; } else dev_info(dev, "Reusing IPMMU context %u\n", domain->context_id); diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index d9cf2820c02e..6bc8925726bf 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -1471,8 +1471,7 @@ omap_iommu_attach_dev(struct iommu_domain *domain, struct device *dev) /* only a single client device can be attached to a domain */ if (omap_domain->dev) { - dev_err(dev, "iommu domain is already attached\n"); - ret = -EBUSY; + ret = -EMEDIUMTYPE; goto out; } diff --git a/drivers/iommu/s390-iommu.c b/drivers/iommu/s390-iommu.c index c898bcbbce11..ddcb78b284bb 100644 --- a/drivers/iommu/s390-iommu.c +++ b/drivers/iommu/s390-iommu.c @@ -127,7 +127,7 @@ static int s390_iommu_attach_device(struct iommu_domain *domain, /* 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; + rc = -EMEDIUMTYPE; spin_unlock_irqrestore(&s390_domain->list_lock, flags); goto out_restore; } diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c index 511959c8a14d..2bc1d34981cc 100644 --- a/drivers/iommu/sprd-iommu.c +++ b/drivers/iommu/sprd-iommu.c @@ -237,10 +237,8 @@ static int sprd_iommu_attach_device(struct iommu_domain *domain, struct sprd_iommu_domain *dom = to_sprd_domain(domain); size_t pgt_size = sprd_iommu_pgt_size(domain); - if (dom->sdev) { - pr_err("There's already a device attached to this domain.\n"); - return -EINVAL; - } + if (dom->sdev) + return -EMEDIUMTYPE; dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL); if (!dom->pgt_va) diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c index e5ca3cf1a949..9d81cc467651 100644 --- a/drivers/iommu/tegra-gart.c +++ b/drivers/iommu/tegra-gart.c @@ -112,7 +112,7 @@ static int gart_iommu_attach_dev(struct iommu_domain *domain, spin_lock(&gart->dom_lock); if (gart->active_domain && gart->active_domain != domain) { - ret = -EBUSY; + ret = -EMEDIUMTYPE; } else if (dev_iommu_priv_get(dev) != domain) { dev_iommu_priv_set(dev, domain); gart->active_domain = domain; diff --git a/drivers/iommu/virtio-iommu.c b/drivers/iommu/virtio-iommu.c index 08eeafc9529f..6172763c69b8 100644 --- a/drivers/iommu/virtio-iommu.c +++ b/drivers/iommu/virtio-iommu.c @@ -733,8 +733,7 @@ static int viommu_attach_dev(struct iommu_domain *domain, struct device *dev) */ ret = viommu_domain_finalise(vdev, domain); } else if (vdomain->viommu != vdev->viommu) { - dev_err(dev, "cannot attach to foreign vIOMMU\n"); - ret = -EXDEV; + ret = -EMEDIUMTYPE; } mutex_unlock(&vdomain->mutex);