netdev - Re: [PATCH 13/15] octeontx2-af: Add support for CGX link management

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: <CA+sq2Cc5p+Hdm573is2rAWMeGZKS+=iUiYHU3+soTR4CSpgcHg@mail.gmail.com>
Date:   Fri, 28 Sep 2018 15:01:41 +0530
From:   Sunil Kovvuri <sunil.kovvuri@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>, linux-soc@...r.kernel.org,
        Linu Cherian <lcherian@...vell.com>,
        Nithya Mani <nmani@...vell.com>
Subject: Re: [PATCH 13/15] octeontx2-af: Add support for CGX link management

On Fri, Sep 28, 2018 at 1:49 PM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Fri, Sep 28, 2018 at 8:09 AM <sunil.kovvuri@...il.com> wrote:
> >
> > +/* scratchx(0) CSR used for ATF->non-secure SW communication.
> > + * This acts as the status register
> > + * Provides details on command ack/status, link status, error details
> > + */
> > +struct cgx_evt_sts {
> > +       uint64_t ack:1;
> > +       uint64_t evt_type:1;            /* cgx_evt_type */
> > +       uint64_t stat:1;                /* cgx_stat */
> > +       uint64_t id:6;                  /* cgx_evt_id/cgx_cmd_id */
> > +       uint64_t reserved:55;
> > +};
> > +
> > +/* Response to command IDs with command status as CGX_STAT_FAIL
> > + *
> > + * Not applicable for commands :
> > + * CGX_CMD_LINK_BRING_UP/DOWN/CGX_EVT_LINK_CHANGE
> > + * check struct cgx_lnk_sts comments
> > + */
> > +struct cgx_err_sts_s {
> > +       uint64_t reserved1:9;
> > +       uint64_t type:10;               /* cgx_error_type */
> > +       uint64_t reserved2:35;
> > +};
> > +
> > +/* Response to cmd ID as CGX_CMD_GET_FW_VER with cmd status as
> > + * CGX_STAT_SUCCESS
> > + */
> > +struct cgx_ver_s {
> > +       uint64_t reserved1:9;
> > +       uint64_t major_ver:4;
> > +       uint64_t minor_ver:4;
> > +       uint64_t reserved2:47;
> > +};
>
> From what I can tell, you pass these structures to the device, so they
> are a binary interface. I don't think you can rely on bitfields to work
> correctly here, they are generally not portable and I wouldn't rely
> on them doing the right thing if you build a big-endian kernel.
>
> It's better to use bitmasks on a u64 value instead to make it obviously
> portable.
>
>        Arnd

This communication between firmware and kernel driver is done using couple of
scratch registers. With limited space available we had to resort to bitfields.
Your point about endianness is correct. As you might be aware that the device to
which this driver registers to, is only found on OcteonTx2 SOC which operates
in a standalone mode. As of now we are not targeting to make these drivers
work in big-endian mode.

We would prefer to make big-endian related changes later on, test them
fully and
submit patches,  would this be okay ?

If not we will define big endian bit fields in all command structures
and re-submit.

Sunil.