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
