Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> From: Robin Murphy > 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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
On Fri, 2022-06-24 at 06:16 +, 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...@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.mur...@arm.com/
sorry, I don't know its current status.
Thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
> 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...@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?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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...@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.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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...@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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/5] iommu: Return -EMEDIUMTYPE for incompatible domain and device/group
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
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
