linux-kernel - Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

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: <CAPLW+4=QOv_gAov2KHC4zR881CV3igESMH5JU7XgLbDLS8UNAg@mail.gmail.com>
Date:   Thu, 14 Jul 2022 16:11:07 +0300
From:   Sam Protsenko <semen.protsenko@...aro.org>
To:     Robin Murphy <robin.murphy@....com>
Cc:     Marek Szyprowski <m.szyprowski@...sung.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Joerg Roedel <joro@...tes.org>, Will Deacon <will@...nel.org>,
        Janghyuck Kim <janghyuck.kim@...sung.com>,
        Cho KyongHo <pullip.cho@...sung.com>,
        Daniel Mentz <danielmentz@...gle.com>,
        David Virag <virag.david003@...il.com>,
        Sumit Semwal <sumit.semwal@...aro.org>, iommu@...ts.linux.dev,
        linux-arm-kernel@...ts.infradead.org,
        linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/7] iommu/exynos: Use lookup based approach to access registers

On Tue, 12 Jul 2022 at 19:24, Robin Murphy <robin.murphy@....com> wrote:

[snip]

> > No functional change here, just a refactoring patch.
>
> FWIW I'd say that this absolutely *is* a functional change. Achieving
> the same end result, but fundamentally changing the mechanism used to
> get there, is a bit different to simply moving code around.
>

As I understand, usually the "functional change" means some change
that can be observed from the user's point of view (i.e. user of this
driver). But ok, I'll clarify this bit in the commit message.

[snip]

> > +/*
> > + * Some SysMMU versions might not implement some registers from this set, thus
> > + * those registers shouldn't be accessed. Set the offsets for those registers to
> > + * 0x1 to trigger an unaligned access exception, which can help one to debug
> > + * related issues.
> > + */
> > +static const unsigned int sysmmu_regs[MAX_REG_SET][MAX_REG_IDX] = {
>
> Do we really need MAX_REG_SET? Maybe there's a consistency argument, I
> guess :/
>

Here and below: I reworked the register table using approach suggested
by Krzysztof, so those enums won't be present in v2 at all.

> > +     /* SysMMU v1..v3 */
> > +     {
> > +             0x00, 0x04, 0x08, 0x14, 0x0c, 0x10, 0x1, 0x1, 0x1,
> > +             0x18, 0x1c,
>
> This looks fragile and unnecessarily difficult to follow and maintain -
> designated initialisers would be a lot better in all respects, i.e.:
>
>         [REG_SET_V1] = {
>                 ...
>                 [IDX_PT_BASE] = REG_PT_BASE_ADDR,
>                 ...
>
> etc.
>

[snip]

> >   static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> > @@ -317,31 +348,33 @@ static void __sysmmu_tlb_invalidate_entry(struct sysmmu_drvdata *data,
> >
> >       if (MMU_MAJ_VER(data->version) < 5) {
> >               for (i = 0; i < num_inv; i++) {
> > -                     writel((iova & SPAGE_MASK) | 1,
> > -                                  data->sfrbase + REG_MMU_FLUSH_ENTRY);
> > +                     sysmmu_write(data, IDX_FLUSH_ENTRY,
> > +                                  (iova & SPAGE_MASK) | 0x1);
> >                       iova += SPAGE_SIZE;
> >               }
> >       } else {
> >               if (num_inv == 1) {
>
> You could merge this condition into the one above now. That much I'd
> call non-functional refactoring ;)
>

Done, thanks.

[snip]

> > +
> > +static void sysmmu_get_hw_info(struct sysmmu_drvdata *data)
> > +{
>
> Seems a bit unnecessary to split the call up like this - I'd say the
> register set is fundamentally connected to the version, and
> "get_hw_info" is even less meaningfully descriptive than just having
> "get_version" take care of one more assignment, but hey ho, it's not my
> driver.
>

Guess I was looking into downstream vendor's kernel too much :) They
do a bit more things in this function -- like getting TLBs number and
"no block mode" capability; that's why I renamed it. Anyway, don't
have a strong opinion on this one, will use the old name in v2.

Thanks for the review!

> Thanks,
> Robin.