RE: [PATCH v3 09/47] igb: Always copy ethernet header

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Sunday, 23 April 2023 06:18
> 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; Tomasz Dzieciol
> <t.dzieciol@partner.samsung.com>; Akihiko Odaki
> <akihiko.odaki@daynix.com>
> Subject: [PATCH v3 09/47] igb: Always copy ethernet header
> 
> igb_receive_internal() used to check the iov length to determine copy the iovs
> to a contiguous buffer, but the check is flawed in two
> ways:
> - It does not ensure that iovcnt > 0.
> - It does not take virtio-net header into consideration.
> 
> The size of this copy is just 22 octets, which can be even less than the code 
> size
> required for checks. This (wrong) optimization is probably not worth so just
> remove it. Removing this also allows igb to assume aligned accesses for the
> ethernet header.
> 
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/igb_core.c | 39 +++++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 18 deletions(-)
> 
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> 21a8d9ada4..1d7f913e5a 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -67,6 +67,11 @@ typedef struct IGBTxPktVmdqCallbackContext {
>      NetClientState *nc;
>  } IGBTxPktVmdqCallbackContext;
> 
> +typedef struct L2Header {
> +    struct eth_header eth;
> +    struct vlan_header vlan;
> +} L2Header;
> +
>  static ssize_t
>  igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>                       bool has_vnet, bool *external_tx); @@ -961,15 +966,16 @@
> igb_rx_is_oversized(IGBCore *core, uint16_t qn, size_t size)
>      return size > (lpe ? max_ethernet_lpe_size : max_ethernet_vlan_size);  }
> 
> -static uint16_t igb_receive_assign(IGBCore *core, const struct eth_header
> *ehdr,
> +static uint16_t igb_receive_assign(IGBCore *core, const L2Header
> +*l2_header,
>                                     size_t size, E1000E_RSSInfo *rss_info,
>                                     bool *external_tx)  {
>      static const int ta_shift[] = { 4, 3, 2, 0 };
> +    const struct eth_header *ehdr = &l2_header->eth;
>      uint32_t f, ra[2], *macp, rctl = core->mac[RCTL];
>      uint16_t queues = 0;
>      uint16_t oversized = 0;
> -    uint16_t vid = lduw_be_p(&PKT_GET_VLAN_HDR(ehdr)->h_tci) &
> VLAN_VID_MASK;
> +    uint16_t vid = be16_to_cpu(l2_header->vlan.h_tci) & VLAN_VID_MASK;
>      bool accepted = false;
>      int i;
> 
> @@ -1590,14 +1596,13 @@ static ssize_t
>  igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>                       bool has_vnet, bool *external_tx)  {
> -    static const int maximum_ethernet_hdr_len = (ETH_HLEN + 4);
> -
>      uint16_t queues = 0;
>      uint32_t n = 0;
> -    uint8_t min_buf[ETH_ZLEN];
> +    union {
> +        L2Header l2_header;
> +        uint8_t octets[ETH_ZLEN];
> +    } min_buf;

I would call this^ buf/filter_buf instead of min_buf. But it is upto you to 
decide.
In any case, 
Reviewed-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>

>      struct iovec min_iov;
> -    struct eth_header *ehdr;
> -    uint8_t *filter_buf;
>      size_t size, orig_size;
>      size_t iov_ofs = 0;
>      E1000E_RxRing rxr;
> @@ -1623,24 +1628,21 @@ igb_receive_internal(IGBCore *core, const struct
> iovec *iov, int iovcnt,
>          net_rx_pkt_unset_vhdr(core->rx_pkt);
>      }
> 
> -    filter_buf = iov->iov_base + iov_ofs;
>      orig_size = iov_size(iov, iovcnt);
>      size = orig_size - iov_ofs;
> 
>      /* Pad to minimum Ethernet frame length */
>      if (size < sizeof(min_buf)) {
> -        iov_to_buf(iov, iovcnt, iov_ofs, min_buf, size);
> -        memset(&min_buf[size], 0, sizeof(min_buf) - size);
> +        iov_to_buf(iov, iovcnt, iov_ofs, &min_buf, size);
> +        memset(&min_buf.octets[size], 0, sizeof(min_buf) - size);
>          e1000x_inc_reg_if_not_full(core->mac, RUC);
> -        min_iov.iov_base = filter_buf = min_buf;
> +        min_iov.iov_base = &min_buf;
>          min_iov.iov_len = size = sizeof(min_buf);
>          iovcnt = 1;
>          iov = &min_iov;
>          iov_ofs = 0;
> -    } else if (iov->iov_len < maximum_ethernet_hdr_len) {
> -        /* This is very unlikely, but may happen. */
> -        iov_to_buf(iov, iovcnt, iov_ofs, min_buf, maximum_ethernet_hdr_len);
> -        filter_buf = min_buf;
> +    } else {
> +        iov_to_buf(iov, iovcnt, iov_ofs, &min_buf,
> + sizeof(min_buf.l2_header));
>      }
> 
>      /* Discard oversized packets if !LPE and !SBP. */ @@ -1648,11 +1650,12
> @@ igb_receive_internal(IGBCore *core, const struct iovec *iov, int iovcnt,
>          return orig_size;
>      }
> 
> -    ehdr = PKT_GET_ETH_HDR(filter_buf);
> -    net_rx_pkt_set_packet_type(core->rx_pkt, get_eth_packet_type(ehdr));
> +    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);
> 
> -    queues = igb_receive_assign(core, ehdr, size, &rss_info, external_tx);
> +    queues = igb_receive_assign(core, &min_buf.l2_header, size,
> +                                &rss_info, external_tx);
>      if (!queues) {
>          trace_e1000e_rx_flt_dropped();
>          return orig_size;
> --
> 2.40.0