netdev - Re: [PATCH v5 04/17] octeontx2-pf: Initialize and config queues

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] [day] [month] [year] [list]

Date:   Mon, 27 Jan 2020 18:36:10 +0530
From:   Sunil Kovvuri <sunil.kovvuri@...il.com>
To:     David Miller <davem@...emloft.net>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        Jakub Kicinski <kubakici@...pl>,
        Michal Kubecek <mkubecek@...e.cz>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Sunil Goutham <sgoutham@...vell.com>,
        Geetha sowjanya <gakula@...vell.com>
Subject: Re: [PATCH v5 04/17] octeontx2-pf: Initialize and config queues

On Mon, Jan 27, 2020 at 1:25 AM David Miller <davem@...emloft.net> wrote:
>
> From: Sunil Kovvuri <sunil.kovvuri@...il.com>
> Date: Sun, 26 Jan 2020 23:30:04 +0530
>
> > On Sun, Jan 26, 2020 at 4:31 PM David Miller <davem@...emloft.net> wrote:
> >>
> >> From: sunil.kovvuri@...il.com
> >> Date: Fri, 24 Jan 2020 23:15:42 +0530
> >>
> >> > @@ -184,6 +192,72 @@ static inline void otx2_mbox_unlock(struct mbox *mbox)
> >> >       mutex_unlock(&mbox->lock);
> >> >  }
> >> >
> >> > +/* With the absence of API for 128-bit IO memory access for arm64,
> >> > + * implement required operations at place.
> >> > + */
> >> > +#if defined(CONFIG_ARM64)
> >> > +static inline void otx2_write128(u64 lo, u64 hi, void __iomem *addr)
> >> > +{
> >> > +     __asm__ volatile("stp %x[x0], %x[x1], [%x[p1],#0]!"
> >> > +                      ::[x0]"r"(lo), [x1]"r"(hi), [p1]"r"(addr));
> >> > +}
> >> > +
> >> > +static inline u64 otx2_atomic64_add(u64 incr, u64 *ptr)
> >> > +{
> >> > +     u64 result;
> >> > +
> >> > +     __asm__ volatile(".cpu   generic+lse\n"
> >> > +                      "ldadd %x[i], %x[r], [%[b]]"
> >> > +                      : [r]"=r"(result), "+m"(*ptr)
> >> > +                      : [i]"r"(incr), [b]"r"(ptr)
> >> > +                      : "memory");
> >> > +     return result;
> >> > +}
> >> > +
> >> > +#else
> >> > +#define otx2_write128(lo, hi, addr)
> >> > +#define otx2_atomic64_add(incr, ptr)         ({ *ptr = incr; })
> >> > +#endif
> >>
> >> So what exactly is going on here?  Are these true 128-bit writes
> >> and atomic operations?  Why is it named atomic64 then?  Why can't
> >> the normal atomic64 kernel interfaces be used?
> >
> > otx2_write128() is used to free receive buffer pointers into buffer pool.
> > It's a register write, which works like,
> > "A 128-bit write (STP) to NPA_LF_AURA_OP_FREE0 and
> > NPA_LF_AURA_OP_FREE1 frees a pointer into a given pool. All other
> > accesses to these registers (e.g. reads and 64-bit writes) are RAZ/WI."
> >
> > Wrt otx2_atomic64_add(), registers for reading IRQ status, queue stats etc
> > works only with 64-bit atomic load-and-add instructions. The nornal
> > atomic64 kernel
> > interface for ARM64 which supports 'ldadd' instruction needs
> > CONFIG_ARM64_LSE_ATOMICS
> > to be enabled. LSE (Large system extensions) is a CPU feature which is supported
> > by silicons which implement ARMv8.1 and later version of instruction set.
> >
> > To support kernel with and without LSE_ATOMICS config enabled, here we are
> > passing "cpu   generic+lse" to the compiler. This is also done to avoid making
> > ARM64 and ARM64_LSE_ATOMICS hard dependency for driver compilation.
> >
> >>
> >> Finally why is the #else case doing an assignment to *ptr rather
> >> than an increment like "*ptr += incr;"?
> >
> > This device is a on-chip network controller which is a ARM64 based.
> > Previously when i submitted driver with ARM64 dependency i was advised
> > to allow this driver to be built for other architectures as well for
> > static analysis
> > reports etc.
> > https://www.spinics.net/lists/linux-soc/msg05847.html
> >
> > Hence added a dummy 'otx2_atomic64_add' just for compilation purposes.
> > Please ignore the definition.
>
> But it doesn't add, it assigns.  That's the point of my question.
>
> If you are going to provide a fallback, at least make it semantically
> correct.

Okay, will fix and resubmit.

Thanks,
Sunil.