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: <CAA8EJpoMwrwQ9wBZE6AcobLLkCchFtN23SnHhw3enNOfX3CzTQ@mail.gmail.com>
Date: Tue, 19 Dec 2023 12:21:58 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Bibek Kumar Patro <quic_bibekkum@...cinc.com>
Cc: Konrad Dybcio <konrad.dybcio@...aro.org>, Robin Murphy <robin.murphy@....com>, 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 Tue, 19 Dec 2023 at 10:25, Bibek Kumar Patro
<quic_bibekkum@...cinc.com> wrote:
>
>
>
> 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)

2 << 8. Isn't that hard.

> #define PREFETCH_DEEP          (3 << 8)
>
> This should be okay I think ?
>
> Thanks,
> Bibek
>


-- 
With best wishes
Dmitry