linux-kernel - Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for SM8550

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1eee8bae-59f0-4066-9d04-8c3a5f750d3a@linaro.org>
Date: Sat, 16 Dec 2023 01:03:07 +0100
From: Konrad Dybcio <konrad.dybcio@...aro.org>
To: Robin Murphy <robin.murphy@....com>,
 Bibek Kumar Patro <quic_bibekkum@...cinc.com>,
 Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: will@...nel.org, joro@...tes.org, jsnitsel@...hat.com,
 quic_bjorande@...cinc.com, mani@...nel.org, quic_eberman@...cinc.com,
 robdclark@...omium.org, u.kleine-koenig@...gutronix.de, robh@...nel.org,
 vladimir.oltean@....com, quic_pkondeti@...cinc.com,
 quic_molvera@...cinc.com, linux-arm-msm@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, iommu@...ts.linux.dev,
 linux-kernel@...r.kernel.org, qipl.kernel.upstream@...cinc.com
Subject: Re: [PATCH v4 3/5] iommu/arm-smmu: add ACTLR data and support for
 SM8550

On 15.12.2023 13:54, Robin Murphy wrote:
> On 2023-12-15 12:20 pm, Bibek Kumar Patro wrote:
>>
>>
>> On 12/15/2023 4:14 PM, Dmitry Baryshkov wrote:
>>> On Fri, 15 Dec 2023 at 12:19, Bibek Kumar Patro
>>> <quic_bibekkum@...cinc.com> wrote:
>>>>
>>>> Add ACTLR data table for SM8550 along with support for
>>>> same including SM8550 specific implementation operations.
>>>>
>>>> Signed-off-by: Bibek Kumar Patro <quic_bibekkum@...cinc.com>
>>>> ---
>>>>   drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c | 89 ++++++++++++++++++++++
>>>>   1 file changed, 89 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> index cb49291f5233..d2006f610243 100644
>>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c
>>>> @@ -20,6 +20,85 @@ struct actlr_config {
>>>>          u32 actlr;
>>>>   };
>>>>
>>>> +/*
>>>> + * SMMU-500 TRM defines BIT(0) as CMTLB (Enable context caching in the
>>>> + * macro TLB) and BIT(1) as CPRE (Enable context caching in the prefetch
>>>> + * buffer). The remaining bits are implementation defined and vary across
>>>> + * SoCs.
>>>> + */
>>>> +
>>>> +#define PREFETCH_DEFAULT       0
>>>> +#define PREFETCH_SHALLOW       BIT(8)
>>>> +#define PREFETCH_MODERATE      BIT(9)
>>>> +#define PREFETCH_DEEP          (BIT(9) | BIT(8))
>>>
>>> I thin the following might be more correct:
>>>
>>> #include <linux/bitfield.h>
>>>
>>> #define PREFETCH_MASK GENMASK(9, 8)
>>> #define PREFETCH_DEFAULT FIELD_PREP(PREFETCH_MASK, 0)
>>> #define PREFETCH_SHALLOW FIELD_PREP(PREFETCH_MASK, 1)
>>> #define PREFETCH_MODERATE FIELD_PREP(PREFETCH_MASK, 2)
>>> #define PREFETCH_DEEP FIELD_PREP(PREFETCH_MASK, 3)
>>>
>>
>> Ack, thanks for this suggestion. Let me try this out using
>> GENMASK. Once tested, will take care of this in next version.
> 
> FWIW the more typical usage would be to just define the named macros for the raw field values, then put the FIELD_PREP() at the point of use. However in this case that's liable to get pretty verbose, so although I'm usually a fan of bitfield.h, the most readable option here might actually be to stick with simpler definitions of "(0 << 8)", "(1 << 8)", etc. However it's not really a big deal either way, and I defer to whatever Dmitry and Konrad prefer, since they're the ones looking after arm-smmu-qcom the most :)
My 5 cents would be to just use the "common" style of doing this, so:

#define ACTRL_PREFETCH	GENMASK(9, 8)
 #define PREFETCH_DEFAULT 0
 #define PREFETCH_SHALLOW 1
 #define PREFETCH_MODERATE 2
 #define PREFETCH_DEEP 3

and then use 

| FIELD_PREP(ACTRL_PREFETCH, PREFETCH_x)

it can get verbose, but.. arguably that's good, since you really want
to make sure the right bits are set here

Konrad