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

[Sriram Yagnaraman]

> -----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. 

> -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?

> +
> +    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"?

> 
> -    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