| Message ID | 7-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 90057dc095dca023ad3e8cd5a129f392b543fb9e |
| Headers | show |
| Series | iommu: Make default_domain's mandatory | expand |
On 2023/8/3 8:07, Jason Gunthorpe wrote: > What mtk does during mtk_iommu_v1_set_platform_dma() is actually putting > the iommu into identity mode. Make this available as a proper IDENTITY > domain. > > The mtk_iommu_v1_def_domain_type() from > commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type") explains > this was needed to allow probe_finalize() to be called, but now the > IDENTITY domain will do the same job so change the returned > def_domain_type. > > mkt_v1 is the only driver that returns IOMMU_DOMAIN_UNMANAGED from > def_domain_type(). This allows the next patch to enforce an IDENTITY > domain policy for this driver. This code seems to be not working properly. * @def_domain_type: device default domain type, return value: * - IOMMU_DOMAIN_IDENTITY: must use an identity domain * - IOMMU_DOMAIN_DMA: must use a dma domain * - 0: use the default setting The core code is not ready to accept any other return value. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/mtk_iommu_v1.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > index 8a0a5e5d049f4a..cc3e7d53d33ad9 100644 > --- a/drivers/iommu/mtk_iommu_v1.c > +++ b/drivers/iommu/mtk_iommu_v1.c > @@ -319,11 +319,27 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device > return 0; > } > > -static void mtk_iommu_v1_set_platform_dma(struct device *dev) > +static int mtk_iommu_v1_identity_attach(struct iommu_domain *identity_domain, > + struct device *dev) > { > struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev); > > mtk_iommu_v1_config(data, dev, false); > + return 0; > +} > + > +static struct iommu_domain_ops mtk_iommu_v1_identity_ops = { > + .attach_dev = mtk_iommu_v1_identity_attach, > +}; > + > +static struct iommu_domain mtk_iommu_v1_identity_domain = { > + .type = IOMMU_DOMAIN_IDENTITY, > + .ops = &mtk_iommu_v1_identity_ops, > +}; > + > +static void mtk_iommu_v1_set_platform_dma(struct device *dev) > +{ > + mtk_iommu_v1_identity_attach(&mtk_iommu_v1_identity_domain, dev); This callback seems to be a dead code now. > } > > static int mtk_iommu_v1_map(struct iommu_domain *domain, unsigned long iova, > @@ -443,7 +459,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg > > static int mtk_iommu_v1_def_domain_type(struct device *dev) > { > - return IOMMU_DOMAIN_UNMANAGED; > + return IOMMU_DOMAIN_IDENTITY; def_domain_type can't be used for this purpose. But this seems to be a temporary code, as it will be removed in patch 09/25. > } > > static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) > @@ -578,6 +594,7 @@ static int mtk_iommu_v1_hw_init(const struct mtk_iommu_v1_data *data) > } > > static const struct iommu_ops mtk_iommu_v1_ops = { > + .identity_domain = &mtk_iommu_v1_identity_domain, > .domain_alloc = mtk_iommu_v1_domain_alloc, > .probe_device = mtk_iommu_v1_probe_device, > .probe_finalize = mtk_iommu_v1_probe_finalize, Best regards, baolu
On Mon, Aug 14, 2023 at 11:06:12AM +0800, Baolu Lu wrote: > On 2023/8/3 8:07, Jason Gunthorpe wrote: > > What mtk does during mtk_iommu_v1_set_platform_dma() is actually putting > > the iommu into identity mode. Make this available as a proper IDENTITY > > domain. > > > > The mtk_iommu_v1_def_domain_type() from > > commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type") explains > > this was needed to allow probe_finalize() to be called, but now the > > IDENTITY domain will do the same job so change the returned > > def_domain_type. > > > > mkt_v1 is the only driver that returns IOMMU_DOMAIN_UNMANAGED from > > def_domain_type(). This allows the next patch to enforce an IDENTITY > > domain policy for this driver. > > This code seems to be not working properly. > > * @def_domain_type: device default domain type, return value: > * - IOMMU_DOMAIN_IDENTITY: must use an identity domain > * - IOMMU_DOMAIN_DMA: must use a dma domain > * - 0: use the default setting > > The core code is not ready to accept any other return value. Right, it is not following the spec. The design does do what it is supposed to though.. > > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > > index 8a0a5e5d049f4a..cc3e7d53d33ad9 100644 > > --- a/drivers/iommu/mtk_iommu_v1.c > > +++ b/drivers/iommu/mtk_iommu_v1.c > > @@ -319,11 +319,27 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device > > return 0; > > } > > -static void mtk_iommu_v1_set_platform_dma(struct device *dev) > > +static int mtk_iommu_v1_identity_attach(struct iommu_domain *identity_domain, > > + struct device *dev) > > { > > struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev); > > mtk_iommu_v1_config(data, dev, false); > > + return 0; > > +} > > + > > +static struct iommu_domain_ops mtk_iommu_v1_identity_ops = { > > + .attach_dev = mtk_iommu_v1_identity_attach, > > +}; > > + > > +static struct iommu_domain mtk_iommu_v1_identity_domain = { > > + .type = IOMMU_DOMAIN_IDENTITY, > > + .ops = &mtk_iommu_v1_identity_ops, > > +}; > > + > > +static void mtk_iommu_v1_set_platform_dma(struct device *dev) > > +{ > > + mtk_iommu_v1_identity_attach(&mtk_iommu_v1_identity_domain, dev); > > This callback seems to be a dead code now. Not yet in the series, it is still used. All this patch does is introduce the identity domain. > > @@ -443,7 +459,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg > > static int mtk_iommu_v1_def_domain_type(struct device *dev) > > { > > - return IOMMU_DOMAIN_UNMANAGED; > > + return IOMMU_DOMAIN_IDENTITY; > > def_domain_type can't be used for this purpose. But this seems to be a > temporary code, as it will be removed in patch 09/25. It looked OK when I checked it, mkt_v1 is really confusing what it tries to do, but it should call probe_finalize and basically do the same hacky thing as what UNMANAGED was trying to accomplish. Did you see something else? Jason
On 2023/8/14 22:34, Jason Gunthorpe wrote: >>> @@ -443,7 +459,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg >>> static int mtk_iommu_v1_def_domain_type(struct device *dev) >>> { >>> - return IOMMU_DOMAIN_UNMANAGED; >>> + return IOMMU_DOMAIN_IDENTITY; >> def_domain_type can't be used for this purpose. But this seems to be a >> temporary code, as it will be removed in patch 09/25. > It looked OK when I checked it, mkt_v1 is really confusing what it > tries to do, but it should call probe_finalize and basically do the > same hacky thing as what UNMANAGED was trying to accomplish. > > Did you see something else? No. Best regards, baolu
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c index 8a0a5e5d049f4a..cc3e7d53d33ad9 100644 --- a/drivers/iommu/mtk_iommu_v1.c +++ b/drivers/iommu/mtk_iommu_v1.c @@ -319,11 +319,27 @@ static int mtk_iommu_v1_attach_device(struct iommu_domain *domain, struct device return 0; } -static void mtk_iommu_v1_set_platform_dma(struct device *dev) +static int mtk_iommu_v1_identity_attach(struct iommu_domain *identity_domain, + struct device *dev) { struct mtk_iommu_v1_data *data = dev_iommu_priv_get(dev); mtk_iommu_v1_config(data, dev, false); + return 0; +} + +static struct iommu_domain_ops mtk_iommu_v1_identity_ops = { + .attach_dev = mtk_iommu_v1_identity_attach, +}; + +static struct iommu_domain mtk_iommu_v1_identity_domain = { + .type = IOMMU_DOMAIN_IDENTITY, + .ops = &mtk_iommu_v1_identity_ops, +}; + +static void mtk_iommu_v1_set_platform_dma(struct device *dev) +{ + mtk_iommu_v1_identity_attach(&mtk_iommu_v1_identity_domain, dev); } static int mtk_iommu_v1_map(struct iommu_domain *domain, unsigned long iova, @@ -443,7 +459,7 @@ static int mtk_iommu_v1_create_mapping(struct device *dev, struct of_phandle_arg static int mtk_iommu_v1_def_domain_type(struct device *dev) { - return IOMMU_DOMAIN_UNMANAGED; + return IOMMU_DOMAIN_IDENTITY; } static struct iommu_device *mtk_iommu_v1_probe_device(struct device *dev) @@ -578,6 +594,7 @@ static int mtk_iommu_v1_hw_init(const struct mtk_iommu_v1_data *data) } static const struct iommu_ops mtk_iommu_v1_ops = { + .identity_domain = &mtk_iommu_v1_identity_domain, .domain_alloc = mtk_iommu_v1_domain_alloc, .probe_device = mtk_iommu_v1_probe_device, .probe_finalize = mtk_iommu_v1_probe_finalize,
What mtk does during mtk_iommu_v1_set_platform_dma() is actually putting the iommu into identity mode. Make this available as a proper IDENTITY domain. The mtk_iommu_v1_def_domain_type() from commit 8bbe13f52cb7 ("iommu/mediatek-v1: Add def_domain_type") explains this was needed to allow probe_finalize() to be called, but now the IDENTITY domain will do the same job so change the returned def_domain_type. mkt_v1 is the only driver that returns IOMMU_DOMAIN_UNMANAGED from def_domain_type(). This allows the next patch to enforce an IDENTITY domain policy for this driver. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/iommu/mtk_iommu_v1.c | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-)