[PATCH 0/2] Use another method to avoid resource conflicts between the SMMU and PMCG drivers

[PATCH 0/2] Use another method to avoid resource conflicts between the SMMU and PMCG drivers

[Zhen Lei]

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 (2):
  perf/smmuv3: Don't reserve the register space that overlaps with the
    SMMUv3
  Revert "iommu/arm-smmu-v3: Don't reserve implementation defined
    register space"

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 +++-------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
 drivers/perf/arm_smmuv3_pmu.c               | 42 +++++++++++++++++++++++++++--
 3 files changed, 44 insertions(+), 33 deletions(-)

-- 
1.8.3

[PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Zhen Lei]

Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
SMMU driver reserves the corresponding resource first, this driver should
not reserve the corresponding resource again. Otherwise, a resource
reservation conflict is reported during boot.

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

diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
index 74474bb322c3f26..dcce085431c6ce8 100644
--- a/drivers/perf/arm_smmuv3_pmu.c
+++ b/drivers/perf/arm_smmuv3_pmu.c
@@ -761,6 +761,44 @@ 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 **out_res)
+{
+	int ret;
+	void __iomem *base;
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
+	if (!res) {
+		dev_err(&pdev->dev, "invalid resource\n");
+		return IOMEM_ERR_PTR(-EINVAL);
+	}
+	if (out_res)
+		*out_res = res;
+
+	ret = region_intersects(res->start, resource_size(res),
+				IORESOURCE_MEM, IORES_DESC_NONE);
+	if (ret == REGION_INTERSECTS) {
+		/*
+		 * The resource has already been reserved by the SMMUv3 driver.
+		 * Don't reserve it again, just do devm_ioremap().
+		 */
+		base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
+	} else {
+		/*
+		 * The resource may have not been reserved by any driver, or
+		 * has been reserved but not type IORESOURCE_MEM. In the latter
+		 * case, devm_ioremap_resource() reports a conflict and returns
+		 * IOMEM_ERR_PTR(-EBUSY).
+		 */
+		base = devm_ioremap_resource(&pdev->dev, res);
+	}
+
+	return base;
+}
+
 static int smmu_pmu_probe(struct platform_device *pdev)
 {
 	struct smmu_pmu *smmu_pmu;
@@ -793,7 +831,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 +839,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

[PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space"

[Zhen Lei]

This reverts commit 52f3fab0067d6fa9e99c1b7f63265dd48ca76046.

This problem has been fixed by another patch. The original method had side
effects, it was not mapped to the user-specified resource size. The code
will become more complex when ECMDQ is supported later.

Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++-------------------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
 2 files changed, 4 insertions(+), 31 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 8ca7415d785d9bf..477f473842e5272 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -91,8 +91,9 @@ struct arm_smmu_option_prop {
 static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
 						 struct arm_smmu_device *smmu)
 {
-	if (offset > SZ_64K)
-		return smmu->page1 + offset - SZ_64K;
+	if ((offset > SZ_64K) &&
+	    (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
+		offset -= SZ_64K;
 
 	return smmu->base + offset;
 }
@@ -3486,18 +3487,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 = {
-		.flags = IORESOURCE_MEM,
-		.start = start,
-		.end = start + size - 1,
-	};
-
-	return devm_ioremap_resource(dev, &res);
-}
-
 static int arm_smmu_device_probe(struct platform_device *pdev)
 {
 	int irq, ret;
@@ -3533,23 +3522,10 @@ 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 {
-		smmu->page1 = smmu->base;
-	}
-
 	/* Interrupt lines */
 
 	irq = platform_get_irq_byname_optional(pdev, "combined");
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 96c2e9565e00282..0c3090c60840c22 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)
@@ -584,7 +582,6 @@ struct arm_smmu_strtab_cfg {
 struct arm_smmu_device {
 	struct device			*dev;
 	void __iomem			*base;
-	void __iomem			*page1;
 
 #define ARM_SMMU_FEAT_2_LVL_STRTAB	(1 << 0)
 #define ARM_SMMU_FEAT_2_LVL_CDTAB	(1 << 1)
-- 
1.8.3

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Robin Murphy]

On 2021-01-19 01:59, Zhen Lei wrote:
> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
> SMMU driver reserves the corresponding resource first, this driver should
> not reserve the corresponding resource again. Otherwise, a resource
> reservation conflict is reported during boot.
> 
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
> index 74474bb322c3f26..dcce085431c6ce8 100644
> --- a/drivers/perf/arm_smmuv3_pmu.c
> +++ b/drivers/perf/arm_smmuv3_pmu.c
> @@ -761,6 +761,44 @@ 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 **out_res)
> +{
> +	int ret;
> +	void __iomem *base;
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> +	if (!res) {
> +		dev_err(&pdev->dev, "invalid resource\n");
> +		return IOMEM_ERR_PTR(-EINVAL);
> +	}
> +	if (out_res)
> +		*out_res = res;
> +
> +	ret = region_intersects(res->start, resource_size(res),
> +				IORESOURCE_MEM, IORES_DESC_NONE);
> +	if (ret == REGION_INTERSECTS) {
> +		/*
> +		 * The resource has already been reserved by the SMMUv3 driver.
> +		 * Don't reserve it again, just do devm_ioremap().
> +		 */
> +		base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> +	} else {
> +		/*
> +		 * The resource may have not been reserved by any driver, or
> +		 * has been reserved but not type IORESOURCE_MEM. In the latter
> +		 * case, devm_ioremap_resource() reports a conflict and returns
> +		 * IOMEM_ERR_PTR(-EBUSY).
> +		 */
> +		base = devm_ioremap_resource(&pdev->dev, res);
> +	}

What if the PMCG driver simply happens to probe first?

Robin.

> +
> +	return base;
> +}
> +
>   static int smmu_pmu_probe(struct platform_device *pdev)
>   {
>   	struct smmu_pmu *smmu_pmu;
> @@ -793,7 +831,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 +839,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 {
> 

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Leizhen (ThunderTown)]

On 2021/1/19 20:32, Robin Murphy wrote:
> On 2021-01-19 01:59, Zhen Lei wrote:
>> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
>> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
>> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
>> SMMU driver reserves the corresponding resource first, this driver should
>> not reserve the corresponding resource again. Otherwise, a resource
>> reservation conflict is reported during boot.
>>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>> index 74474bb322c3f26..dcce085431c6ce8 100644
>> --- a/drivers/perf/arm_smmuv3_pmu.c
>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>> @@ -761,6 +761,44 @@ 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 **out_res)
>> +{
>> +    int ret;
>> +    void __iomem *base;
>> +    struct resource *res;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>> +    if (!res) {
>> +        dev_err(&pdev->dev, "invalid resource\n");
>> +        return IOMEM_ERR_PTR(-EINVAL);
>> +    }
>> +    if (out_res)
>> +        *out_res = res;
>> +
>> +    ret = region_intersects(res->start, resource_size(res),
>> +                IORESOURCE_MEM, IORES_DESC_NONE);
>> +    if (ret == REGION_INTERSECTS) {
>> +        /*
>> +         * The resource has already been reserved by the SMMUv3 driver.
>> +         * Don't reserve it again, just do devm_ioremap().
>> +         */
>> +        base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>> +    } else {
>> +        /*
>> +         * The resource may have not been reserved by any driver, or
>> +         * has been reserved but not type IORESOURCE_MEM. In the latter
>> +         * case, devm_ioremap_resource() reports a conflict and returns
>> +         * IOMEM_ERR_PTR(-EBUSY).
>> +         */
>> +        base = devm_ioremap_resource(&pdev->dev, res);
>> +    }
> 
> What if the PMCG driver simply happens to probe first?

There are 4 cases:
1) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=y
   It's not allowed. Becase: ARM_SMMU_V3_PMU depends on ARM_SMMU_V3
   config ARM_SMMU_V3_PMU
         tristate "ARM SMMUv3 Performance Monitors Extension"
         depends on ARM64 && ACPI && ARM_SMMU_V3

2) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=m
   No problem, SMMUv3 will be initialized first.

3) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=y
   vi drivers/Makefile
   60 obj-y                           += iommu/
   172 obj-$(CONFIG_PERF_EVENTS)       += perf/

   This link sequence ensure that SMMUv3 driver will be initialized first.
   They are currently at the same initialization level.

4) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=m
   Sorry, I thought module dependencies were generated based on "depends on".
   But I tried it today，module dependencies are generated only when symbol
   dependencies exist. I should use MODULE_SOFTDEP() to explicitly mark the
   dependency. I will send V2 later.


> 
> Robin.
> 
>> +
>> +    return base;
>> +}
>> +
>>   static int smmu_pmu_probe(struct platform_device *pdev)
>>   {
>>       struct smmu_pmu *smmu_pmu;
>> @@ -793,7 +831,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 +839,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 {
>>
> 
> .
> 

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Leizhen (ThunderTown)]

On 2021/1/20 11:37, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/1/19 20:32, Robin Murphy wrote:
>> On 2021-01-19 01:59, Zhen Lei wrote:
>>> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
>>> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
>>> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
>>> SMMU driver reserves the corresponding resource first, this driver should
>>> not reserve the corresponding resource again. Otherwise, a resource
>>> reservation conflict is reported during boot.
>>>
>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> ---
>>>   drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 40 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>>> index 74474bb322c3f26..dcce085431c6ce8 100644
>>> --- a/drivers/perf/arm_smmuv3_pmu.c
>>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>>> @@ -761,6 +761,44 @@ 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 **out_res)
>>> +{
>>> +    int ret;
>>> +    void __iomem *base;
>>> +    struct resource *res;
>>> +
>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>>> +    if (!res) {
>>> +        dev_err(&pdev->dev, "invalid resource\n");
>>> +        return IOMEM_ERR_PTR(-EINVAL);
>>> +    }
>>> +    if (out_res)
>>> +        *out_res = res;
>>> +
>>> +    ret = region_intersects(res->start, resource_size(res),
>>> +                IORESOURCE_MEM, IORES_DESC_NONE);
>>> +    if (ret == REGION_INTERSECTS) {
>>> +        /*
>>> +         * The resource has already been reserved by the SMMUv3 driver.
>>> +         * Don't reserve it again, just do devm_ioremap().
>>> +         */
>>> +        base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>> +    } else {
>>> +        /*
>>> +         * The resource may have not been reserved by any driver, or
>>> +         * has been reserved but not type IORESOURCE_MEM. In the latter
>>> +         * case, devm_ioremap_resource() reports a conflict and returns
>>> +         * IOMEM_ERR_PTR(-EBUSY).
>>> +         */
>>> +        base = devm_ioremap_resource(&pdev->dev, res);
>>> +    }
>>
>> What if the PMCG driver simply happens to probe first?
> 
> There are 4 cases:
> 1) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=y
>    It's not allowed. Becase: ARM_SMMU_V3_PMU depends on ARM_SMMU_V3
>    config ARM_SMMU_V3_PMU
>          tristate "ARM SMMUv3 Performance Monitors Extension"
>          depends on ARM64 && ACPI && ARM_SMMU_V3
> 
> 2) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=m
>    No problem, SMMUv3 will be initialized first.
> 
> 3) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=y
>    vi drivers/Makefile
>    60 obj-y                           += iommu/
>    172 obj-$(CONFIG_PERF_EVENTS)       += perf/
> 
>    This link sequence ensure that SMMUv3 driver will be initialized first.
>    They are currently at the same initialization level.
> 
> 4) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=m
>    Sorry, I thought module dependencies were generated based on "depends on".
>    But I tried it today，module dependencies are generated only when symbol
>    dependencies exist. I should use MODULE_SOFTDEP() to explicitly mark the
>    dependency. I will send V2 later.
> 

Hi Robin:
  I think I misunderstood your question. The probe() instead of module_init()
determines the time for reserving register space resources.  So we'd better
reserve multiple small blocks of resources in SMMUv3 but perform ioremap() for
the entire resource, if the probe() of the PMCG occurs first.
  I'll refine these patches to make both initialization sequences work well.
I'm trying to send V2 this week.

> 
>>
>> Robin.
>>
>>> +
>>> +    return base;
>>> +}
>>> +
>>>   static int smmu_pmu_probe(struct platform_device *pdev)
>>>   {
>>>       struct smmu_pmu *smmu_pmu;
>>> @@ -793,7 +831,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 +839,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 {
>>>
>>
>> .
>>

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Robin Murphy]

On 2021-01-20 09:26, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/1/20 11:37, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/1/19 20:32, Robin Murphy wrote:
>>> On 2021-01-19 01:59, Zhen Lei wrote:
>>>> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
>>>> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
>>>> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
>>>> SMMU driver reserves the corresponding resource first, this driver should
>>>> not reserve the corresponding resource again. Otherwise, a resource
>>>> reservation conflict is reported during boot.
>>>>
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>>    drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>>>> index 74474bb322c3f26..dcce085431c6ce8 100644
>>>> --- a/drivers/perf/arm_smmuv3_pmu.c
>>>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>>>> @@ -761,6 +761,44 @@ 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 **out_res)
>>>> +{
>>>> +    int ret;
>>>> +    void __iomem *base;
>>>> +    struct resource *res;
>>>> +
>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>>>> +    if (!res) {
>>>> +        dev_err(&pdev->dev, "invalid resource\n");
>>>> +        return IOMEM_ERR_PTR(-EINVAL);
>>>> +    }
>>>> +    if (out_res)
>>>> +        *out_res = res;
>>>> +
>>>> +    ret = region_intersects(res->start, resource_size(res),
>>>> +                IORESOURCE_MEM, IORES_DESC_NONE);
>>>> +    if (ret == REGION_INTERSECTS) {
>>>> +        /*
>>>> +         * The resource has already been reserved by the SMMUv3 driver.
>>>> +         * Don't reserve it again, just do devm_ioremap().
>>>> +         */
>>>> +        base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>> +    } else {
>>>> +        /*
>>>> +         * The resource may have not been reserved by any driver, or
>>>> +         * has been reserved but not type IORESOURCE_MEM. In the latter
>>>> +         * case, devm_ioremap_resource() reports a conflict and returns
>>>> +         * IOMEM_ERR_PTR(-EBUSY).
>>>> +         */
>>>> +        base = devm_ioremap_resource(&pdev->dev, res);
>>>> +    }
>>>
>>> What if the PMCG driver simply happens to probe first?
>>
>> There are 4 cases:
>> 1) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=y
>>     It's not allowed. Becase: ARM_SMMU_V3_PMU depends on ARM_SMMU_V3
>>     config ARM_SMMU_V3_PMU
>>           tristate "ARM SMMUv3 Performance Monitors Extension"
>>           depends on ARM64 && ACPI && ARM_SMMU_V3
>>
>> 2) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=m
>>     No problem, SMMUv3 will be initialized first.
>>
>> 3) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=y
>>     vi drivers/Makefile
>>     60 obj-y                           += iommu/
>>     172 obj-$(CONFIG_PERF_EVENTS)       += perf/
>>
>>     This link sequence ensure that SMMUv3 driver will be initialized first.
>>     They are currently at the same initialization level.
>>
>> 4) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=m
>>     Sorry, I thought module dependencies were generated based on "depends on".
>>     But I tried it today，module dependencies are generated only when symbol
>>     dependencies exist. I should use MODULE_SOFTDEP() to explicitly mark the
>>     dependency. I will send V2 later.
>>
> 
> Hi Robin:
>    I think I misunderstood your question. The probe() instead of module_init()
> determines the time for reserving register space resources.  So we'd better
> reserve multiple small blocks of resources in SMMUv3 but perform ioremap() for
> the entire resource, if the probe() of the PMCG occurs first.
>    I'll refine these patches to make both initialization sequences work well.
> I'm trying to send V2 this week.

There's still the possibility that a PMCG is implemented in a root 
complex or some other device that isn't an SMMU, so as I've said before, 
none of this trickery really scales.

As far as I understand it, the main point of reserving resources is to 
catch bugs where things definitely should not be overlapping. PMCGs are 
by definition part of some other device, so in general they *can* be 
expected to overlap with whatever device that is. I still think it's 
most logical to simply *not* try to reserve PMCG resources at all.

Robin.

>>>> +
>>>> +    return base;
>>>> +}
>>>> +
>>>>    static int smmu_pmu_probe(struct platform_device *pdev)
>>>>    {
>>>>        struct smmu_pmu *smmu_pmu;
>>>> @@ -793,7 +831,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 +839,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 {
>>>>
>>>
>>> .
>>>
> 

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Leizhen (ThunderTown)]

On 2021/1/20 21:27, Robin Murphy wrote:
> On 2021-01-20 09:26, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/1/20 11:37, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2021/1/19 20:32, Robin Murphy wrote:
>>>> On 2021-01-19 01:59, Zhen Lei wrote:
>>>>> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
>>>>> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
>>>>> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
>>>>> SMMU driver reserves the corresponding resource first, this driver should
>>>>> not reserve the corresponding resource again. Otherwise, a resource
>>>>> reservation conflict is reported during boot.
>>>>>
>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>> ---
>>>>>    drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>    1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>>>>> index 74474bb322c3f26..dcce085431c6ce8 100644
>>>>> --- a/drivers/perf/arm_smmuv3_pmu.c
>>>>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>>>>> @@ -761,6 +761,44 @@ 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 **out_res)
>>>>> +{
>>>>> +    int ret;
>>>>> +    void __iomem *base;
>>>>> +    struct resource *res;
>>>>> +
>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>>>>> +    if (!res) {
>>>>> +        dev_err(&pdev->dev, "invalid resource\n");
>>>>> +        return IOMEM_ERR_PTR(-EINVAL);
>>>>> +    }
>>>>> +    if (out_res)
>>>>> +        *out_res = res;
>>>>> +
>>>>> +    ret = region_intersects(res->start, resource_size(res),
>>>>> +                IORESOURCE_MEM, IORES_DESC_NONE);
>>>>> +    if (ret == REGION_INTERSECTS) {
>>>>> +        /*
>>>>> +         * The resource has already been reserved by the SMMUv3 driver.
>>>>> +         * Don't reserve it again, just do devm_ioremap().
>>>>> +         */
>>>>> +        base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>>> +    } else {
>>>>> +        /*
>>>>> +         * The resource may have not been reserved by any driver, or
>>>>> +         * has been reserved but not type IORESOURCE_MEM. In the latter
>>>>> +         * case, devm_ioremap_resource() reports a conflict and returns
>>>>> +         * IOMEM_ERR_PTR(-EBUSY).
>>>>> +         */
>>>>> +        base = devm_ioremap_resource(&pdev->dev, res);
>>>>> +    }
>>>>
>>>> What if the PMCG driver simply happens to probe first?
>>>
>>> There are 4 cases:
>>> 1) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=y
>>>     It's not allowed. Becase: ARM_SMMU_V3_PMU depends on ARM_SMMU_V3
>>>     config ARM_SMMU_V3_PMU
>>>           tristate "ARM SMMUv3 Performance Monitors Extension"
>>>           depends on ARM64 && ACPI && ARM_SMMU_V3
>>>
>>> 2) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=m
>>>     No problem, SMMUv3 will be initialized first.
>>>
>>> 3) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=y
>>>     vi drivers/Makefile
>>>     60 obj-y                           += iommu/
>>>     172 obj-$(CONFIG_PERF_EVENTS)       += perf/
>>>
>>>     This link sequence ensure that SMMUv3 driver will be initialized first.
>>>     They are currently at the same initialization level.
>>>
>>> 4) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=m
>>>     Sorry, I thought module dependencies were generated based on "depends on".
>>>     But I tried it today，module dependencies are generated only when symbol
>>>     dependencies exist. I should use MODULE_SOFTDEP() to explicitly mark the
>>>     dependency. I will send V2 later.
>>>
>>
>> Hi Robin:
>>    I think I misunderstood your question. The probe() instead of module_init()
>> determines the time for reserving register space resources.  So we'd better
>> reserve multiple small blocks of resources in SMMUv3 but perform ioremap() for
>> the entire resource, if the probe() of the PMCG occurs first.
>>    I'll refine these patches to make both initialization sequences work well.
>> I'm trying to send V2 this week.
> 
> There's still the possibility that a PMCG is implemented in a root complex or some other device that isn't an SMMU, so as I've said before, none of this trickery really scales.
> 
> As far as I understand it, the main point of reserving resources is to catch bugs where things definitely should not be overlapping. PMCGs are by definition part of some other device, so in general they *can* be expected to overlap with whatever device that is. I still think it's most logical to simply *not* try to reserve PMCG resources at all.

If PMCG resources may overlap with other devices, that's really going to be a very tricky thing. OK, I'll follow your advice，just do ioremap() for the PMCG resources.

I know that I/O resource information can be queried by running "cat /proc/iomem". If we do not reserve PMCG resources, should we provide an additional fs query interface?

> 
> Robin.
> 
>>>>> +
>>>>> +    return base;
>>>>> +}
>>>>> +
>>>>>    static int smmu_pmu_probe(struct platform_device *pdev)
>>>>>    {
>>>>>        struct smmu_pmu *smmu_pmu;
>>>>> @@ -793,7 +831,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 +839,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 {
>>>>>
>>>>
>>>> .
>>>>
>>
> 
> .
> 

Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space"

[Robin Murphy]

On 2021-01-19 01:59, Zhen Lei wrote:
> This reverts commit 52f3fab0067d6fa9e99c1b7f63265dd48ca76046.
> 
> This problem has been fixed by another patch. The original method had side
> effects, it was not mapped to the user-specified resource size. The code
> will become more complex when ECMDQ is supported later.

FWIW I don't think that's a significant issue either way - there could 
be any number of imp-def pages between SMMU page 0 and the ECMDQ control 
pages, so it will still be logical to map them as another separate thing 
anyway.

Robin.

> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++-------------------------
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
>   2 files changed, 4 insertions(+), 31 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 8ca7415d785d9bf..477f473842e5272 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -91,8 +91,9 @@ struct arm_smmu_option_prop {
>   static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>   						 struct arm_smmu_device *smmu)
>   {
> -	if (offset > SZ_64K)
> -		return smmu->page1 + offset - SZ_64K;
> +	if ((offset > SZ_64K) &&
> +	    (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
> +		offset -= SZ_64K;
>   
>   	return smmu->base + offset;
>   }
> @@ -3486,18 +3487,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 = {
> -		.flags = IORESOURCE_MEM,
> -		.start = start,
> -		.end = start + size - 1,
> -	};
> -
> -	return devm_ioremap_resource(dev, &res);
> -}
> -
>   static int arm_smmu_device_probe(struct platform_device *pdev)
>   {
>   	int irq, ret;
> @@ -3533,23 +3522,10 @@ 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 {
> -		smmu->page1 = smmu->base;
> -	}
> -
>   	/* Interrupt lines */
>   
>   	irq = platform_get_irq_byname_optional(pdev, "combined");
> 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 96c2e9565e00282..0c3090c60840c22 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)
> @@ -584,7 +582,6 @@ struct arm_smmu_strtab_cfg {
>   struct arm_smmu_device {
>   	struct device			*dev;
>   	void __iomem			*base;
> -	void __iomem			*page1;
>   
>   #define ARM_SMMU_FEAT_2_LVL_STRTAB	(1 << 0)
>   #define ARM_SMMU_FEAT_2_LVL_CDTAB	(1 << 1)
> 

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Robin Murphy]

On 2021-01-20 14:14, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/1/20 21:27, Robin Murphy wrote:
>> On 2021-01-20 09:26, Leizhen (ThunderTown) wrote:
>>>
>>>
>>> On 2021/1/20 11:37, Leizhen (ThunderTown) wrote:
>>>>
>>>>
>>>> On 2021/1/19 20:32, Robin Murphy wrote:
>>>>> On 2021-01-19 01:59, Zhen Lei wrote:
>>>>>> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
>>>>>> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
>>>>>> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
>>>>>> SMMU driver reserves the corresponding resource first, this driver should
>>>>>> not reserve the corresponding resource again. Otherwise, a resource
>>>>>> reservation conflict is reported during boot.
>>>>>>
>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>> ---
>>>>>>     drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>     1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>>>>>> index 74474bb322c3f26..dcce085431c6ce8 100644
>>>>>> --- a/drivers/perf/arm_smmuv3_pmu.c
>>>>>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>>>>>> @@ -761,6 +761,44 @@ 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 **out_res)
>>>>>> +{
>>>>>> +    int ret;
>>>>>> +    void __iomem *base;
>>>>>> +    struct resource *res;
>>>>>> +
>>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>>>>>> +    if (!res) {
>>>>>> +        dev_err(&pdev->dev, "invalid resource\n");
>>>>>> +        return IOMEM_ERR_PTR(-EINVAL);
>>>>>> +    }
>>>>>> +    if (out_res)
>>>>>> +        *out_res = res;
>>>>>> +
>>>>>> +    ret = region_intersects(res->start, resource_size(res),
>>>>>> +                IORESOURCE_MEM, IORES_DESC_NONE);
>>>>>> +    if (ret == REGION_INTERSECTS) {
>>>>>> +        /*
>>>>>> +         * The resource has already been reserved by the SMMUv3 driver.
>>>>>> +         * Don't reserve it again, just do devm_ioremap().
>>>>>> +         */
>>>>>> +        base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>>>> +    } else {
>>>>>> +        /*
>>>>>> +         * The resource may have not been reserved by any driver, or
>>>>>> +         * has been reserved but not type IORESOURCE_MEM. In the latter
>>>>>> +         * case, devm_ioremap_resource() reports a conflict and returns
>>>>>> +         * IOMEM_ERR_PTR(-EBUSY).
>>>>>> +         */
>>>>>> +        base = devm_ioremap_resource(&pdev->dev, res);
>>>>>> +    }
>>>>>
>>>>> What if the PMCG driver simply happens to probe first?
>>>>
>>>> There are 4 cases:
>>>> 1) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=y
>>>>      It's not allowed. Becase: ARM_SMMU_V3_PMU depends on ARM_SMMU_V3
>>>>      config ARM_SMMU_V3_PMU
>>>>            tristate "ARM SMMUv3 Performance Monitors Extension"
>>>>            depends on ARM64 && ACPI && ARM_SMMU_V3
>>>>
>>>> 2) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=m
>>>>      No problem, SMMUv3 will be initialized first.
>>>>
>>>> 3) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=y
>>>>      vi drivers/Makefile
>>>>      60 obj-y                           += iommu/
>>>>      172 obj-$(CONFIG_PERF_EVENTS)       += perf/
>>>>
>>>>      This link sequence ensure that SMMUv3 driver will be initialized first.
>>>>      They are currently at the same initialization level.
>>>>
>>>> 4) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=m
>>>>      Sorry, I thought module dependencies were generated based on "depends on".
>>>>      But I tried it today，module dependencies are generated only when symbol
>>>>      dependencies exist. I should use MODULE_SOFTDEP() to explicitly mark the
>>>>      dependency. I will send V2 later.
>>>>
>>>
>>> Hi Robin:
>>>     I think I misunderstood your question. The probe() instead of module_init()
>>> determines the time for reserving register space resources.  So we'd better
>>> reserve multiple small blocks of resources in SMMUv3 but perform ioremap() for
>>> the entire resource, if the probe() of the PMCG occurs first.
>>>     I'll refine these patches to make both initialization sequences work well.
>>> I'm trying to send V2 this week.
>>
>> There's still the possibility that a PMCG is implemented in a root complex or some other device that isn't an SMMU, so as I've said before, none of this trickery really scales.
>>
>> As far as I understand it, the main point of reserving resources is to catch bugs where things definitely should not be overlapping. PMCGs are by definition part of some other device, so in general they *can* be expected to overlap with whatever device that is. I still think it's most logical to simply *not* try to reserve PMCG resources at all.
> 
> If PMCG resources may overlap with other devices, that's really going to be a very tricky thing. OK, I'll follow your advice，just do ioremap() for the PMCG resources.
> 
> I know that I/O resource information can be queried by running "cat /proc/iomem". If we do not reserve PMCG resources, should we provide an additional fs query interface?

What would that be necessary for? The PMU devices are already named by 
base address so they can be identified. Sysfs tells you whether the 
driver bound to any platform devices or not (although in this case the 
existence of PMU devices already makes that clear). /proc/iomem is for 
showing claimed resources, and many drivers don't claim resources. I've 
never heard any inkling of that being a practical problem :/

Robin.

>>>>>> +
>>>>>> +    return base;
>>>>>> +}
>>>>>> +
>>>>>>     static int smmu_pmu_probe(struct platform_device *pdev)
>>>>>>     {
>>>>>>         struct smmu_pmu *smmu_pmu;
>>>>>> @@ -793,7 +831,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 +839,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 {
>>>>>>
>>>>>
>>>>> .
>>>>>
>>>
>>
>> .
>>
> 

Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space"

[Leizhen (ThunderTown)]

On 2021/1/20 23:02, Robin Murphy wrote:
> On 2021-01-19 01:59, Zhen Lei wrote:
>> This reverts commit 52f3fab0067d6fa9e99c1b7f63265dd48ca76046.
>>
>> This problem has been fixed by another patch. The original method had side
>> effects, it was not mapped to the user-specified resource size. The code
>> will become more complex when ECMDQ is supported later.
> 
> FWIW I don't think that's a significant issue either way - there could be any number of imp-def pages between SMMU page 0 and the ECMDQ control pages, so it will still be logical to map them as another separate thing anyway.

Yes, so now I'm thinking of preserving the SMMUv3 resources and eliminating the imp-def area. Then use another devm_ioremap() to cover the entire resource，assign it to smmu->base.
Otherwise, a base pointer needs to be defined for each separated register space，or call a function to convert each time.

> 
> Robin.
> 
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++-------------------------
>>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
>>   2 files changed, 4 insertions(+), 31 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 8ca7415d785d9bf..477f473842e5272 100644
>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>> @@ -91,8 +91,9 @@ struct arm_smmu_option_prop {
>>   static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>>                            struct arm_smmu_device *smmu)
>>   {
>> -    if (offset > SZ_64K)
>> -        return smmu->page1 + offset - SZ_64K;
>> +    if ((offset > SZ_64K) &&
>> +        (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
>> +        offset -= SZ_64K;
>>         return smmu->base + offset;
>>   }
>> @@ -3486,18 +3487,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 = {
>> -        .flags = IORESOURCE_MEM,
>> -        .start = start,
>> -        .end = start + size - 1,
>> -    };
>> -
>> -    return devm_ioremap_resource(dev, &res);
>> -}
>> -
>>   static int arm_smmu_device_probe(struct platform_device *pdev)
>>   {
>>       int irq, ret;
>> @@ -3533,23 +3522,10 @@ 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 {
>> -        smmu->page1 = smmu->base;
>> -    }
>> -
>>       /* Interrupt lines */
>>         irq = platform_get_irq_byname_optional(pdev, "combined");
>> 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 96c2e9565e00282..0c3090c60840c22 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)
>> @@ -584,7 +582,6 @@ struct arm_smmu_strtab_cfg {
>>   struct arm_smmu_device {
>>       struct device            *dev;
>>       void __iomem            *base;
>> -    void __iomem            *page1;
>>     #define ARM_SMMU_FEAT_2_LVL_STRTAB    (1 << 0)
>>   #define ARM_SMMU_FEAT_2_LVL_CDTAB    (1 << 1)
>>
> 
> .
> 

Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3

[Leizhen (ThunderTown)]

On 2021/1/20 23:54, Robin Murphy wrote:
> On 2021-01-20 14:14, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/1/20 21:27, Robin Murphy wrote:
>>> On 2021-01-20 09:26, Leizhen (ThunderTown) wrote:
>>>>
>>>>
>>>> On 2021/1/20 11:37, Leizhen (ThunderTown) wrote:
>>>>>
>>>>>
>>>>> On 2021/1/19 20:32, Robin Murphy wrote:
>>>>>> On 2021-01-19 01:59, Zhen Lei wrote:
>>>>>>> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
>>>>>>> inside the first 64kB region of the SMMU. Since SMMU and PMCG are managed
>>>>>>> by two separate drivers, and this driver depends on ARM_SMMU_V3, so the
>>>>>>> SMMU driver reserves the corresponding resource first, this driver should
>>>>>>> not reserve the corresponding resource again. Otherwise, a resource
>>>>>>> reservation conflict is reported during boot.
>>>>>>>
>>>>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>>>>> ---
>>>>>>>     drivers/perf/arm_smmuv3_pmu.c | 42 ++++++++++++++++++++++++++++++++++++++++--
>>>>>>>     1 file changed, 40 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/perf/arm_smmuv3_pmu.c b/drivers/perf/arm_smmuv3_pmu.c
>>>>>>> index 74474bb322c3f26..dcce085431c6ce8 100644
>>>>>>> --- a/drivers/perf/arm_smmuv3_pmu.c
>>>>>>> +++ b/drivers/perf/arm_smmuv3_pmu.c
>>>>>>> @@ -761,6 +761,44 @@ 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 **out_res)
>>>>>>> +{
>>>>>>> +    int ret;
>>>>>>> +    void __iomem *base;
>>>>>>> +    struct resource *res;
>>>>>>> +
>>>>>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, index);
>>>>>>> +    if (!res) {
>>>>>>> +        dev_err(&pdev->dev, "invalid resource\n");
>>>>>>> +        return IOMEM_ERR_PTR(-EINVAL);
>>>>>>> +    }
>>>>>>> +    if (out_res)
>>>>>>> +        *out_res = res;
>>>>>>> +
>>>>>>> +    ret = region_intersects(res->start, resource_size(res),
>>>>>>> +                IORESOURCE_MEM, IORES_DESC_NONE);
>>>>>>> +    if (ret == REGION_INTERSECTS) {
>>>>>>> +        /*
>>>>>>> +         * The resource has already been reserved by the SMMUv3 driver.
>>>>>>> +         * Don't reserve it again, just do devm_ioremap().
>>>>>>> +         */
>>>>>>> +        base = devm_ioremap(&pdev->dev, res->start, resource_size(res));
>>>>>>> +    } else {
>>>>>>> +        /*
>>>>>>> +         * The resource may have not been reserved by any driver, or
>>>>>>> +         * has been reserved but not type IORESOURCE_MEM. In the latter
>>>>>>> +         * case, devm_ioremap_resource() reports a conflict and returns
>>>>>>> +         * IOMEM_ERR_PTR(-EBUSY).
>>>>>>> +         */
>>>>>>> +        base = devm_ioremap_resource(&pdev->dev, res);
>>>>>>> +    }
>>>>>>
>>>>>> What if the PMCG driver simply happens to probe first?
>>>>>
>>>>> There are 4 cases:
>>>>> 1) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=y
>>>>>      It's not allowed. Becase: ARM_SMMU_V3_PMU depends on ARM_SMMU_V3
>>>>>      config ARM_SMMU_V3_PMU
>>>>>            tristate "ARM SMMUv3 Performance Monitors Extension"
>>>>>            depends on ARM64 && ACPI && ARM_SMMU_V3
>>>>>
>>>>> 2) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=m
>>>>>      No problem, SMMUv3 will be initialized first.
>>>>>
>>>>> 3) ARM_SMMU_V3=y, ARM_SMMU_V3_PMU=y
>>>>>      vi drivers/Makefile
>>>>>      60 obj-y                           += iommu/
>>>>>      172 obj-$(CONFIG_PERF_EVENTS)       += perf/
>>>>>
>>>>>      This link sequence ensure that SMMUv3 driver will be initialized first.
>>>>>      They are currently at the same initialization level.
>>>>>
>>>>> 4) ARM_SMMU_V3=m, ARM_SMMU_V3_PMU=m
>>>>>      Sorry, I thought module dependencies were generated based on "depends on".
>>>>>      But I tried it today，module dependencies are generated only when symbol
>>>>>      dependencies exist. I should use MODULE_SOFTDEP() to explicitly mark the
>>>>>      dependency. I will send V2 later.
>>>>>
>>>>
>>>> Hi Robin:
>>>>     I think I misunderstood your question. The probe() instead of module_init()
>>>> determines the time for reserving register space resources.  So we'd better
>>>> reserve multiple small blocks of resources in SMMUv3 but perform ioremap() for
>>>> the entire resource, if the probe() of the PMCG occurs first.
>>>>     I'll refine these patches to make both initialization sequences work well.
>>>> I'm trying to send V2 this week.
>>>
>>> There's still the possibility that a PMCG is implemented in a root complex or some other device that isn't an SMMU, so as I've said before, none of this trickery really scales.
>>>
>>> As far as I understand it, the main point of reserving resources is to catch bugs where things definitely should not be overlapping. PMCGs are by definition part of some other device, so in general they *can* be expected to overlap with whatever device that is. I still think it's most logical to simply *not* try to reserve PMCG resources at all.
>>
>> If PMCG resources may overlap with other devices, that's really going to be a very tricky thing. OK, I'll follow your advice，just do ioremap() for the PMCG resources.
>>
>> I know that I/O resource information can be queried by running "cat /proc/iomem". If we do not reserve PMCG resources, should we provide an additional fs query interface?
> 
> What would that be necessary for? The PMU devices are already named by base address so they can be identified. Sysfs tells you whether the driver bound to any platform devices or not (although in this case the existence of PMU devices already makes that clear). /proc/iomem is for showing claimed resources, and many drivers don't claim resources. I've never heard any inkling of that being a practical problem :/
> 

OK, so keep the code of the PMCG driver unchanged. Currently, PMCG resources may not overlap with other devices except SMMUv3.

I sometimes use "cat /proc/ioomem" to look up the base address, then use devmem command to query the register content.

> Robin.
> 
>>>>>>> +
>>>>>>> +    return base;
>>>>>>> +}
>>>>>>> +
>>>>>>>     static int smmu_pmu_probe(struct platform_device *pdev)
>>>>>>>     {
>>>>>>>         struct smmu_pmu *smmu_pmu;
>>>>>>> @@ -793,7 +831,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 +839,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 {
>>>>>>>
>>>>>>
>>>>>> .
>>>>>>
>>>>
>>>
>>> .
>>>
>>
> 
> .
> 

Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space"

[Robin Murphy]

On 2021-01-21 02:04, Leizhen (ThunderTown) wrote:
> 
> 
> On 2021/1/20 23:02, Robin Murphy wrote:
>> On 2021-01-19 01:59, Zhen Lei wrote:
>>> This reverts commit 52f3fab0067d6fa9e99c1b7f63265dd48ca76046.
>>>
>>> This problem has been fixed by another patch. The original method had side
>>> effects, it was not mapped to the user-specified resource size. The code
>>> will become more complex when ECMDQ is supported later.
>>
>> FWIW I don't think that's a significant issue either way - there could be any number of imp-def pages between SMMU page 0 and the ECMDQ control pages, so it will still be logical to map them as another separate thing anyway.
> 
> Yes, so now I'm thinking of preserving the SMMUv3 resources and eliminating the imp-def area. Then use another devm_ioremap() to cover the entire resource，assign it to smmu->base.
> Otherwise, a base pointer needs to be defined for each separated register space，or call a function to convert each time.

But we'll almost certainly want to maintain a pointer to start of the 
ECMDQ control page block anyway, since that's not fixed relative to 
smmu->base. Therefore what's the harm in handling that via a dedicated 
mapping, once we've determined that we *do* intend to use ECMDQs? 
Otherwise we end up with in the complicated dance of trying to map 
"everything" up-front in order to be able to read the ID registers to 
determine what the actual extent of "everything" is supposed to be.

(also this reminds me that I was going to remove arm_smmu_page1_fixup() 
entirely - I'd totally forgotten about that...)

Robin.

>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>> ---
>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++-------------------------
>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
>>>    2 files changed, 4 insertions(+), 31 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 8ca7415d785d9bf..477f473842e5272 100644
>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>> @@ -91,8 +91,9 @@ struct arm_smmu_option_prop {
>>>    static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>>>                             struct arm_smmu_device *smmu)
>>>    {
>>> -    if (offset > SZ_64K)
>>> -        return smmu->page1 + offset - SZ_64K;
>>> +    if ((offset > SZ_64K) &&
>>> +        (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
>>> +        offset -= SZ_64K;
>>>          return smmu->base + offset;
>>>    }
>>> @@ -3486,18 +3487,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 = {
>>> -        .flags = IORESOURCE_MEM,
>>> -        .start = start,
>>> -        .end = start + size - 1,
>>> -    };
>>> -
>>> -    return devm_ioremap_resource(dev, &res);
>>> -}
>>> -
>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>    {
>>>        int irq, ret;
>>> @@ -3533,23 +3522,10 @@ 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 {
>>> -        smmu->page1 = smmu->base;
>>> -    }
>>> -
>>>        /* Interrupt lines */
>>>          irq = platform_get_irq_byname_optional(pdev, "combined");
>>> 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 96c2e9565e00282..0c3090c60840c22 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)
>>> @@ -584,7 +582,6 @@ struct arm_smmu_strtab_cfg {
>>>    struct arm_smmu_device {
>>>        struct device            *dev;
>>>        void __iomem            *base;
>>> -    void __iomem            *page1;
>>>      #define ARM_SMMU_FEAT_2_LVL_STRTAB    (1 << 0)
>>>    #define ARM_SMMU_FEAT_2_LVL_CDTAB    (1 << 1)
>>>
>>
>> .
>>
> 

Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space"

[Leizhen (ThunderTown)]

On 2021/1/21 20:50, Robin Murphy wrote:
> On 2021-01-21 02:04, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2021/1/20 23:02, Robin Murphy wrote:
>>> On 2021-01-19 01:59, Zhen Lei wrote:
>>>> This reverts commit 52f3fab0067d6fa9e99c1b7f63265dd48ca76046.
>>>>
>>>> This problem has been fixed by another patch. The original method had side
>>>> effects, it was not mapped to the user-specified resource size. The code
>>>> will become more complex when ECMDQ is supported later.
>>>
>>> FWIW I don't think that's a significant issue either way - there could be any number of imp-def pages between SMMU page 0 and the ECMDQ control pages, so it will still be logical to map them as another separate thing anyway.
>>
>> Yes, so now I'm thinking of preserving the SMMUv3 resources and eliminating the imp-def area. Then use another devm_ioremap() to cover the entire resource，assign it to smmu->base.
>> Otherwise, a base pointer needs to be defined for each separated register space，or call a function to convert each time.
> 
> But we'll almost certainly want to maintain a pointer to start of the ECMDQ control page block anyway, since that's not fixed relative to smmu->base. Therefore what's the harm in handling that via a dedicated mapping, once we've determined that we *do* intend to use ECMDQs? Otherwise we end up with in the complicated dance of trying to map "everything" up-front in order to be able to read the ID registers to determine what the actual extent of "everything" is supposed to be.

Currently, we only mapped the first 0xe00 size, so the SMMU_CMDQ_CONTROL_PAGE_XXXn registers space at offset 0x4000 should be mapped again.
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.

> 
> (also this reminds me that I was going to remove arm_smmu_page1_fixup() entirely - I'd totally forgotten about that...)

Ah, that patch you made is so clever.

> 
> Robin.
> 
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 32 ++++-------------------------
>>>>    drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  3 ---
>>>>    2 files changed, 4 insertions(+), 31 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 8ca7415d785d9bf..477f473842e5272 100644
>>>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
>>>> @@ -91,8 +91,9 @@ struct arm_smmu_option_prop {
>>>>    static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>>>>                             struct arm_smmu_device *smmu)
>>>>    {
>>>> -    if (offset > SZ_64K)
>>>> -        return smmu->page1 + offset - SZ_64K;
>>>> +    if ((offset > SZ_64K) &&
>>>> +        (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
>>>> +        offset -= SZ_64K;
>>>>          return smmu->base + offset;
>>>>    }
>>>> @@ -3486,18 +3487,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 = {
>>>> -        .flags = IORESOURCE_MEM,
>>>> -        .start = start,
>>>> -        .end = start + size - 1,
>>>> -    };
>>>> -
>>>> -    return devm_ioremap_resource(dev, &res);
>>>> -}
>>>> -
>>>>    static int arm_smmu_device_probe(struct platform_device *pdev)
>>>>    {
>>>>        int irq, ret;
>>>> @@ -3533,23 +3522,10 @@ 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 {
>>>> -        smmu->page1 = smmu->base;
>>>> -    }
>>>> -
>>>>        /* Interrupt lines */
>>>>          irq = platform_get_irq_byname_optional(pdev, "combined");
>>>> 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 96c2e9565e00282..0c3090c60840c22 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)
>>>> @@ -584,7 +582,6 @@ struct arm_smmu_strtab_cfg {
>>>>    struct arm_smmu_device {
>>>>        struct device            *dev;
>>>>        void __iomem            *base;
>>>> -    void __iomem            *page1;
>>>>      #define ARM_SMMU_FEAT_2_LVL_STRTAB    (1 << 0)
>>>>    #define ARM_SMMU_FEAT_2_LVL_CDTAB    (1 << 1)
>>>>
>>>
>>> .
>>>
>>
> 
> .
>