Re: [dpdk-dev] [PATCH] net/nfp: add CPP bridge as service

[Alejandro Lucero <alejandro.lucero@netronome.com>]

On Wed, Jan 9, 2019 at 4:15 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> On 1/9/2019 2:20 PM, Alejandro Lucero wrote:
> >
> >
> > On Wed, Jan 9, 2019 at 10:54 AM Ferruh Yigit <ferruh.yigit@intel.com
> > <mailto:ferruh.yigit@intel.com>> wrote:
> >
> >     On 1/3/2019 8:56 AM, Alejandro Lucero wrote:
> >     > The Netronome's Network Flow Processor chip is highly programmable
> >     > with the goal of processing packets at high speed. Processing units
> >     > and other chip components are available from the host through the
> >     > PCIe CPP(Command Push Pull bus) interface. The NFP PF PMD
> configures
> >     > a CPP handler for setting up and working with vNICs, perform
> actions
> >     > like link up or down, or accessing extended stats from the MAC
> component.
> >     >
> >     > There exist NFP host tools which access the NFP components for
> >     > programming and debugging but they require the CPP interface. When
> the
> >     > PMD is bound to the PF, the DPDK app owns the CPP interface, so
> these
> >     > host tools can not access the NFP through other means like NFP
> kernel
> >     > drivers.
> >     >
> >     > This patch adds a CPP bridge using the rte_service API which can be
> >     > enabled by a DPDK app. Interestingly, DPDK clients like OVS will
> not
> >     > enable specific service cores, but this can be performed with a
> >     > secondary process specifically enabling this CPP bridge service and
> >     > therefore giving access to the NFP to those host tools.
> >
> >     Hi Alejandro,
> >
> >
> > Hi Ferruh,
> >
> >
> >     Getting a few build errors, more details below.
> >
> >     >
> >     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
> >     <mailto:alejandro.lucero@netronome.com>>
> >     <...>
> >
> >     > +     /* Obtain target's CPP ID and offset in target */
> >     > +     cpp_id = (offset >> 40) << 8;
> >
> >     With icc, i686 getting [1], it seems 'off_t' is 32bits long on 32bit
> build.
> >
> >     [1]
> >     error #63: shift count is too large
> >
> >
> > We do not support 32 bits. I thought our PMD was not built in that case.
>
> If PMD doesn't support 32 bits, above is OK, I will update my script
> accordingly.
>
> >
> >
> >     <...>
> >
> >     > +                     if (err != (int)len) {
> >     > +                             printf("%s: error when receiving, %d
> of %lu\n",
> >     > +                                     __func__, err, count);
> >
> >     Giving build error for 32bits [3], and can you please use logging
> macros instead
> >     of printf?
> >
> >
> > Sure.
> >
> >
> >     [3]
> >     error: format ‘%lu’ expects argument of type ‘long unsigned int’,
> but argument 4
> >     has type ‘size_t’ {aka ‘unsigned int’} [-Werror=format=]
> >
> >     <...>
> >
> >     > +     /* Obtain target's CPP ID and offset in target */
> >     > +     cpp_id = (offset >> 40) << 8;
> >
> >     Same as above [1].
> >
> >     <...>
> >
> >     > +                     if (err != (int)len) {
> >     > +                             printf("%s: error when sending: %d
> of %lu\n",
> >     > +                                     __func__, err, count);
> >
> >     Same build error with above [3].
> >
> >     <...>
> >
> >     > +nfp_cpp_bridge_serve_ioctl(int sockfd, struct nfp_cpp *cpp)
> >     > +{
> >     > +     int cmd, err;
> >     > +     uint32_t ident_size, tmp;
> >     > +
> >     > +     /* Reading now the IOCTL command */
> >     > +     err = recv(sockfd, &cmd, 4, 0);
> >     > +     if (err != 4) {
> >     > +             printf("%s: read error from socket\n", __func__);
> >     > +             return -EIO;
> >     > +     }
> >     > +
> >     > +     /* Only supporting NFP_IOCTL_CPP_IDENTIFICATION */
> >     > +     if (cmd != NFP_IOCTL_CPP_IDENTIFICATION) {
> >
> >     Giving build error with ppc_64-power8-linuxapp-gcc [2].
> >
> >
> > We do not support power architecture.
>
> Yes but issue seems not exactly ppc issue, more like signed - unsigned
> comparison. Can you please check if is there any valid issue here?
>
>
This is a funny one. NFP_IOCTL_CPP_IDENTIFICATION is not zero, and cmd
could be anything.
And it does work with other compilers!

Talking with a compiler guy in the office, and it is hard to know why the
compiler is triggering an error here. I suspect this is some sort of
endianness mess, and he thinks the compiler could be assuming the cmd
variable after recv call is always negative or positive, and the macro
always being the opposite in powerpc, so the comparison is always true,
what is what the error message says.

Anyway, it is not clear how to fix this. Maybe defining cmd as uint32_t
could help. Any change we can test this before sending another patch
version?



> >
> >
> >     [2]
> >     error: comparison is always true due to limited range of data type
> >     [-Werror=type-limits]
> >
>
>