> -----Original Message----- > From: Akihiko Odaki <akihiko.od...@daynix.com> > Sent: Sunday, 23 April 2023 06:18 > Cc: Sriram Yagnaraman <sriram.yagnara...@est.tech>; Jason Wang > <jasow...@redhat.com>; Dmitry Fleytman <dmitry.fleyt...@gmail.com>; > Michael S . Tsirkin <m...@redhat.com>; Alex Bennée > <alex.ben...@linaro.org>; Philippe Mathieu-Daudé <phi...@linaro.org>; > Thomas Huth <th...@redhat.com>; Wainer dos Santos Moschetta > <waine...@redhat.com>; Beraldo Leal <bl...@redhat.com>; Cleber Rosa > <cr...@redhat.com>; Laurent Vivier <lviv...@redhat.com>; Paolo Bonzini > <pbonz...@redhat.com>; qemu-devel@nongnu.org; Tomasz Dzieciol > <t.dziec...@partner.samsung.com>; Akihiko Odaki > <akihiko.od...@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.od...@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.yagnara...@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