[PATCH v3 0/3] perf/smmuv3: Don't reserve the PMCG register spaces

[PATCH v3 0/3] perf/smmuv3: Don't reserve the PMCG register spaces

[Zhen Lei]

v2 --> v3:
Patch 3 is updated because https://lkml.org/lkml/2021/1/22/532 has been queued in advance.

v1 --> v2:
According to Robin Murphy's suggestion: https://lkml.org/lkml/2021/1/20/470
Don't reserve the PMCG register spaces, and reserve the entire SMMU register space.

v1:
Since the PMCG may implement its resigters space(4KB Page0 and 4KB Page1)
within the SMMUv3 64KB Page0. In this case, when the SMMUv3 driver reserves the
64KB Page0 resource in advance, the PMCG driver try to reserve its Page0 and
Page1 resources, a resource conflict occurs.

commit 52f3fab0067d6fa ("iommu/arm-smmu-v3: Don't reserve implementation
defined register space") reduce the resource reservation range of the SMMUv3
driver, it only reserves the first 0xe00 bytes in the 64KB Page0, to avoid
the above-mentioned resource conflicts.

But the SMMUv3.3 add support for ECMDQ, its registers space is also implemented
in the SMMUv3 64KB Page0. This means we need to build two separate mappings.
New features may be added in the future, and more independent mappings may be
required. The simple problem is complicated because the user expects to map the
entire SMMUv3 64KB Page0.

Therefore, the proper solution is: If the PMCG register resources are located in
the 64KB Page0 of the SMMU, the PMCG driver does not reserve the conflict resources
when the SMMUv3 driver has reserved the conflict resources before. Instead, the PMCG
driver only performs devm_ioremap() to ensure that it can work properly.

Zhen Lei (3):
  perf/smmuv3: Don't reserve the PMCG register spaces
  perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU
  iommu/arm-smmu-v3: Reserving the entire SMMU register space

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 ++++--------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 --
 drivers/perf/arm_smmuv3_pmu.c               | 28 ++++++++++++++++++++++++++--
 3 files changed, 30 insertions(+), 24 deletions(-)

-- 
1.8.3



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/3] perf/smmuv3: Don't reserve the PMCG register spaces

[Zhen Lei]

According to the SMMUv3 specification:
Each PMCG counter group is represented by one 4KB page (Page 0) with one
optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
DEFINED base addresses.

This means that the PMCG register spaces may be within the 64KB pages of
the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
their own resources, a resource conflict occurs.

To avoid this conflict, don't reserve the PMCG regions.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 74474bb322c3f26..e5e505a0804fe53 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -761,6 +761,29 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
 	dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
 }
 
+static void __iomem *
+smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
+				  unsigned int index,
+				  struct resource **res)
+{
+	void __iomem *base;
+	struct resource *r;
+
+	r = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	if (!r) {
+		dev_err(&pdev->dev, "invalid resource\n");
+		return ERR_PTR(-EINVAL);
+	}
+	if (res)
+		*res = r;
+
+	base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
+	if (!base)
+		return ERR_PTR(-ENOMEM);
+
+	return base;
+}
+
 static int smmu_pmu_probe(struct platform_device *pdev)
 {
 	struct smmu_pmu *smmu_pmu;
@@ -793,7 +816,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
 	};
 
-	smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res_0);
+	smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
 	if (IS_ERR(smmu_pmu->reg_base))
 		return PTR_ERR(smmu_pmu->reg_base);
 
@@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
 
 	/* Determine if page 1 is present */
 	if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
-		smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
+		smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL);
 		if (IS_ERR(smmu_pmu->reloc_base))
 			return PTR_ERR(smmu_pmu->reloc_base);
 	} else {
-- 
1.8.3



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/3] perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU

[Zhen Lei]

The MODULE_SOFTDEP() gives user space a hint of the loading sequence. And
when command "modprobe arm_smmuv3_pmu" is executed, the arm_smmu_v3.ko is
automatically loaded in advance.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/perf/arm_smmuv3_pmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index e5e505a0804fe53..9a305ac51208cd2 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -950,6 +950,7 @@ static void __exit arm_smmu_pmu_exit(void)
 module_exit(arm_smmu_pmu_exit);
 
 MODULE_DESCRIPTION("PMU driver for ARM SMMUv3 Performance Monitors Extension");
+MODULE_SOFTDEP("pre: arm_smmu_v3");
 MODULE_AUTHOR("Neil Leeder <nleeder@codeaurora.org>");
 MODULE_AUTHOR("Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>");
 MODULE_LICENSE("GPL v2");
-- 
1.8.3



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/3] iommu/arm-smmu-v3: Reserving the entire SMMU register space

[Zhen Lei]

commit 52f3fab0067d ("iommu/arm-smmu-v3: Don't reserve implementation
defined register space") only reserves the basic SMMU register space. So
the ECMDQ register space is not covered, it should be mapped again. Due
to the size of this ECMDQ resource is not fixed, depending on
SMMU_IDR6.CMDQ_CONTROL_PAGE_LOG2NUMQ. Processing its resource reservation
to avoid resource conflict with PMCG is a bit more complicated.

Therefore, the resources of the PMCG are not reserved, and the entire SMMU
resources are reserved.

Suggested-by: Robin Murphy <robin.murphy@arm.com>
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 ++++--------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 --
 2 files changed, 4 insertions(+), 22 deletions(-)

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 f04c55a7503c790..fde24403b06a9e3 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -3476,14 +3476,6 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
 	return err;
 }
 
-static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
-				      resource_size_t size)
-{
-	struct resource res = DEFINE_RES_MEM(start, size);
-
-	return devm_ioremap_resource(dev, &res);
-}
-
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -3519,22 +3511,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 	}
 	ioaddr = res->start;
 
-	/*
-	 * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
-	 * the PMCG registers which are reserved by the PMU driver.
-	 */
-	smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
+	smmu->base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(smmu->base))
 		return PTR_ERR(smmu->base);
 
-	if (arm_smmu_resource_size(smmu) > SZ_64K) {
-		smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
-					       ARM_SMMU_REG_SZ);
-		if (IS_ERR(smmu->page1))
-			return PTR_ERR(smmu->page1);
-	} else {
+	if (arm_smmu_resource_size(smmu) > SZ_64K)
+		smmu->page1 = smmu->base + SZ_64K;
+	else
 		smmu->page1 = smmu->base;
-	}
 
 	/* Interrupt lines */
 
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index da525f46dab4fc1..63f2b476987d6ae 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -152,8 +152,6 @@
 #define ARM_SMMU_PRIQ_IRQ_CFG1		0xd8
 #define ARM_SMMU_PRIQ_IRQ_CFG2		0xdc
 
-#define ARM_SMMU_REG_SZ			0xe00
-
 /* Common MSI config fields */
 #define MSI_CFG0_ADDR_MASK		GENMASK_ULL(51, 2)
 #define MSI_CFG2_SH			GENMASK(5, 4)
-- 
1.8.3



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/3] perf/smmuv3: Don't reserve the PMCG register spaces

[Will Deacon]

On Wed, Jan 27, 2021 at 07:32:55PM +0800, Zhen Lei wrote:
> v2 --> v3:
> Patch 3 is updated because https://lkml.org/lkml/2021/1/22/532 has been queued in advance.
> 
> v1 --> v2:
> According to Robin Murphy's suggestion: https://lkml.org/lkml/2021/1/20/470
> Don't reserve the PMCG register spaces, and reserve the entire SMMU register space.
> 
> v1:
> Since the PMCG may implement its resigters space(4KB Page0 and 4KB Page1)
> within the SMMUv3 64KB Page0. In this case, when the SMMUv3 driver reserves the
> 64KB Page0 resource in advance, the PMCG driver try to reserve its Page0 and
> Page1 resources, a resource conflict occurs.
> 
> commit 52f3fab0067d6fa ("iommu/arm-smmu-v3: Don't reserve implementation
> defined register space") reduce the resource reservation range of the SMMUv3
> driver, it only reserves the first 0xe00 bytes in the 64KB Page0, to avoid
> the above-mentioned resource conflicts.
> 
> But the SMMUv3.3 add support for ECMDQ, its registers space is also implemented
> in the SMMUv3 64KB Page0. This means we need to build two separate mappings.
> New features may be added in the future, and more independent mappings may be
> required. The simple problem is complicated because the user expects to map the
> entire SMMUv3 64KB Page0.
> 
> Therefore, the proper solution is: If the PMCG register resources are located in
> the 64KB Page0 of the SMMU, the PMCG driver does not reserve the conflict resources
> when the SMMUv3 driver has reserved the conflict resources before. Instead, the PMCG
> driver only performs devm_ioremap() to ensure that it can work properly.
> 
> Zhen Lei (3):
>   perf/smmuv3: Don't reserve the PMCG register spaces
>   perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU
>   iommu/arm-smmu-v3: Reserving the entire SMMU register space

I'll need Robin's ack on these.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 0/3] perf/smmuv3: Don't reserve the PMCG register spaces

[Leizhen (ThunderTown)]

On 2021/1/29 4:31, Will Deacon wrote:
> On Wed, Jan 27, 2021 at 07:32:55PM +0800, Zhen Lei wrote:
>> v2 --> v3:
>> Patch 3 is updated because https://lkml.org/lkml/2021/1/22/532 has been queued in advance.
>>
>> v1 --> v2:
>> According to Robin Murphy's suggestion: https://lkml.org/lkml/2021/1/20/470
>> Don't reserve the PMCG register spaces, and reserve the entire SMMU register space.
>>
>> v1:
>> Since the PMCG may implement its resigters space(4KB Page0 and 4KB Page1)
>> within the SMMUv3 64KB Page0. In this case, when the SMMUv3 driver reserves the
>> 64KB Page0 resource in advance, the PMCG driver try to reserve its Page0 and
>> Page1 resources, a resource conflict occurs.
>>
>> commit 52f3fab0067d6fa ("iommu/arm-smmu-v3: Don't reserve implementation
>> defined register space") reduce the resource reservation range of the SMMUv3
>> driver, it only reserves the first 0xe00 bytes in the 64KB Page0, to avoid
>> the above-mentioned resource conflicts.
>>
>> But the SMMUv3.3 add support for ECMDQ, its registers space is also implemented
>> in the SMMUv3 64KB Page0. This means we need to build two separate mappings.
>> New features may be added in the future, and more independent mappings may be
>> required. The simple problem is complicated because the user expects to map the
>> entire SMMUv3 64KB Page0.
>>
>> Therefore, the proper solution is: If the PMCG register resources are located in
>> the 64KB Page0 of the SMMU, the PMCG driver does not reserve the conflict resources
>> when the SMMUv3 driver has reserved the conflict resources before. Instead, the PMCG
>> driver only performs devm_ioremap() to ensure that it can work properly.
>>
>> Zhen Lei (3):
>>   perf/smmuv3: Don't reserve the PMCG register spaces
>>   perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU
>>   iommu/arm-smmu-v3: Reserving the entire SMMU register space
> 
> I'll need Robin's ack on these.

Hi, Robin:
  What's your opinion?
  In fact, I have written the patches that SMMU and PMCG whoever come first
will reserve the resources, and whoever comes next does not reserve. However,
the processing logic is relatively complex.

> 
> Will
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] perf/smmuv3: Don't reserve the PMCG register spaces

[Robin Murphy]

On 2021-01-27 11:32, Zhen Lei wrote:
> According to the SMMUv3 specification:
> Each PMCG counter group is represented by one 4KB page (Page 0) with one
> optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
> DEFINED base addresses.
> 
> This means that the PMCG register spaces may be within the 64KB pages of
> the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
> their own resources, a resource conflict occurs.
> 
> To avoid this conflict, don't reserve the PMCG regions.

I'm still not a fan of this get_and_ioremap notion in general, 
especially when the "helper" function ends up over twice the size of all 
the code it replaces[1], but for the actual functional change here,

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/perf/arm_smmuv3_pmu.c | 27 +++++++++++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index 74474bb322c3f26..e5e505a0804fe53 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -761,6 +761,29 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
>   	dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
>   }
>   
> +static void __iomem *
> +smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
> +				  unsigned int index,
> +				  struct resource **res)
> +{
> +	void __iomem *base;
> +	struct resource *r;
> +
> +	r = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +	if (!r) {
> +		dev_err(&pdev->dev, "invalid resource\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +	if (res)
> +		*res = r;
> +
> +	base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> +	if (!base)
> +		return ERR_PTR(-ENOMEM);
> +
> +	return base;
> +}
> +
>   static int smmu_pmu_probe(struct platform_device *pdev)
>   {
>   	struct smmu_pmu *smmu_pmu;
> @@ -793,7 +816,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>   		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
>   	};
>   
> -	smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res_0);
> +	smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
>   	if (IS_ERR(smmu_pmu->reg_base))
>   		return PTR_ERR(smmu_pmu->reg_base);
>   
> @@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>   
>   	/* Determine if page 1 is present */
>   	if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
> -		smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
> +		smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL);
>   		if (IS_ERR(smmu_pmu->reloc_base))
>   			return PTR_ERR(smmu_pmu->reloc_base);
>   	} else {
> 

[1]
----->8-----
  drivers/perf/arm_smmuv3_pmu.c | 35 +++++++++--------------------------
  1 file changed, 9 insertions(+), 26 deletions(-)

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index e5e505a0804f..c9adbc7b55a1 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -761,33 +761,10 @@ static void smmu_pmu_get_acpi_options(struct 
smmu_pmu *smmu_pmu)
  	dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
  }

-static void __iomem *
-smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
-				  unsigned int index,
-				  struct resource **res)
-{
-	void __iomem *base;
-	struct resource *r;
-
-	r = platform_get_resource(pdev, IORESOURCE_MEM, index);
-	if (!r) {
-		dev_err(&pdev->dev, "invalid resource\n");
-		return ERR_PTR(-EINVAL);
-	}
-	if (res)
-		*res = r;
-
-	base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
-	if (!base)
-		return ERR_PTR(-ENOMEM);
-
-	return base;
-}
-
  static int smmu_pmu_probe(struct platform_device *pdev)
  {
  	struct smmu_pmu *smmu_pmu;
-	struct resource *res_0;
+	struct resource *res_0, *res_1;
  	u32 cfgr, reg_size;
  	u64 ceid_64[2];
  	int irq, err;
@@ -816,7 +793,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)
  		.capabilities	= PERF_PMU_CAP_NO_EXCLUDE,
  	};

-	smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
+	res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res_0)
+		return ERR_PTR(-EINVAL);
+	smmu_pmu->reg_base = devm_ioremap(dev, res_0->start, 
resource_size(res_0));
  	if (IS_ERR(smmu_pmu->reg_base))
  		return PTR_ERR(smmu_pmu->reg_base);

@@ -824,7 +804,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)

  	/* Determine if page 1 is present */
  	if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
-		smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL);
+		res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		if (!res_1)
+			return -EINVAL;
+		smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start, 
resource_size(res_1));
  		if (IS_ERR(smmu_pmu->reloc_base))
  			return PTR_ERR(smmu_pmu->reloc_base);
  	} else {

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU

[Robin Murphy]

On 2021-01-27 11:32, Zhen Lei wrote:
> The MODULE_SOFTDEP() gives user space a hint of the loading sequence. And
> when command "modprobe arm_smmuv3_pmu" is executed, the arm_smmu_v3.ko is
> automatically loaded in advance.

Why do we need this? If probe order doesn't matter when both drivers are 
built-in, why should module load order?

TBH I'm not sure why we even have a Kconfig dependency on ARM_SMMU_V3, 
given that the drivers operate completely independently :/

Robin.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/perf/arm_smmuv3_pmu.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index e5e505a0804fe53..9a305ac51208cd2 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -950,6 +950,7 @@ static void __exit arm_smmu_pmu_exit(void)
>   module_exit(arm_smmu_pmu_exit);
>   
>   MODULE_DESCRIPTION("PMU driver for ARM SMMUv3 Performance Monitors Extension");
> +MODULE_SOFTDEP("pre: arm_smmu_v3");
>   MODULE_AUTHOR("Neil Leeder <nleeder@codeaurora.org>");
>   MODULE_AUTHOR("Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>");
>   MODULE_LICENSE("GPL v2");
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/3] iommu/arm-smmu-v3: Reserving the entire SMMU register space

[Robin Murphy]

On 2021-01-27 11:32, Zhen Lei wrote:
> commit 52f3fab0067d ("iommu/arm-smmu-v3: Don't reserve implementation
> defined register space") only reserves the basic SMMU register space. So
> the ECMDQ register space is not covered, it should be mapped again. Due
> to the size of this ECMDQ resource is not fixed, depending on
> SMMU_IDR6.CMDQ_CONTROL_PAGE_LOG2NUMQ. Processing its resource reservation
> to avoid resource conflict with PMCG is a bit more complicated.
> 
> Therefore, the resources of the PMCG are not reserved, and the entire SMMU
> resources are reserved.

This is the opposite of what I suggested. My point was that it will make 
the most sense to map the ECMDQ pages as a separate request anyway, 
therefore there is no reason to stop mapping page 0 and page 1 
separately either.

If we need to expand the page 0 mapping to cover more of page 0 to reach 
the SMMU_CMDQ_CONTROL_PAGE_* registers, we can do that when we actually 
add ECMDQ support.

Robin.

> Suggested-by: Robin Murphy <robin.murphy@arm.com>
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 ++++--------------------
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 --
>   2 files changed, 4 insertions(+), 22 deletions(-)
> 
> 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 f04c55a7503c790..fde24403b06a9e3 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -3476,14 +3476,6 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
>   	return err;
>   }
>   
> -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
> -				      resource_size_t size)
> -{
> -	struct resource res = DEFINE_RES_MEM(start, size);
> -
> -	return devm_ioremap_resource(dev, &res);
> -}
> -
>   static int arm_smmu_device_probe(struct platform_device *pdev)
>   {
>   	int irq, ret;
> @@ -3519,22 +3511,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   	}
>   	ioaddr = res->start;
>   
> -	/*
> -	 * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
> -	 * the PMCG registers which are reserved by the PMU driver.
> -	 */
> -	smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
> +	smmu->base = devm_ioremap_resource(dev, res);
>   	if (IS_ERR(smmu->base))
>   		return PTR_ERR(smmu->base);
>   
> -	if (arm_smmu_resource_size(smmu) > SZ_64K) {
> -		smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
> -					       ARM_SMMU_REG_SZ);
> -		if (IS_ERR(smmu->page1))
> -			return PTR_ERR(smmu->page1);
> -	} else {
> +	if (arm_smmu_resource_size(smmu) > SZ_64K)
> +		smmu->page1 = smmu->base + SZ_64K;
> +	else
>   		smmu->page1 = smmu->base;
> -	}
>   
>   	/* Interrupt lines */
>   
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index da525f46dab4fc1..63f2b476987d6ae 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -152,8 +152,6 @@
>   #define ARM_SMMU_PRIQ_IRQ_CFG1		0xd8
>   #define ARM_SMMU_PRIQ_IRQ_CFG2		0xdc
>   
> -#define ARM_SMMU_REG_SZ			0xe00
> -
>   /* Common MSI config fields */
>   #define MSI_CFG0_ADDR_MASK		GENMASK_ULL(51, 2)
>   #define MSI_CFG2_SH			GENMASK(5, 4)
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU

[John Garry]

On 29/01/2021 15:12, Robin Murphy wrote:
> On 2021-01-27 11:32, Zhen Lei wrote:
>> The MODULE_SOFTDEP() gives user space a hint of the loading sequence. And
>> when command "modprobe arm_smmuv3_pmu" is executed, the arm_smmu_v3.ko is
>> automatically loaded in advance.
> 
> Why do we need this? If probe order doesn't matter when both drivers are 
> built-in, why should module load order?
> 
> TBH I'm not sure why we even have a Kconfig dependency on ARM_SMMU_V3, 
> given that the drivers operate completely independently :/

Can that Kconfig dependency just be removed? I think that it was added 
under the idea that there is no point in having the SMMUv3 PMU driver 
without the SMMUv3 driver.

However even on that basis it seems broken, as we cannot have 
ARM_SMMU_V3=m + ARM_SMMU_V3_PMU=y.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU

[Robin Murphy]

On 2021-01-29 15:34, John Garry wrote:
> On 29/01/2021 15:12, Robin Murphy wrote:
>> On 2021-01-27 11:32, Zhen Lei wrote:
>>> The MODULE_SOFTDEP() gives user space a hint of the loading sequence. 
>>> And
>>> when command "modprobe arm_smmuv3_pmu" is executed, the 
>>> arm_smmu_v3.ko is
>>> automatically loaded in advance.
>>
>> Why do we need this? If probe order doesn't matter when both drivers 
>> are built-in, why should module load order?
>>
>> TBH I'm not sure why we even have a Kconfig dependency on ARM_SMMU_V3, 
>> given that the drivers operate completely independently :/
> 
> Can that Kconfig dependency just be removed? I think that it was added 
> under the idea that there is no point in having the SMMUv3 PMU driver 
> without the SMMUv3 driver.

A PMCG *might* be usable for simply counting transactions to measure 
device activity regardless of its associated SMMU being enabled. Either 
way, it's not really Kconfig's job to decide what makes sense (beyond 
the top-level "can this driver *ever* be used on this platform" 
visibility choices). Imagine if we gave every PCI/USB/etc. device driver 
an explicit dependency on at least one host controller driver being 
enabled...

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 2/3] perf/smmuv3: Add a MODULE_SOFTDEP() to indicate dependency on SMMU

[Leizhen (ThunderTown)]

On 2021/1/30 1:03, Robin Murphy wrote:
> On 2021-01-29 15:34, John Garry wrote:
>> On 29/01/2021 15:12, Robin Murphy wrote:
>>> On 2021-01-27 11:32, Zhen Lei wrote:
>>>> The MODULE_SOFTDEP() gives user space a hint of the loading sequence. And
>>>> when command "modprobe arm_smmuv3_pmu" is executed, the arm_smmu_v3.ko is
>>>> automatically loaded in advance.
>>>
>>> Why do we need this? If probe order doesn't matter when both drivers are built-in, why should module load order?
>>>
>>> TBH I'm not sure why we even have a Kconfig dependency on ARM_SMMU_V3, given that the drivers operate completely independently :/
>>
>> Can that Kconfig dependency just be removed? I think that it was added under the idea that there is no point in having the SMMUv3 PMU driver without the SMMUv3 driver.
> 
> A PMCG *might* be usable for simply counting transactions to measure device activity regardless of its associated SMMU being enabled.

If that's the case, the SOFTDEP really shouldn't be added. I wasn't trying to make sure they were loaded in order, just to make sure that the SMMU was not forgotten to load.

> Either way, it's not really Kconfig's job to decide what makes sense (beyond the top-level "can this driver *ever* be used on this platform" visibility choices). Imagine if we gave every PCI/USB/etc. device driver an explicit ?dependency on at least one host controller driver being enabled...
> 
> Robin.
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/3] iommu/arm-smmu-v3: Reserving the entire SMMU register space

[Leizhen (ThunderTown)]

On 2021/1/29 23:27, Robin Murphy wrote:
> On 2021-01-27 11:32, Zhen Lei wrote:
>> commit 52f3fab0067d ("iommu/arm-smmu-v3: Don't reserve implementation
>> defined register space") only reserves the basic SMMU register space. So
>> the ECMDQ register space is not covered, it should be mapped again. Due
>> to the size of this ECMDQ resource is not fixed, depending on
>> SMMU_IDR6.CMDQ_CONTROL_PAGE_LOG2NUMQ. Processing its resource reservation
>> to avoid resource conflict with PMCG is a bit more complicated.
>>
>> Therefore, the resources of the PMCG are not reserved, and the entire SMMU
>> resources are reserved.
> 
> This is the opposite of what I suggested. My point was that it will make the most sense to map the ECMDQ pages as a separate request anyway, therefore there is no reason to stop mapping page 0 and page 1 separately either.

I don't understand why the ECMDQ mapping must be performed separately. If the conflict with PMCG resources is eliminated. ECMDQ cannot be a separate driver like PMCG.

> 
> If we need to expand the page 0 mapping to cover more of page 0 to reach the SMMU_CMDQ_CONTROL_PAGE_* registers, we can do that when we actually add ECMDQ support.
> 
> Robin.
> 
>> Suggested-by: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 ++++--------------------
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 --
>>   2 files changed, 4 insertions(+), 22 deletions(-)
>>
>> 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 f04c55a7503c790..fde24403b06a9e3 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -3476,14 +3476,6 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
>>       return err;
>>   }
>>   -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>> -                      resource_size_t size)
>> -{
>> -    struct resource res = DEFINE_RES_MEM(start, size);
>> -
>> -    return devm_ioremap_resource(dev, &res);
>> -}
>> -
>>   static int arm_smmu_device_probe(struct platform_device *pdev)
>>   {
>>       int irq, ret;
>> @@ -3519,22 +3511,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>       }
>>       ioaddr = res->start;
>>   -    /*
>> -     * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
>> -     * the PMCG registers which are reserved by the PMU driver.
>> -     */
>> -    smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
>> +    smmu->base = devm_ioremap_resource(dev, res);
>>       if (IS_ERR(smmu->base))
>>           return PTR_ERR(smmu->base);
>>   -    if (arm_smmu_resource_size(smmu) > SZ_64K) {
>> -        smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
>> -                           ARM_SMMU_REG_SZ);
>> -        if (IS_ERR(smmu->page1))
>> -            return PTR_ERR(smmu->page1);
>> -    } else {
>> +    if (arm_smmu_resource_size(smmu) > SZ_64K)
>> +        smmu->page1 = smmu->base + SZ_64K;
>> +    else
>>           smmu->page1 = smmu->base;
>> -    }
>>         /* Interrupt lines */
>>   diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> index da525f46dab4fc1..63f2b476987d6ae 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>> @@ -152,8 +152,6 @@
>>   #define ARM_SMMU_PRIQ_IRQ_CFG1        0xd8
>>   #define ARM_SMMU_PRIQ_IRQ_CFG2        0xdc
>>   -#define ARM_SMMU_REG_SZ            0xe00
>> -
>>   /* Common MSI config fields */
>>   #define MSI_CFG0_ADDR_MASK        GENMASK_ULL(51, 2)
>>   #define MSI_CFG2_SH            GENMASK(5, 4)
>>
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] perf/smmuv3: Don't reserve the PMCG register spaces

[Leizhen (ThunderTown)]

On 2021/1/29 23:06, Robin Murphy wrote:
> On 2021-01-27 11:32, Zhen Lei wrote:
>> According to the SMMUv3 specification:
>> Each PMCG counter group is represented by one 4KB page (Page 0) with one
>> optional additional 4KB page (Page 1), both of which are at IMPLEMENTATION
>> DEFINED base addresses.
>>
>> This means that the PMCG register spaces may be within the 64KB pages of
>> the SMMUv3 register space. When both the SMMU and PMCG drivers reserve
>> their own resources, a resource conflict occurs.
>>
>> To avoid this conflict, don't reserve the PMCG regions.
> 
> I'm still not a fan of this get_and_ioremap notion in general, especially when the "helper" function ends up over twice the size of all the code it replaces[1], but for the actual functional change here,

OK，I'll change it to [1] and add some comments.

> 
> Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> 
>> Suggested-by: Robin Murphy <robin.murphy@arm.com>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/perf/arm_smmuv3_pmu.c | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>> index 74474bb322c3f26..e5e505a0804fe53 100644
>> --- a/drivers/perf/arm_smmuv3_pmu.c
>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>> @@ -761,6 +761,29 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
>>       dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
>>   }
>>   +static void __iomem *
>> +smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
>> +                  unsigned int index,
>> +                  struct resource **res)
>> +{
>> +    void __iomem *base;
>> +    struct resource *r;
>> +
>> +    r = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +    if (!r) {
>> +        dev_err(&pdev->dev, "invalid resource\n");
>> +        return ERR_PTR(-EINVAL);
>> +    }
>> +    if (res)
>> +        *res = r;
>> +
>> +    base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
>> +    if (!base)
>> +        return ERR_PTR(-ENOMEM);
>> +
>> +    return base;
>> +}
>> +
>>   static int smmu_pmu_probe(struct platform_device *pdev)
>>   {
>>       struct smmu_pmu *smmu_pmu;
>> @@ -793,7 +816,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>>           .capabilities    = PERF_PMU_CAP_NO_EXCLUDE,
>>       };
>>   -    smmu_pmu->reg_base = devm_platform_get_and_ioremap_resource(pdev, 0, &res_0);
>> +    smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
>>       if (IS_ERR(smmu_pmu->reg_base))
>>           return PTR_ERR(smmu_pmu->reg_base);
>>   @@ -801,7 +824,7 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>>         /* Determine if page 1 is present */
>>       if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
>> -        smmu_pmu->reloc_base = devm_platform_ioremap_resource(pdev, 1);
>> +        smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL);
>>           if (IS_ERR(smmu_pmu->reloc_base))
>>               return PTR_ERR(smmu_pmu->reloc_base);
>>       } else {
>>
> 
> [1]
> ----->8-----
>  drivers/perf/arm_smmuv3_pmu.c | 35 +++++++++--------------------------
>  1 file changed, 9 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index e5e505a0804f..c9adbc7b55a1 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -761,33 +761,10 @@ static void smmu_pmu_get_acpi_options(struct smmu_pmu *smmu_pmu)
>      dev_notice(smmu_pmu->dev, "option mask 0x%x\n", smmu_pmu->options);
>  }
> 
> -static void __iomem *
> -smmu_pmu_get_and_ioremap_resource(struct platform_device *pdev,
> -                  unsigned int index,
> -                  struct resource **res)
> -{
> -    void __iomem *base;
> -    struct resource *r;
> -
> -    r = platform_get_resource(pdev, IORESOURCE_MEM, index);
> -    if (!r) {
> -        dev_err(&pdev->dev, "invalid resource\n");
> -        return ERR_PTR(-EINVAL);
> -    }
> -    if (res)
> -        *res = r;
> -
> -    base = devm_ioremap(&pdev->dev, r->start, resource_size(r));
> -    if (!base)
> -        return ERR_PTR(-ENOMEM);
> -
> -    return base;
> -}
> -
>  static int smmu_pmu_probe(struct platform_device *pdev)
>  {
>      struct smmu_pmu *smmu_pmu;
> -    struct resource *res_0;
> +    struct resource *res_0, *res_1;
>      u32 cfgr, reg_size;
>      u64 ceid_64[2];
>      int irq, err;
> @@ -816,7 +793,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)
>          .capabilities    = PERF_PMU_CAP_NO_EXCLUDE,
>      };
> 
> -    smmu_pmu->reg_base = smmu_pmu_get_and_ioremap_resource(pdev, 0, &res_0);
> +    res_0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +    if (!res_0)
> +        return ERR_PTR(-EINVAL);
> +    smmu_pmu->reg_base = devm_ioremap(dev, res_0->start, resource_size(res_0));
>      if (IS_ERR(smmu_pmu->reg_base))
>          return PTR_ERR(smmu_pmu->reg_base);
> 
> @@ -824,7 +804,10 @@ static int smmu_pmu_probe(struct platform_device *pdev)
> 
>      /* Determine if page 1 is present */
>      if (cfgr & SMMU_PMCG_CFGR_RELOC_CTRS) {
> -        smmu_pmu->reloc_base = smmu_pmu_get_and_ioremap_resource(pdev, 1, NULL);
> +        res_1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +        if (!res_1)
> +            return -EINVAL;
> +        smmu_pmu->reloc_base = devm_ioremap(dev, res_1->start, resource_size(res_1));
>          if (IS_ERR(smmu_pmu->reloc_base))
>              return PTR_ERR(smmu_pmu->reloc_base);
>      } else {
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/3] iommu/arm-smmu-v3: Reserving the entire SMMU register space

[Robin Murphy]

On 2021-01-30 01:54, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/1/29 23:27, Robin Murphy wrote:
>> On 2021-01-27 11:32, Zhen Lei wrote:
>>> commit 52f3fab0067d ("iommu/arm-smmu-v3: Don't reserve implementation
>>> defined register space") only reserves the basic SMMU register space. So
>>> the ECMDQ register space is not covered, it should be mapped again. Due
>>> to the size of this ECMDQ resource is not fixed, depending on
>>> SMMU_IDR6.CMDQ_CONTROL_PAGE_LOG2NUMQ. Processing its resource reservation
>>> to avoid resource conflict with PMCG is a bit more complicated.
>>>
>>> Therefore, the resources of the PMCG are not reserved, and the entire SMMU
>>> resources are reserved.
>>
>> This is the opposite of what I suggested. My point was that it will make the most sense to map the ECMDQ pages as a separate request anyway, therefore there is no reason to stop mapping page 0 and page 1 separately either.
> 
> I don't understand why the ECMDQ mapping must be performed separately. If the conflict with PMCG resources is eliminated. ECMDQ cannot be a separate driver like PMCG.

I mean in terms of the basic practice of not mapping megabytes worth of 
IMP-DEF crap that this driver doesn't need or even understand. If we 
don't have ECMDQ, we definitely don't need anything beyond page 1, so 
there's no point mapping it all, and indeed it's safest not to anyway. 
Even if we do have ECMDQ, it's still safer not to map all the unknown 
stuff that may be in between, and until we've mapped page 0 we don't 
know whether we have ECMDQ or not.

Therefore the most sensible thing to do either way is to map the basic 
page(s) first, then map the ECMDQ pages specifically if we determine 
that we need to. And either way we don't even need to think about this 
until adding ECMDQ support.

Robin.

>> If we need to expand the page 0 mapping to cover more of page 0 to reach the SMMU_CMDQ_CONTROL_PAGE_* registers, we can do that when we actually add ECMDQ support.
>>
>> Robin.
>>
>>> Suggested-by: Robin Murphy <robin.murphy@arm.com>
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> ---
>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 ++++--------------------
>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 --
>>>    2 files changed, 4 insertions(+), 22 deletions(-)
>>>
>>> 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 f04c55a7503c790..fde24403b06a9e3 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -3476,14 +3476,6 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
>>>        return err;
>>>    }
>>>    -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>>> -                      resource_size_t size)
>>> -{
>>> -    struct resource res = DEFINE_RES_MEM(start, size);
>>> -
>>> -    return devm_ioremap_resource(dev, &res);
>>> -}
>>> -
>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>    {
>>>        int irq, ret;
>>> @@ -3519,22 +3511,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>        }
>>>        ioaddr = res->start;
>>>    -    /*
>>> -     * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
>>> -     * the PMCG registers which are reserved by the PMU driver.
>>> -     */
>>> -    smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
>>> +    smmu->base = devm_ioremap_resource(dev, res);
>>>        if (IS_ERR(smmu->base))
>>>            return PTR_ERR(smmu->base);
>>>    -    if (arm_smmu_resource_size(smmu) > SZ_64K) {
>>> -        smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
>>> -                           ARM_SMMU_REG_SZ);
>>> -        if (IS_ERR(smmu->page1))
>>> -            return PTR_ERR(smmu->page1);
>>> -    } else {
>>> +    if (arm_smmu_resource_size(smmu) > SZ_64K)
>>> +        smmu->page1 = smmu->base + SZ_64K;
>>> +    else
>>>            smmu->page1 = smmu->base;
>>> -    }
>>>          /* Interrupt lines */
>>>    diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>> index da525f46dab4fc1..63f2b476987d6ae 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>> @@ -152,8 +152,6 @@
>>>    #define ARM_SMMU_PRIQ_IRQ_CFG1        0xd8
>>>    #define ARM_SMMU_PRIQ_IRQ_CFG2        0xdc
>>>    -#define ARM_SMMU_REG_SZ            0xe00
>>> -
>>>    /* Common MSI config fields */
>>>    #define MSI_CFG0_ADDR_MASK        GENMASK_ULL(51, 2)
>>>    #define MSI_CFG2_SH            GENMASK(5, 4)
>>>
>>
>> .
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/3] iommu/arm-smmu-v3: Reserving the entire SMMU register space

[Leizhen (ThunderTown)]

On 2021/2/1 19:44, Robin Murphy wrote:
> On 2021-01-30 01:54, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/1/29 23:27, Robin Murphy wrote:
>>> On 2021-01-27 11:32, Zhen Lei wrote:
>>>> commit 52f3fab0067d ("iommu/arm-smmu-v3: Don't reserve implementation
>>>> defined register space") only reserves the basic SMMU register space. So
>>>> the ECMDQ register space is not covered, it should be mapped again. Due
>>>> to the size of this ECMDQ resource is not fixed, depending on
>>>> SMMU_IDR6.CMDQ_CONTROL_PAGE_LOG2NUMQ. Processing its resource reservation
>>>> to avoid resource conflict with PMCG is a bit more complicated.
>>>>
>>>> Therefore, the resources of the PMCG are not reserved, and the entire SMMU
>>>> resources are reserved.
>>>
>>> This is the opposite of what I suggested. My point was that it will make the most sense to map the ECMDQ pages as a separate request anyway, therefore there is no reason to stop mapping page 0 and page 1 separately either.
>>
>> I don't understand why the ECMDQ mapping must be performed separately. If the conflict with PMCG resources is eliminated. ECMDQ cannot be a separate driver like PMCG.
> 
> I mean in terms of the basic practice of not mapping megabytes worth of IMP-DEF crap that this driver doesn't need or even understand. If we don't have ECMDQ, we definitely don't need anything beyond page 1, so there's no point mapping it all, and indeed it's safest not to anyway. Even if we do have ECMDQ, it's still safer not to map all the unknown stuff that may be in between, and until we've mapped page 0 we don't know whether we have ECMDQ or not.
> 
> Therefore the most sensible thing to do either way is to map the basic page(s) first, then map the ECMDQ pages specifically if we determine that we need to. And either way we don't even need to think about this until adding ECMDQ support.

Okay, I got it. We really don't have to write code ahead of time for what might happen in the future.

> 
> Robin.
> 
>>> If we need to expand the page 0 mapping to cover more of page 0 to reach the SMMU_CMDQ_CONTROL_PAGE_* registers, we can do that when we actually add ECMDQ support.
>>>
>>> Robin.
>>>
>>>> Suggested-by: Robin Murphy <robin.murphy@arm.com>
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 24 ++++--------------------
>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  2 --
>>>>    2 files changed, 4 insertions(+), 22 deletions(-)
>>>>
>>>> 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 f04c55a7503c790..fde24403b06a9e3 100644
>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> @@ -3476,14 +3476,6 @@ static int arm_smmu_set_bus_ops(struct iommu_ops *ops)
>>>>        return err;
>>>>    }
>>>>    -static void __iomem *arm_smmu_ioremap(struct device *dev, resource_size_t start,
>>>> -                      resource_size_t size)
>>>> -{
>>>> -    struct resource res = DEFINE_RES_MEM(start, size);
>>>> -
>>>> -    return devm_ioremap_resource(dev, &res);
>>>> -}
>>>> -
>>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>    {
>>>>        int irq, ret;
>>>> @@ -3519,22 +3511,14 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>        }
>>>>        ioaddr = res->start;
>>>>    -    /*
>>>> -     * Don't map the IMPLEMENTATION DEFINED regions, since they may contain
>>>> -     * the PMCG registers which are reserved by the PMU driver.
>>>> -     */
>>>> -    smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_REG_SZ);
>>>> +    smmu->base = devm_ioremap_resource(dev, res);
>>>>        if (IS_ERR(smmu->base))
>>>>            return PTR_ERR(smmu->base);
>>>>    -    if (arm_smmu_resource_size(smmu) > SZ_64K) {
>>>> -        smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
>>>> -                           ARM_SMMU_REG_SZ);
>>>> -        if (IS_ERR(smmu->page1))
>>>> -            return PTR_ERR(smmu->page1);
>>>> -    } else {
>>>> +    if (arm_smmu_resource_size(smmu) > SZ_64K)
>>>> +        smmu->page1 = smmu->base + SZ_64K;
>>>> +    else
>>>>            smmu->page1 = smmu->base;
>>>> -    }
>>>>          /* Interrupt lines */
>>>>    diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>>> index da525f46dab4fc1..63f2b476987d6ae 100644
>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
>>>> @@ -152,8 +152,6 @@
>>>>    #define ARM_SMMU_PRIQ_IRQ_CFG1        0xd8
>>>>    #define ARM_SMMU_PRIQ_IRQ_CFG2        0xdc
>>>>    -#define ARM_SMMU_REG_SZ            0xe00
>>>> -
>>>>    /* Common MSI config fields */
>>>>    #define MSI_CFG0_ADDR_MASK        GENMASK_ULL(51, 2)
>>>>    #define MSI_CFG2_SH            GENMASK(5, 4)
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel