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. 😊 ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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. ___ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
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?
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
