Re: [PATCH v5 21/29] hw/net/net_tx_pkt: Automatically determine if virtio-net header is used

[Akihiko Odaki]

On 2023/02/21 12:38, Jason Wang wrote:
> 在 2023/2/1 11:35, Akihiko Odaki 写道:
> > The new function qemu_get_using_vnet_hdr() allows to automatically
> > determine if virtio-net header is used.
> >
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   hw/net/e1000e_core.c |  3 +--
> >   hw/net/net_tx_pkt.c  | 19 ++++++++++---------
> >   hw/net/net_tx_pkt.h  |  3 +--
> >   hw/net/vmxnet3.c     |  6 ++----
> >   4 files changed, 14 insertions(+), 17 deletions(-)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index 38d374fba3..954a007151 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -3376,8 +3376,7 @@ e1000e_core_pci_realize(E1000ECore     *core,
> >           qemu_add_vm_change_state_handler(e1000e_vm_state_change, core);
> >       for (i = 0; i < E1000E_NUM_QUEUES; i++) {
> > -        net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner,
> > -                        E1000E_MAX_TX_FRAGS, core->has_vnet);
> > +        net_tx_pkt_init(&core->tx[i].tx_pkt, core->owner, 
> > E1000E_MAX_TX_FRAGS);
> >       }
> >       net_rx_pkt_init(&core->rx_pkt, core->has_vnet);
> > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> > index 8a23899a4d..cf46c8457f 100644
> > --- a/hw/net/net_tx_pkt.c
> > +++ b/hw/net/net_tx_pkt.c
> > @@ -35,7 +35,6 @@ struct NetTxPkt {
> >       PCIDevice *pci_dev;
> >       struct virtio_net_hdr virt_hdr;
> > -    bool has_virt_hdr;
>
>
> So this requires implicit coupling of NetTxPkt and a NetClientState (not 
> self contained). This may work now but probably not the future e.g when 
> two packets were queued in a list when one packet has a vnet header but 
> another doesn't?
>
> Thanks

This patch is actually intended to remove coupling of NetTxPkt and 
NetClientState. e1000e and igb have loopback mode, and in this mode, 
NetTxPkt needs to perform segmentation by itself even if the peer 
accepts vnet header. However, before this patch, has_virt_hdr flag is 
fixed in net_tx_pkt_init() so it couldn't handle a case where one packet 
needs vnet header and another doesn't.

This patch fixes such a case by deferring the decision whether to have 
vnet header (and to offload segmentation) to the point when actually 
sending the packet. This allows NetTxPkt to add a vnet header and not to 
do so, depending on the situation.

Patch "e1000e: Perform software segmentation for loopback" further 
decouples NetTxPkt and NetClientState by introducing a new function, 
net_tx_pkt_send_custom(). Unlike net_tx_pkt_send(), 
net_tx_pkt_send_custom() do not need NetClientState, and it is totally 
up to the caller whether to have vnet header or to offload segmentation.

Regards,
Akihiko Odaki

> >       struct iovec *raw;
> >       uint32_t raw_frags;
> > @@ -59,7 +58,7 @@ struct NetTxPkt {
> >   };
> >   void net_tx_pkt_init(struct NetTxPkt **pkt, PCIDevice *pci_dev,
> > -    uint32_t max_frags, bool has_virt_hdr)
> > +    uint32_t max_frags)
> >   {
> >       struct NetTxPkt *p = g_malloc0(sizeof *p);
> > @@ -71,10 +70,8 @@ void net_tx_pkt_init(struct NetTxPkt **pkt, 
> > PCIDevice *pci_dev,
> >       p->max_payload_frags = max_frags;
> >       p->max_raw_frags = max_frags;
> > -    p->has_virt_hdr = has_virt_hdr;
> >       p->vec[NET_TX_PKT_VHDR_FRAG].iov_base = &p->virt_hdr;
> > -    p->vec[NET_TX_PKT_VHDR_FRAG].iov_len =
> > -        p->has_virt_hdr ? sizeof p->virt_hdr : 0;
> > +    p->vec[NET_TX_PKT_VHDR_FRAG].iov_len = sizeof p->virt_hdr;
> >       p->vec[NET_TX_PKT_L2HDR_FRAG].iov_base = &p->l2_hdr;
> >       p->vec[NET_TX_PKT_L3HDR_FRAG].iov_base = &p->l3_hdr;
> > @@ -617,9 +614,11 @@ static bool net_tx_pkt_do_sw_fragmentation(struct 
> > NetTxPkt *pkt,
> >   bool net_tx_pkt_send(struct NetTxPkt *pkt, NetClientState *nc)
> >   {
> > +    bool using_vnet_hdr = qemu_get_using_vnet_hdr(nc->peer);
> > +
> >       assert(pkt);
> > -    if (!pkt->has_virt_hdr &&
> > +    if (!using_vnet_hdr &&
> >           pkt->virt_hdr.flags & VIRTIO_NET_HDR_F_NEEDS_CSUM) {
> >           net_tx_pkt_do_sw_csum(pkt);
> >       }
> > @@ -636,11 +635,13 @@ bool net_tx_pkt_send(struct NetTxPkt *pkt, 
> > NetClientState *nc)
> >           }
> >       }
> > -    if (pkt->has_virt_hdr ||
> > +    if (using_vnet_hdr ||
> >           pkt->virt_hdr.gso_type == VIRTIO_NET_HDR_GSO_NONE) {
> > +        int index = using_vnet_hdr ?
> > +                    NET_TX_PKT_VHDR_FRAG : NET_TX_PKT_L2HDR_FRAG;
> >           net_tx_pkt_fix_ip6_payload_len(pkt);
> > -        net_tx_pkt_sendv(pkt, nc, pkt->vec,
> > -            pkt->payload_frags + NET_TX_PKT_PL_START_FRAG);
> > +        net_tx_pkt_sendv(pkt, nc, pkt->vec + index,
> > +            pkt->payload_frags + NET_TX_PKT_PL_START_FRAG - index);
> >           return true;
> >       }
> > diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> > index 2e38a5fa69..8d3faa42fb 100644
> > --- a/hw/net/net_tx_pkt.h
> > +++ b/hw/net/net_tx_pkt.h
> > @@ -32,10 +32,9 @@ struct NetTxPkt;
> >    * @pkt:            packet pointer
> >    * @pci_dev:        PCI device processing this packet
> >    * @max_frags:      max tx ip fragments
> > - * @has_virt_hdr:   device uses virtio header.
> >    */
> >   void net_tx_pkt_init(struct NetTxPkt **pkt, PCIDevice *pci_dev,
> > -    uint32_t max_frags, bool has_virt_hdr);
> > +    uint32_t max_frags);
> >   /**
> >    * Clean all tx packet resources.
> > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > index c63bbb59bd..8c3f5d6e14 100644
> > --- a/hw/net/vmxnet3.c
> > +++ b/hw/net/vmxnet3.c
> > @@ -1521,8 +1521,7 @@ static void vmxnet3_activate_device(VMXNET3State 
> > *s)
> >       /* Preallocate TX packet wrapper */
> >       VMW_CFPRN("Max TX fragments is %u", s->max_tx_frags);
> > -    net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
> > -                    s->max_tx_frags, s->peer_has_vhdr);
> > +    net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s), s->max_tx_frags);
> >       net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
> >       /* Read rings memory locations for RX queues */
> > @@ -2402,8 +2401,7 @@ static int vmxnet3_post_load(void *opaque, int 
> > version_id)
> >   {
> >       VMXNET3State *s = opaque;
> > -    net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s),
> > -                    s->max_tx_frags, s->peer_has_vhdr);
> > +    net_tx_pkt_init(&s->tx_pkt, PCI_DEVICE(s), s->max_tx_frags);
> >       net_rx_pkt_init(&s->rx_pkt, s->peer_has_vhdr);
> >       if (s->msix_used) {