* [PATCH 0/2] Use another method to avoid resource conflicts between the SMMU and PMCG drivers @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Zhen Lei, Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/2] Use another method to avoid resource conflicts between the SMMU and PMCG drivers @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum, 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 0/2] Use another method to avoid resource conflicts between the SMMU and PMCG drivers @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-19 1:59 ` Zhen Lei (?) @ 2021-01-19 1:59 ` Zhen Lei -1 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Zhen Lei, Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum, 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-19 1:59 ` Zhen Lei (?) @ 2021-01-19 12:32 ` Robin Murphy -1 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-19 12:32 UTC (permalink / raw) To: Zhen Lei, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 { > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-19 12:32 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-19 12:32 UTC (permalink / raw) To: Zhen Lei, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 { > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-19 12:32 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-19 12:32 UTC (permalink / raw) To: Zhen Lei, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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 { > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-19 12:32 ` Robin Murphy (?) @ 2021-01-20 3:37 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 3:37 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 { >> > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 3:37 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 3:37 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 { >> > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 3:37 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 3:37 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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 { >> > > . > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-20 3:37 ` Leizhen (ThunderTown) (?) @ 2021-01-20 9:26 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 9:26 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 { >>> >> >> . >> ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 9:26 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 9:26 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 { >>> >> >> . >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 9:26 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 9:26 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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 { >>> >> >> . >> _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-20 9:26 ` Leizhen (ThunderTown) (?) @ 2021-01-20 13:27 ` Robin Murphy -1 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 13:27 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>> >>> >>> . >>> > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 13:27 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 13:27 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>> >>> >>> . >>> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 13:27 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 13:27 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker 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 { >>>> >>> >>> . >>> > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-20 13:27 ` Robin Murphy (?) @ 2021-01-20 14:14 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 14:14 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>>> >>>> >>>> . >>>> >> > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 14:14 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 14:14 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>>> >>>> >>>> . >>>> >> > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 14:14 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-20 14:14 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker 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 { >>>>> >>>> >>>> . >>>> >> > > . > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-20 14:14 ` Leizhen (ThunderTown) (?) @ 2021-01-20 15:54 ` Robin Murphy -1 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 15:54 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>>>> >>>>> >>>>> . >>>>> >>> >> >> . >> > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 15:54 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 15:54 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>>>> >>>>> >>>>> . >>>>> >>> >> >> . >> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-20 15:54 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 15:54 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker 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 { >>>>>> >>>>> >>>>> . >>>>> >>> >> >> . >> > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 2021-01-20 15:54 ` Robin Murphy (?) @ 2021-01-21 2:19 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-21 2:19 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>>>>> >>>>>> >>>>>> . >>>>>> >>>> >>> >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-21 2:19 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-21 2:19 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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 { >>>>>>> >>>>>> >>>>>> . >>>>>> >>>> >>> >>> . >>> >> > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 @ 2021-01-21 2:19 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-21 2:19 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker 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 { >>>>>>> >>>>>> >>>>>> . >>>>>> >>>> >>> >>> . >>> >> > > . > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" 2021-01-19 1:59 ` Zhen Lei (?) @ 2021-01-19 1:59 ` Zhen Lei -1 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Zhen Lei, Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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 ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum, 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 42+ messages in thread
* [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-19 1:59 ` Zhen Lei 0 siblings, 0 replies; 42+ messages in thread From: Zhen Lei @ 2021-01-19 1:59 UTC (permalink / raw) To: Will Deacon, Robin Murphy, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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 _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply related [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" 2021-01-19 1:59 ` Zhen Lei (?) @ 2021-01-20 15:02 ` Robin Murphy -1 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 15:02 UTC (permalink / raw) To: Zhen Lei, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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) > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-20 15:02 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 15:02 UTC (permalink / raw) To: Zhen Lei, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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) > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-20 15:02 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-20 15:02 UTC (permalink / raw) To: Zhen Lei, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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) > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" 2021-01-20 15:02 ` Robin Murphy (?) @ 2021-01-21 2:04 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-21 2:04 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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) >> > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-21 2:04 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-21 2:04 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder, Shameer Kolothum 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) >> > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-21 2:04 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-21 2:04 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Neil Leeder 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) >> > > . > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" 2021-01-21 2:04 ` Leizhen (ThunderTown) (?) @ 2021-01-21 12:50 ` Robin Murphy -1 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-21 12:50 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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) >>> >> >> . >> > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-21 12:50 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-21 12:50 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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) >>> >> >> . >> > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-21 12:50 ` Robin Murphy 0 siblings, 0 replies; 42+ messages in thread From: Robin Murphy @ 2021-01-21 12:50 UTC (permalink / raw) To: Leizhen (ThunderTown), Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker 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) >>> >> >> . >> > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" 2021-01-21 12:50 ` Robin Murphy (?) @ 2021-01-22 2:50 ` Leizhen (ThunderTown) -1 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-22 2:50 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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) >>>> >>> >>> . >>> >> > > . > ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-22 2:50 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-22 2:50 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker, Shameer Kolothum 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) >>>> >>> >>> . >>> >> > > . > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 42+ messages in thread
* Re: [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" @ 2021-01-22 2:50 ` Leizhen (ThunderTown) 0 siblings, 0 replies; 42+ messages in thread From: Leizhen (ThunderTown) @ 2021-01-22 2:50 UTC (permalink / raw) To: Robin Murphy, Will Deacon, Mark Rutland, Joerg Roedel, linux-arm-kernel, iommu, linux-kernel Cc: Jean-Philippe Brucker 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) >>>> >>> >>> . >>> >> > > . > _______________________________________________ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu ^ permalink raw reply [flat|nested] 42+ messages in thread
end of thread, other threads:[~2021-01-22 2:52 UTC | newest] Thread overview: 42+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-19 1:59 [PATCH 0/2] Use another method to avoid resource conflicts between the SMMU and PMCG drivers Zhen Lei 2021-01-19 1:59 ` Zhen Lei 2021-01-19 1:59 ` Zhen Lei 2021-01-19 1:59 ` [PATCH 1/2] perf/smmuv3: Don't reserve the register space that overlaps with the SMMUv3 Zhen Lei 2021-01-19 1:59 ` Zhen Lei 2021-01-19 1:59 ` Zhen Lei 2021-01-19 12:32 ` Robin Murphy 2021-01-19 12:32 ` Robin Murphy 2021-01-19 12:32 ` Robin Murphy 2021-01-20 3:37 ` Leizhen (ThunderTown) 2021-01-20 3:37 ` Leizhen (ThunderTown) 2021-01-20 3:37 ` Leizhen (ThunderTown) 2021-01-20 9:26 ` Leizhen (ThunderTown) 2021-01-20 9:26 ` Leizhen (ThunderTown) 2021-01-20 9:26 ` Leizhen (ThunderTown) 2021-01-20 13:27 ` Robin Murphy 2021-01-20 13:27 ` Robin Murphy 2021-01-20 13:27 ` Robin Murphy 2021-01-20 14:14 ` Leizhen (ThunderTown) 2021-01-20 14:14 ` Leizhen (ThunderTown) 2021-01-20 14:14 ` Leizhen (ThunderTown) 2021-01-20 15:54 ` Robin Murphy 2021-01-20 15:54 ` Robin Murphy 2021-01-20 15:54 ` Robin Murphy 2021-01-21 2:19 ` Leizhen (ThunderTown) 2021-01-21 2:19 ` Leizhen (ThunderTown) 2021-01-21 2:19 ` Leizhen (ThunderTown) 2021-01-19 1:59 ` [PATCH 2/2] Revert "iommu/arm-smmu-v3: Don't reserve implementation defined register space" Zhen Lei 2021-01-19 1:59 ` Zhen Lei 2021-01-19 1:59 ` Zhen Lei 2021-01-20 15:02 ` Robin Murphy 2021-01-20 15:02 ` Robin Murphy 2021-01-20 15:02 ` Robin Murphy 2021-01-21 2:04 ` Leizhen (ThunderTown) 2021-01-21 2:04 ` Leizhen (ThunderTown) 2021-01-21 2:04 ` Leizhen (ThunderTown) 2021-01-21 12:50 ` Robin Murphy 2021-01-21 12:50 ` Robin Murphy 2021-01-21 12:50 ` Robin Murphy 2021-01-22 2:50 ` Leizhen (ThunderTown) 2021-01-22 2:50 ` Leizhen (ThunderTown) 2021-01-22 2:50 ` Leizhen (ThunderTown)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.