diff mbox series

[v6,07/25] iommu/mtk_iommu_v1: Implement an IDENTITY domain

Message ID 7-v6-e8114faedade+425-iommu_all_defdom_jgg@nvidia.com
State Superseded
Headers show
Series iommu: Make default_domain's mandatory | expand

Commit Message

Jason Gunthorpe Aug. 3, 2023, 12:07 a.m. UTC 
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(-)

Comments

Baolu Lu Aug. 14, 2023, 3:06 a.m. UTC | #1 
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
Jason Gunthorpe Aug. 14, 2023, 2:34 p.m. UTC | #2 
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
Baolu Lu Aug. 15, 2023, 12:34 a.m. UTC | #3 
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 mbox series

Patch

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,