| Message ID | 20220623200029.26007-2-nicolinc@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Simplify vfio_iommu_type1 attach/detach routine | expand |
On 2022/6/24 04:00, Nicolin Chen wrote: > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > index e1cb51b9866c..5386d889429d 100644 > --- a/drivers/iommu/mtk_iommu_v1.c > +++ b/drivers/iommu/mtk_iommu_v1.c > @@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device > /* Only allow the domain created internally. */ > mtk_mapping = data->mapping; > if (mtk_mapping->domain != domain) > - return 0; > + return -EMEDIUMTYPE; > > if (!data->m4u_dom) { > data->m4u_dom = dom; This change looks odd. It turns the return value from success to failure. Is it a bug? If so, it should go through a separated fix patch. Best regards, baolu
On Fri, Jun 24, 2022 at 09:35:49AM +0800, Baolu Lu wrote: > External email: Use caution opening links or attachments > > > On 2022/6/24 04:00, Nicolin Chen wrote: > > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > > index e1cb51b9866c..5386d889429d 100644 > > --- a/drivers/iommu/mtk_iommu_v1.c > > +++ b/drivers/iommu/mtk_iommu_v1.c > > @@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device > > /* Only allow the domain created internally. */ > > mtk_mapping = data->mapping; > > if (mtk_mapping->domain != domain) > > - return 0; > > + return -EMEDIUMTYPE; > > > > if (!data->m4u_dom) { > > data->m4u_dom = dom; > > This change looks odd. It turns the return value from success to > failure. Is it a bug? If so, it should go through a separated fix patch. Makes sense. I read the commit log of the original change: https://lore.kernel.org/r/1589530123-30240-1-git-send-email-yong.wu@mediatek.com It doesn't seem to allow devices to get attached to different domains other than the shared mapping->domain, created in the in the mtk_iommu_probe_device(). So it looks like returning 0 is intentional. Though I am still very confused by this return value here, I doubt it has ever been used in a VFIO context. Young, would you please give us some input? Overall, I feel it's better to play it safe here by dropping this part. If we later confirm there is a need to fix it, we will do that in a separate patch anyway. Thanks Nic
On Thu, 2022-06-23 at 19:44 -0700, Nicolin Chen wrote: > On Fri, Jun 24, 2022 at 09:35:49AM +0800, Baolu Lu wrote: > > External email: Use caution opening links or attachments > > > > > > On 2022/6/24 04:00, Nicolin Chen wrote: > > > diff --git a/drivers/iommu/mtk_iommu_v1.c > > > b/drivers/iommu/mtk_iommu_v1.c > > > index e1cb51b9866c..5386d889429d 100644 > > > --- a/drivers/iommu/mtk_iommu_v1.c > > > +++ b/drivers/iommu/mtk_iommu_v1.c > > > @@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct > > > iommu_domain *domain, struct device > > > /* Only allow the domain created internally. */ > > > mtk_mapping = data->mapping; > > > if (mtk_mapping->domain != domain) > > > - return 0; > > > + return -EMEDIUMTYPE; > > > > > > if (!data->m4u_dom) { > > > data->m4u_dom = dom; > > > > This change looks odd. It turns the return value from success to > > failure. Is it a bug? If so, it should go through a separated fix > > patch. Thanks for the review:) > > Makes sense. > > I read the commit log of the original change: > https://lore.kernel.org/r/1589530123-30240-1-git-send-email-yong.wu@mediatek.com > > It doesn't seem to allow devices to get attached to different > domains other than the shared mapping->domain, created in the > in the mtk_iommu_probe_device(). So it looks like returning 0 > is intentional. Though I am still very confused by this return > value here, I doubt it has ever been used in a VFIO context. It's not used in VFIO context. "return 0" just satisfy the iommu framework to go ahead. and yes, here we only allow the shared "mapping- >domain" (All the devices share a domain created internally). thus I think we should still keep "return 0" here. Thanks:) > > Young, would you please give us some input? > > Overall, I feel it's better to play it safe here by dropping > this part. If we later confirm there is a need to fix it, we > will do that in a separate patch anyway. > > Thanks > Nic
On Fri, Jun 24, 2022 at 01:38:58PM +0800, Yong Wu wrote: > > > > diff --git a/drivers/iommu/mtk_iommu_v1.c > > > > b/drivers/iommu/mtk_iommu_v1.c > > > > index e1cb51b9866c..5386d889429d 100644 > > > > --- a/drivers/iommu/mtk_iommu_v1.c > > > > +++ b/drivers/iommu/mtk_iommu_v1.c > > > > @@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct > > > > iommu_domain *domain, struct device > > > > /* Only allow the domain created internally. */ > > > > mtk_mapping = data->mapping; > > > > if (mtk_mapping->domain != domain) > > > > - return 0; > > > > + return -EMEDIUMTYPE; > > > > > > > > if (!data->m4u_dom) { > > > > data->m4u_dom = dom; > > > > > > This change looks odd. It turns the return value from success to > > > failure. Is it a bug? If so, it should go through a separated fix > > > patch. > > Thanks for the review:) > > > > > Makes sense. > > > > I read the commit log of the original change: > > > https://lore.kernel.org/r/1589530123-30240-1-git-send-email-yong.wu@mediatek.com > > > > It doesn't seem to allow devices to get attached to different > > domains other than the shared mapping->domain, created in the > > in the mtk_iommu_probe_device(). So it looks like returning 0 > > is intentional. Though I am still very confused by this return > > value here, I doubt it has ever been used in a VFIO context. > > It's not used in VFIO context. "return 0" just satisfy the iommu > framework to go ahead. and yes, here we only allow the shared "mapping- > >domain" (All the devices share a domain created internally). > > thus I think we should still keep "return 0" here. Thanks for the reply. I will just drop the change of this file.
> From: Yong Wu > Sent: Friday, June 24, 2022 1:39 PM > > On Thu, 2022-06-23 at 19:44 -0700, Nicolin Chen wrote: > > On Fri, Jun 24, 2022 at 09:35:49AM +0800, Baolu Lu wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On 2022/6/24 04:00, Nicolin Chen wrote: > > > > diff --git a/drivers/iommu/mtk_iommu_v1.c > > > > b/drivers/iommu/mtk_iommu_v1.c > > > > index e1cb51b9866c..5386d889429d 100644 > > > > --- a/drivers/iommu/mtk_iommu_v1.c > > > > +++ b/drivers/iommu/mtk_iommu_v1.c > > > > @@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct > > > > iommu_domain *domain, struct device > > > > /* Only allow the domain created internally. */ > > > > mtk_mapping = data->mapping; > > > > if (mtk_mapping->domain != domain) > > > > - return 0; > > > > + return -EMEDIUMTYPE; > > > > > > > > if (!data->m4u_dom) { > > > > data->m4u_dom = dom; > > > > > > This change looks odd. It turns the return value from success to > > > failure. Is it a bug? If so, it should go through a separated fix > > > patch. > > Thanks for the review:) > > > > > Makes sense. > > > > I read the commit log of the original change: > > > https://lore.kernel.org/r/1589530123-30240-1-git-send-email- > yong.wu@mediatek.com > > > > It doesn't seem to allow devices to get attached to different > > domains other than the shared mapping->domain, created in the > > in the mtk_iommu_probe_device(). So it looks like returning 0 > > is intentional. Though I am still very confused by this return > > value here, I doubt it has ever been used in a VFIO context. > > It's not used in VFIO context. "return 0" just satisfy the iommu > framework to go ahead. and yes, here we only allow the shared "mapping- > >domain" (All the devices share a domain created internally). > > thus I think we should still keep "return 0" here. > What prevent this driver from being used in VFIO context? and why would we want to go ahead when an obvious error occurs i.e. when a device is attached to an unexpected domain?
On Fri, 2022-06-24 at 06:16 +0000, Tian, Kevin wrote: > > From: Yong Wu > > Sent: Friday, June 24, 2022 1:39 PM > > > > On Thu, 2022-06-23 at 19:44 -0700, Nicolin Chen wrote: > > > On Fri, Jun 24, 2022 at 09:35:49AM +0800, Baolu Lu wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On 2022/6/24 04:00, Nicolin Chen wrote: > > > > > diff --git a/drivers/iommu/mtk_iommu_v1.c > > > > > b/drivers/iommu/mtk_iommu_v1.c > > > > > index e1cb51b9866c..5386d889429d 100644 > > > > > --- a/drivers/iommu/mtk_iommu_v1.c > > > > > +++ b/drivers/iommu/mtk_iommu_v1.c > > > > > @@ -304,7 +304,7 @@ static int > > > > > mtk_iommu_v1_attach_device(struct > > > > > iommu_domain *domain, struct device > > > > > /* Only allow the domain created internally. */ > > > > > mtk_mapping = data->mapping; > > > > > if (mtk_mapping->domain != domain) > > > > > - return 0; > > > > > + return -EMEDIUMTYPE; > > > > > > > > > > if (!data->m4u_dom) { > > > > > data->m4u_dom = dom; > > > > > > > > This change looks odd. It turns the return value from success > > > > to > > > > failure. Is it a bug? If so, it should go through a separated > > > > fix > > > > patch. > > > > Thanks for the review:) > > > > > > > > Makes sense. > > > > > > I read the commit log of the original change: > > > > > > > https://lore.kernel.org/r/1589530123-30240-1-git-send-email- > > yong.wu@mediatek.com > > > > > > It doesn't seem to allow devices to get attached to different > > > domains other than the shared mapping->domain, created in the > > > in the mtk_iommu_probe_device(). So it looks like returning 0 > > > is intentional. Though I am still very confused by this return > > > value here, I doubt it has ever been used in a VFIO context. > > > > It's not used in VFIO context. "return 0" just satisfy the iommu > > framework to go ahead. and yes, here we only allow the shared > > "mapping- > > > domain" (All the devices share a domain created internally). > > > > thus I think we should still keep "return 0" here. > > > > What prevent this driver from being used in VFIO context? Nothing prevent this. Just I didn't test. mtk_iommu_v1.c only is used in mt2701 and there is no VFIO scenario. I'm not sure if it supports VFIO. (mtk_iommu.c support VFIO.) > and why would we want to go ahead when an obvious error occurs > i.e. when a device is attached to an unexpected domain? The iommu flow in this file always is a bit odd as we need share iommu domain in ARM32. As I tested before in the above link, "The iommu framework will create a iommu domain for each a device.", therefore we have to *workaround* in this file. And this was expected to be fixed by: https://lore.kernel.org/linux-iommu/cover.1597931875.git.robin.murphy@arm.com/ sorry, I don't know its current status. Thanks.
On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: > > > It's not used in VFIO context. "return 0" just satisfy the iommu > > > framework to go ahead. and yes, here we only allow the shared > > > "mapping-domain" (All the devices share a domain created > > > internally). What part of the iommu framework is trying to attach a domain and wants to see success when the domain was not actually attached ? > > What prevent this driver from being used in VFIO context? > > Nothing prevent this. Just I didn't test. This is why it is wrong to return success here. Jason
On Fri, Jun 24, 2022 at 03:19:43PM -0300, Jason Gunthorpe wrote: > On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: > > > > > It's not used in VFIO context. "return 0" just satisfy the iommu > > > > framework to go ahead. and yes, here we only allow the shared > > > > "mapping-domain" (All the devices share a domain created > > > > internally). > > What part of the iommu framework is trying to attach a domain and > wants to see success when the domain was not actually attached ? > > > > What prevent this driver from being used in VFIO context? > > > > Nothing prevent this. Just I didn't test. > > This is why it is wrong to return success here. Hi Yong, would you or someone you know be able to confirm whether this "return 0" is still a must or not? Considering that it's an old 32-bit platform for MTK, if it would take time to do so, I'd like to drop the change in MTK driver and note in commit log for you or other MTK folks to change in future. Thanks Nic
On 2022-06-29 20:47, Nicolin Chen wrote: > On Fri, Jun 24, 2022 at 03:19:43PM -0300, Jason Gunthorpe wrote: >> On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: >> >>>>> It's not used in VFIO context. "return 0" just satisfy the iommu >>>>> framework to go ahead. and yes, here we only allow the shared >>>>> "mapping-domain" (All the devices share a domain created >>>>> internally). >> >> What part of the iommu framework is trying to attach a domain and >> wants to see success when the domain was not actually attached ? >> >>>> What prevent this driver from being used in VFIO context? >>> >>> Nothing prevent this. Just I didn't test. >> >> This is why it is wrong to return success here. > > Hi Yong, would you or someone you know be able to confirm whether > this "return 0" is still a must or not? From memory, it is unfortunately required, due to this driver being in the rare position of having to support multiple devices in a single address space on 32-bit ARM. Since the old ARM DMA code doesn't understand groups, the driver sets up its own canonical dma_iommu_mapping to act like a default domain, but then has to politely say "yeah OK" to arm_setup_iommu_dma_ops() for each device so that they do all end up with the right DMA ops rather than dying in screaming failure (the ARM code's per-device mappings then get leaked, but we can't really do any better). The whole mess disappears in the proper default domain conversion, but in the meantime, it's still safe to assume that nobody's doing VFIO with embedded display/video codec/etc. blocks that don't even have reset drivers. Thanks, Robin.
On Wed, 2022-06-29 at 12:47 -0700, Nicolin Chen wrote: > On Fri, Jun 24, 2022 at 03:19:43PM -0300, Jason Gunthorpe wrote: > > On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: > > > > > > > It's not used in VFIO context. "return 0" just satisfy the > > > > > iommu > > > > > framework to go ahead. and yes, here we only allow the shared > > > > > "mapping-domain" (All the devices share a domain created > > > > > internally). > > > > What part of the iommu framework is trying to attach a domain and > > wants to see success when the domain was not actually attached ? > > > > > > What prevent this driver from being used in VFIO context? > > > > > > Nothing prevent this. Just I didn't test. > > > > This is why it is wrong to return success here. > > Hi Yong, would you or someone you know be able to confirm whether > this "return 0" is still a must or not? > > Considering that it's an old 32-bit platform for MTK, if it would > take time to do so, I'd like to drop the change in MTK driver and > note in commit log for you or other MTK folks to change in future. Yes. Please help drop the change in this file. Sorry I don't have the board at hand right now and I could not list the backtrace where this is needed(should be bus_iommu_probe from the previous debug...) > > Thanks > Nic
> From: Robin Murphy <robin.murphy@arm.com> > Sent: Thursday, June 30, 2022 4:22 PM > > On 2022-06-29 20:47, Nicolin Chen wrote: > > On Fri, Jun 24, 2022 at 03:19:43PM -0300, Jason Gunthorpe wrote: > >> On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: > >> > >>>>> It's not used in VFIO context. "return 0" just satisfy the iommu > >>>>> framework to go ahead. and yes, here we only allow the shared > >>>>> "mapping-domain" (All the devices share a domain created > >>>>> internally). > >> > >> What part of the iommu framework is trying to attach a domain and > >> wants to see success when the domain was not actually attached ? > >> > >>>> What prevent this driver from being used in VFIO context? > >>> > >>> Nothing prevent this. Just I didn't test. > >> > >> This is why it is wrong to return success here. > > > > Hi Yong, would you or someone you know be able to confirm whether > > this "return 0" is still a must or not? > > From memory, it is unfortunately required, due to this driver being in > the rare position of having to support multiple devices in a single > address space on 32-bit ARM. Since the old ARM DMA code doesn't > understand groups, the driver sets up its own canonical > dma_iommu_mapping to act like a default domain, but then has to politely > say "yeah OK" to arm_setup_iommu_dma_ops() for each device so that they > do all end up with the right DMA ops rather than dying in screaming > failure (the ARM code's per-device mappings then get leaked, but we > can't really do any better). > > The whole mess disappears in the proper default domain conversion, but > in the meantime, it's still safe to assume that nobody's doing VFIO with > embedded display/video codec/etc. blocks that don't even have reset drivers. > Probably above is worth a comment in mtk code so we don't need always dig it out from memory when similar question arises in the the future.
On Thu, Jun 30, 2022 at 05:33:16PM +0800, Yong Wu wrote: > External email: Use caution opening links or attachments > > > On Wed, 2022-06-29 at 12:47 -0700, Nicolin Chen wrote: > > On Fri, Jun 24, 2022 at 03:19:43PM -0300, Jason Gunthorpe wrote: > > > On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: > > > > > > > > > It's not used in VFIO context. "return 0" just satisfy the > > > > > > iommu > > > > > > framework to go ahead. and yes, here we only allow the shared > > > > > > "mapping-domain" (All the devices share a domain created > > > > > > internally). > > > > > > What part of the iommu framework is trying to attach a domain and > > > wants to see success when the domain was not actually attached ? > > > > > > > > What prevent this driver from being used in VFIO context? > > > > > > > > Nothing prevent this. Just I didn't test. > > > > > > This is why it is wrong to return success here. > > > > Hi Yong, would you or someone you know be able to confirm whether > > this "return 0" is still a must or not? > > > > Considering that it's an old 32-bit platform for MTK, if it would > > take time to do so, I'd like to drop the change in MTK driver and > > note in commit log for you or other MTK folks to change in future. > > Yes. Please help drop the change in this file. > > Sorry I don't have the board at hand right now and I could not list the > backtrace where this is needed(should be bus_iommu_probe from the > previous debug...) OK. Thanks for the reply.
On Thu, Jun 30, 2022 at 09:21:42AM +0100, Robin Murphy wrote: > External email: Use caution opening links or attachments > > > On 2022-06-29 20:47, Nicolin Chen wrote: > > On Fri, Jun 24, 2022 at 03:19:43PM -0300, Jason Gunthorpe wrote: > > > On Fri, Jun 24, 2022 at 06:35:49PM +0800, Yong Wu wrote: > > > > > > > > > It's not used in VFIO context. "return 0" just satisfy the iommu > > > > > > framework to go ahead. and yes, here we only allow the shared > > > > > > "mapping-domain" (All the devices share a domain created > > > > > > internally). > > > > > > What part of the iommu framework is trying to attach a domain and > > > wants to see success when the domain was not actually attached ? > > > > > > > > What prevent this driver from being used in VFIO context? > > > > > > > > Nothing prevent this. Just I didn't test. > > > > > > This is why it is wrong to return success here. > > > > Hi Yong, would you or someone you know be able to confirm whether > > this "return 0" is still a must or not? > > From memory, it is unfortunately required, due to this driver being in > the rare position of having to support multiple devices in a single > address space on 32-bit ARM. Since the old ARM DMA code doesn't > understand groups, the driver sets up its own canonical > dma_iommu_mapping to act like a default domain, but then has to politely > say "yeah OK" to arm_setup_iommu_dma_ops() for each device so that they > do all end up with the right DMA ops rather than dying in screaming > failure (the ARM code's per-device mappings then get leaked, but we > can't really do any better). > > The whole mess disappears in the proper default domain conversion, but > in the meantime, it's still safe to assume that nobody's doing VFIO with > embedded display/video codec/etc. blocks that don't even have reset drivers. Thanks for the input! I'll just respin it by dropping mtk_v1 diff. Nic
diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c index 840831d5d2ad..ad499658a6b6 100644 --- a/drivers/iommu/amd/iommu.c +++ b/drivers/iommu/amd/iommu.c @@ -1662,7 +1662,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 8af0242a90d9..e58dc310afd7 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 88817a3376ef..5b64138f549d 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2420,24 +2420,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 2ed3594f384e..072cac5ab5a4 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -1135,10 +1135,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) struct arm_smmu_device *smmu; int ret; - if (!fwspec || fwspec->ops != &arm_smmu_ops) { - dev_err(dev, "cannot attach to SMMU, is it on the same bus?\n"); - return -ENXIO; - } + if (!fwspec || fwspec->ops != &arm_smmu_ops) + return -EMEDIUMTYPE; /* * FIXME: The arch/arm DMA API code tries to attach devices to its own diff --git a/drivers/iommu/arm/arm-smmu/qcom_iommu.c b/drivers/iommu/arm/arm-smmu/qcom_iommu.c index 4c077c38fbd6..8372f985c14a 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 44016594831d..db5fb799e350 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -4323,19 +4323,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 847ad47a2dfd..5b0afe39275e 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1972,6 +1972,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; @@ -2098,6 +2112,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 8fdb84b3642b..82d63394b166 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/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c index e1cb51b9866c..5386d889429d 100644 --- a/drivers/iommu/mtk_iommu_v1.c +++ b/drivers/iommu/mtk_iommu_v1.c @@ -304,7 +304,7 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device /* Only allow the domain created internally. */ mtk_mapping = data->mapping; if (mtk_mapping->domain != domain) - return 0; + return -EMEDIUMTYPE; if (!data->m4u_dom) { data->m4u_dom = dom; 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 bd409bab6286..f6ae230ca1cd 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 a6700a40a6f8..011c33e6ae31 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 25be4b822aa0..a41a62dccb4d 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);