Re: [PATCH 1/3] iommu/arm-smmu: introduction of ACTLR for custom prefetcher settings

[Bibek Kumar Patro <quic_bibekkum@quicinc.com>]

On 11/6/2023 11:42 AM, Bibek Kumar Patro wrote:
> 
> 
> On 11/4/2023 3:33 AM, Dmitry Baryshkov wrote:
>> On Fri, 3 Nov 2023 at 23:53, Bibek Kumar Patro
>> <quic_bibekkum@quicinc.com> wrote:
>>>
>>> Currently in Qualcomm  SoCs the default prefetch is set to 1 which 
>>> allows
>>> the TLB to fetch just the next page table. MMU-500 features ACTLR
>>> register which is implementation defined and is used for Qualcomm SoCs
>>> to have a prefetch setting of 1/3/7/15 enabling TLB to prefetch
>>> the next set of page tables accordingly allowing for faster 
>>> translations.
>>>
>>> ACTLR value is unique for each SMR (Stream matching register) and stored
>>> in a pre-populated table. This value is set to the register during
>>> context bank initialisation.
>>>
>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@quicinc.com>
>>> ---
>>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 34 ++++++++++++++++++++++
>>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h |  2 ++
>>>   drivers/iommu/arm/arm-smmu/arm-smmu.c      |  5 ++--
>>>   drivers/iommu/arm/arm-smmu/arm-smmu.h      |  5 ++++
>>>   4 files changed, 44 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> index ae7cae015193..68c1f4908473 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>> @@ -14,6 +14,17 @@
>>>
>>>   #define QCOM_DUMMY_VAL -1
>>>
>>> +struct actlr_config {
>>> +       const struct actlr_data *adata;
>>> +       u32 size;
>>
>> This should be size_t.
>>
>> Also could you please drop the separate struct actlr_config and move
>> these two fields into struct qcom_smmu_config.
>>
> 
> Ack, will address both these inputs in the next patch.
> 

Dimitry, Tried moving both fields to qcom_smmu_config but since
actlr_data need to be a pointer to array and not scalar, size needs
to be calculated dynamically for each SoC data in a loop which is 
doable.But readily available implementations like ARRAY_SIZE cannot be 
used, so I think this extra struct indirection of actlr_config would be 
beneficial.
Some drivers like llcc (drivers/soc/qcom/llcc-qcom.c) is also using
similar implementation, I believe for the same reason.

regards,
Bibek
>>> +};
>>> +
>>> +struct actlr_data {
>>> +       u16 sid;
>>> +       u16 mask;
>>> +       u32 actlr;
>>> +};
>>> +
>>>   static struct qcom_smmu *to_qcom_smmu(struct arm_smmu_device *smmu)
>>>   {
>>>          return container_of(smmu, struct qcom_smmu, smmu);
>>> @@ -270,6 +281,26 @@ static const struct of_device_id 
>>> qcom_smmu_client_of_match[] __maybe_unused = {
>>>   static int qcom_smmu_init_context(struct arm_smmu_domain *smmu_domain,
>>>                  struct io_pgtable_cfg *pgtbl_cfg, struct device *dev)
>>>   {
>>> +       struct arm_smmu_device *smmu = smmu_domain->smmu;
>>> +       struct qcom_smmu *qsmmu = to_qcom_smmu(smmu);
>>> +       const struct actlr_config *actlrcfg;
>>> +       struct arm_smmu_smr *smr = smmu->smrs;
>>> +       int idx = smmu_domain->cfg.cbndx;
>>> +       int i;
>>> +       u16 id;
>>> +       u16 mask;
>>> +
>>> +       if (qsmmu->actlrcfg) {
>>> +               actlrcfg = qsmmu->actlrcfg;
>>> +               for (i = 0; i < actlrcfg->size; ++i) {
>>> +                       id = actlrcfg->adata[i].sid;
>>> +                       mask = actlrcfg->adata[i].mask;
>>> +                       if (!smr_is_subset(*smr, id, mask))
>>> +                               arm_smmu_cb_write(smmu, idx, 
>>> ARM_SMMU_CB_ACTLR,
>>> +                                               
>>> actlrcfg->adata[i].actlr);
>>> +               }
>>> +       }
>>
>> Consider extracting this to a separate function. This way you can
>> reduce 4 indentation levels into a single loop.
>>
> 
> Ack, thanks for this sugestion. Will move this entire for loop into a 
> separate function for simplicity reduced indent levels.
> 
>>> +
>>>          smmu_domain->cfg.flush_walk_prefer_tlbiasid = true;
>>>
>>>          return 0;
>>> @@ -459,6 +490,9 @@ static struct arm_smmu_device 
>>> *qcom_smmu_create(struct arm_smmu_device *smmu,
>>>          qsmmu->smmu.impl = impl;
>>>          qsmmu->cfg = data->cfg;
>>>
>>> +       if (data->actlrcfg && (data->actlrcfg->size))
>>> +               qsmmu->actlrcfg = data->actlrcfg;
>>> +
>>>          return &qsmmu->smmu;
>>>   }
>>>
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> index 593910567b88..4b6862715070 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.h
>>> @@ -9,6 +9,7 @@
>>>   struct qcom_smmu {
>>>          struct arm_smmu_device smmu;
>>>          const struct qcom_smmu_config *cfg;
>>> +       const struct actlr_config *actlrcfg;
>>>          bool bypass_quirk;
>>>          u8 bypass_cbndx;
>>>          u32 stall_enabled;
>>> @@ -25,6 +26,7 @@ struct qcom_smmu_config {
>>>   };
>>>
>>>   struct qcom_smmu_match_data {
>>> +       const struct actlr_config *actlrcfg;
>>>          const struct qcom_smmu_config *cfg;
>>>          const struct arm_smmu_impl *impl;
>>>          const struct arm_smmu_impl *adreno_impl;
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index 4c79ef6f4c75..38ac1cbc799b 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -992,9 +992,10 @@ static int arm_smmu_find_sme(struct 
>>> arm_smmu_device *smmu, u16 id, u16 mask)
>>>                   * expect simply identical entries for this case, 
>>> but there's
>>>                   * no harm in accommodating the generalisation.
>>>                   */
>>> -               if ((mask & smrs[i].mask) == mask &&
>>> -                   !((id ^ smrs[i].id) & ~smrs[i].mask))
>>> +
>>> +               if (smr_is_subset(smrs[i], id, mask))
>>>                          return i;
>>> +
>>>                  /*
>>>                   * If the new entry has any other overlap with an 
>>> existing one,
>>>                   * though, then there always exists at least one 
>>> stream ID
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h 
>>> b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> index 703fd5817ec1..b1638bbc41d4 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
>>> @@ -501,6 +501,11 @@ static inline void arm_smmu_writeq(struct 
>>> arm_smmu_device *smmu, int page,
>>>                  writeq_relaxed(val, arm_smmu_page(smmu, page) + 
>>> offset);
>>>   }
>>>
>>> +static inline bool smr_is_subset(struct arm_smmu_smr smrs, u16 id, 
>>> u16 mask)
>>> +{
>>> +       return (mask & smrs.mask) == mask && !((id ^ smrs.id) & 
>>> ~smrs.mask);
>>> +}
>>> +
>>>   #define ARM_SMMU_GR0           0
>>>   #define ARM_SMMU_GR1           1
>>>   #define ARM_SMMU_CB(s, n)      ((s)->numpage + (n))
>>> -- 
>>> 2.17.1
>>>
>>
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel