RE: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> From: Nicolin Chen
> 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 +, 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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> From: Jason Gunthorpe > 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> From: Nicolin Chen
> 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.
> + * - 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
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [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 - 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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org
RE: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> 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.
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
RE: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> From: Jason Gunthorpe
> 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
> - 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
Re: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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
Re: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
Re: [PATCH v6 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
