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.leiz...@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