Re: [PATCH v2 27/41] net/eth: Always add VLAN tag

[Akihiko Odaki]

On 2023/04/21 1:22, Sriram Yagnaraman wrote:
> > -----Original Message-----
> > From: Akihiko Odaki <akihiko.odaki@daynix.com>
> > Sent: Thursday, 20 April 2023 07:47
> > 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 v2 27/41] net/eth: Always add VLAN tag
> >
> > It is possible to have another VLAN tag even if the packet is already tagged.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   include/net/eth.h   |  4 ++--
> >   hw/net/net_tx_pkt.c | 16 +++++++---------
> >   net/eth.c           | 22 ++++++----------------
> >   3 files changed, 15 insertions(+), 27 deletions(-)
> >
> > diff --git a/include/net/eth.h b/include/net/eth.h index
> > 95ff24d6b8..048e434685 100644
> > --- a/include/net/eth.h
> > +++ b/include/net/eth.h
> > @@ -353,8 +353,8 @@ eth_strip_vlan_ex(const struct iovec *iov, int iovcnt,
> > size_t iovoff,  uint16_t  eth_get_l3_proto(const struct iovec *l2hdr_iov, int
> > iovcnt, size_t l2hdr_len);
> >
> > -void eth_setup_vlan_headers(struct eth_header *ehdr, uint16_t vlan_tag,
> > -    uint16_t vlan_ethtype, bool *is_new);
> > +void eth_setup_vlan_headers(struct eth_header *ehdr, size_t *ehdr_size,
> > +                            uint16_t vlan_tag, uint16_t vlan_ethtype);
> >
> >
> >   uint8_t eth_get_gso_type(uint16_t l3_proto, uint8_t *l3_hdr, uint8_t l4proto);
> > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index
> > ce6b102391..af8f77a3f0 100644
> > --- a/hw/net/net_tx_pkt.c
> > +++ b/hw/net/net_tx_pkt.c
> > @@ -40,7 +40,10 @@ struct NetTxPkt {
> >
> >       struct iovec *vec;
> >
> > -    uint8_t l2_hdr[ETH_MAX_L2_HDR_LEN];
> > +    struct {
> > +        struct eth_header eth;
> > +        struct vlan_header vlan[3];
> > +    } l2_hdr;
> >       union {
> >           struct ip_header ip;
> >           struct ip6_header ip6;
> > @@ -365,18 +368,13 @@ bool net_tx_pkt_build_vheader(struct NetTxPkt
> > *pkt, bool tso_enable,  void net_tx_pkt_setup_vlan_header_ex(struct NetTxPkt
> > *pkt,
> >       uint16_t vlan, uint16_t vlan_ethtype)  {
> > -    bool is_new;
> >       assert(pkt);
> >
> >       eth_setup_vlan_headers(pkt->vec[NET_TX_PKT_L2HDR_FRAG].iov_base,
> > -        vlan, vlan_ethtype, &is_new);
> > +                           &pkt->vec[NET_TX_PKT_L2HDR_FRAG].iov_len,
> > +                           vlan, vlan_ethtype);
> >
> > -    /* update l2hdrlen */
> > -    if (is_new) {
> > -        pkt->hdr_len += sizeof(struct vlan_header);
> > -        pkt->vec[NET_TX_PKT_L2HDR_FRAG].iov_len +=
> > -            sizeof(struct vlan_header);
> > -    }
> > +    pkt->hdr_len += sizeof(struct vlan_header);
> >   }
> >
> >   bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, void *base, size_t
> > len) diff --git a/net/eth.c b/net/eth.c index f7ffbda600..5307978486 100644
> > --- a/net/eth.c
> > +++ b/net/eth.c
> > @@ -21,26 +21,16 @@
> >   #include "net/checksum.h"
> >   #include "net/tap.h"
> >
> > -void eth_setup_vlan_headers(struct eth_header *ehdr, uint16_t vlan_tag,
> > -    uint16_t vlan_ethtype, bool *is_new)
> > +void eth_setup_vlan_headers(struct eth_header *ehdr, size_t *ehdr_size,
> > +                            uint16_t vlan_tag, uint16_t vlan_ethtype)
> >   {
> >       struct vlan_header *vhdr = PKT_GET_VLAN_HDR(ehdr);
> >
> > -    switch (be16_to_cpu(ehdr->h_proto)) {
> > -    case ETH_P_VLAN:
> > -    case ETH_P_DVLAN:
> > -        /* vlan hdr exists */
> > -        *is_new = false;
> > -        break;
> > -
> > -    default:
> > -        /* No VLAN header, put a new one */
> > -        vhdr->h_proto = ehdr->h_proto;
> > -        ehdr->h_proto = cpu_to_be16(vlan_ethtype);
> > -        *is_new = true;
> > -        break;
> > -    }
> > +    memmove(vhdr + 1, vhdr, *ehdr_size - ETH_HLEN);
>
> Do we need a check that we are not overflowing the vlan array size?

It is the caller's responsibility to accommodate the space necessary for 
the new VLAN tag.

> >       vhdr->h_tci = cpu_to_be16(vlan_tag);
> > +    vhdr->h_proto = ehdr->h_proto;
> > +    ehdr->h_proto = cpu_to_be16(vlan_ethtype);
> > +    *ehdr_size += sizeof(*vhdr);
> >   }
> >
> >   uint8_t
> > --
> > 2.40.0