Re: [PATCH 30/40] igb: Implement igb-specific oversize check

[Akihiko Odaki]

On 2023/04/16 20:22, Sriram Yagnaraman wrote:
> > -----Original Message-----
> > From: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Sent: Friday, 14 April 2023 13:37
> > Cc: Sriram Yagnaraman <sriram.yagnaraman@est.tech>; Jason Wang
> > <jasowang@redhat.com>; Dmitry Fleytman <dmitry.fleytman@gmail.com>;
> > Michael S. Tsirkin <mst@redhat.com>; Alex Bennée <alex.bennee@linaro.org>;
> > Philippe Mathieu-Daudé <philmd@linaro.org>; Thomas Huth
> > <thuth@redhat.com>; Wainer dos Santos Moschetta
> > <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>; Cleber Rosa
> > <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>; Paolo Bonzini
> > <pbonzini@redhat.com>; qemu-devel@nongnu.org; Akihiko Odaki
> > <akihiko.odaki@daynix.com>
> > Subject: [PATCH 30/40] igb: Implement igb-specific oversize check
> >
> > igb has a configurable size limit for LPE, and uses different limits depending 
> > on
> > whether the packet is treated as a VLAN packet.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   hw/net/igb_core.c | 41 +++++++++++++++++++++++++++--------------
> >   1 file changed, 27 insertions(+), 14 deletions(-)
> >
> > diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> > 2013a9a53d..569897fb99 100644
> > --- a/hw/net/igb_core.c
> > +++ b/hw/net/igb_core.c
> > @@ -954,16 +954,21 @@ igb_rx_l4_cso_enabled(IGBCore *core)
> >       return !!(core->mac[RXCSUM] & E1000_RXCSUM_TUOFLD);  }
> >
> > -static bool
>
> The convention in seems to be to declare return value in first line and then 
> the function name in the next line.

There are already functions not following the convention, and it is more 
like exceptional in the entire QEMU code base. This patch prioritize the 
QEMU's common practice over e1000e's old convention.

> > -igb_rx_is_oversized(IGBCore *core, uint16_t qn, size_t size)
> > +static bool igb_rx_is_oversized(IGBCore *core, const struct eth_header *ehdr,
> > +                                size_t size, bool lpe, uint16_t rlpml)
> >   {
> > -    uint16_t pool = qn % IGB_NUM_VM_POOLS;
> > -    bool lpe = !!(core->mac[VMOLR0 + pool] & E1000_VMOLR_LPE);
> > -    int max_ethernet_lpe_size =
> > -        core->mac[VMOLR0 + pool] & E1000_VMOLR_RLPML_MASK;
> > -    int max_ethernet_vlan_size = 1522;
> > +    size += 4;
>
> Is the above 4 CRC bytes?

Yes. In v2, a new constant ETH_FCS_LEN is used to explictly state that.

> > +
> > +    if (lpe) {
> > +        return size > rlpml;
> > +    }
> > +
> > +    if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0xffff) &&
> > +        e1000x_vlan_rx_filter_enabled(core->mac)) {
> > +        return size > 1522;
> > +    }
>
> Should a check for 1526 bytes if extended VLAN is present be added?
> Maybe in "igb: Strip the second VLAN tag for extended VLAN"?

In v2, I placed "igb: Strip the second VLAN tag for extended VLAN" 
earlier than this patch, and this patch is rewritten so it can handle 
the second VLAN tag too.

> > -    return size > (lpe ? max_ethernet_lpe_size : max_ethernet_vlan_size);
> > +    return size > 1518;
> >   }
> >
> >   static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
> > @@ -976,6 +981,8 @@ static uint16_t igb_receive_assign(IGBCore *core,
> > const L2Header *l2_header,
> >       uint16_t queues = 0;
> >       uint16_t oversized = 0;
> >       uint16_t vid = be16_to_cpu(l2_header->vlan[0].h_tci) & VLAN_VID_MASK;
> > +    bool lpe;
> > +    uint16_t rlpml;
> >       int i;
> >
> >       memset(rss_info, 0, sizeof(E1000E_RSSInfo)); @@ -984,6 +991,14 @@
> > static uint16_t igb_receive_assign(IGBCore *core, const L2Header *l2_header,
> >           *external_tx = true;
> >       }
> >
> > +    lpe = !!(core->mac[RCTL] & E1000_RCTL_LPE);
> > +    rlpml = core->mac[RLPML];
> > +    if (!(core->mac[RCTL] & E1000_RCTL_SBP) &&
> > +        igb_rx_is_oversized(core, ehdr, size, lpe, rlpml)) {
> > +        trace_e1000x_rx_oversized(size);
> > +        return queues;
> > +    }
> > +
> >       if (e1000x_is_vlan_packet(ehdr, core->mac[VET] & 0xffff) &&
> >           !e1000x_rx_vlan_filter(core->mac, PKT_GET_VLAN_HDR(ehdr))) {
> >           return queues;
> > @@ -1067,7 +1082,10 @@ static uint16_t igb_receive_assign(IGBCore *core,
> > const L2Header *l2_header,
> >           queues &= core->mac[VFRE];
> >           if (queues) {
> >               for (i = 0; i < IGB_NUM_VM_POOLS; i++) {
> > -                if ((queues & BIT(i)) && igb_rx_is_oversized(core, i, size)) {
> > +                lpe = !!(core->mac[VMOLR0 + i] & E1000_VMOLR_LPE);
> > +                rlpml = core->mac[VMOLR0 + i] & E1000_VMOLR_RLPML_MASK;
> > +                if ((queues & BIT(i)) &&
> > +                    igb_rx_is_oversized(core, ehdr, size, lpe, rlpml))
> > + {
> >                       oversized |= BIT(i);
> >                   }
> >               }
> > @@ -1609,11 +1627,6 @@ igb_receive_internal(IGBCore *core, const struct
> > iovec *iov, int iovcnt,
> >           iov_to_buf(iov, iovcnt, iov_ofs, &min_buf, sizeof(min_buf.l2_header));
> >       }
> >
> > -    /* Discard oversized packets if !LPE and !SBP. */
> > -    if (e1000x_is_oversized(core->mac, size)) {
> > -        return orig_size;
> > -    }
> > -
> >       net_rx_pkt_set_packet_type(core->rx_pkt,
> >                                  get_eth_packet_type(&min_buf.l2_header.eth));
> >       net_rx_pkt_set_protocols(core->rx_pkt, iov, iovcnt, iov_ofs);
> > --
> > 2.40.0