| 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
| ||
|
Message-ID: <a363c860-62be-43a7-930c-cab8a6f3fa6c@quicinc.com>
Date: Tue, 19 Dec 2023 13:54:55 +0530
From: Bibek Kumar Patro <quic_bibekkum@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
Konrad Dybcio
<konrad.dybcio@...aro.org>,
Robin Murphy <robin.murphy@....com>
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 12/18/2023 7:51 PM, Dmitry Baryshkov wrote:
> On 18/12/2023 13:23, Bibek Kumar Patro wrote:
>>
>>
>> On 12/16/2023 9:45 PM, Dmitry Baryshkov wrote:
>>> On 16/12/2023 02:03, Konrad Dybcio wrote:
>>>> 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
>>>
>>> Sounds good to me.
>>>
>>
>> Konrad, Dimitry, just checked FIELD_PREP() implementation
>>
>> #define FIELD_FIT(_mask, _val)
>> ({ \
>> __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>> ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \
>> })
>>
>> since it is defined as a block, it won't be possible to use FIELD_PREP
>> in macro or as a structure value, and can only be used inside a
>> block/function. Orelse would show compilation errors as following
>>
>> kernel/drivers/iommu/arm/arm-smmu/arm-smmu-qcom.c:94:20: note: in
>> expansion of macro 'PREFETCH_SHALLOW'
>> { 0x1947, 0x0000, PREFETCH_SHALLOW | CPRE | CMTLB },
>> ^
>> kernel/include/linux/bitfield.h:113:2: error: braced-group within
>> expression allowed only inside a function
>> ({ \
>> ^
>>
>> So as per my understanding I think, we might need to go ahead with the
>> generic implementation only. Let me know if I missed something.
>
> Then anyway (foo << bar) is better compared to BIT(n) | BIT(m).
>
Sure Dmitry, (foo << bar) would be simpler as well as Robin mentioned
earlier in his reply.
I can implement the defines as:
#define PREFETCH_DEFAULT 0
#define PREFETCH_SHALLOW (1 << 8)
#define PREFETCH_MODERATE (1 << 9)
#define PREFETCH_DEEP (3 << 8)
This should be okay I think ?
Thanks,
Bibek
Powered by blists - more mailing lists