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

[Leizhen (ThunderTown)]

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

Currently, we only mapped the first 0xe00 size, so the SMMU_CMDQ_CONTROL_PAGE_XXXn registers space at offset 0x4000 should be mapped again.
The size of this ECMDQ resource is not fixed, depending on SMMU_IDR6.CMDQ_CONTROL_PAGE_LOG2NUMQ.
Processing its resource reservation to avoid resource conflict with PMCG is a bit more complicated.

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

Ah, that patch you made is so clever.

> 
> Robin.
> 
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@xxxxxxxxxx>
>>>> ---
>>>>    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)
>>>>
>>>
>>> .
>>>
>>
> 
> .
>