Re: [PATCH v2 01/41] hw/net/net_tx_pkt: Decouple implementation from PCI

[Akihiko Odaki]

On 2023/04/20 18:46, Philippe Mathieu-Daudé wrote:
> Hi Akihiko,
>
> On 20/4/23 07:46, Akihiko Odaki wrote:
> > This is intended to be followed by another change for the interface.
> > It also fixes the leak of memory mapping when the specified memory is
> > partially mapped.
> >
> > Fixes: e263cd49c7 ("Packet abstraction for VMWARE network devices")
> > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> > ---
> >   hw/net/net_tx_pkt.h |  9 ++++++++
> >   hw/net/net_tx_pkt.c | 53 ++++++++++++++++++++++++++++-----------------
> >   2 files changed, 42 insertions(+), 20 deletions(-)
> >
> > diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> > index e5ce6f20bc..5eb123ef90 100644
> > --- a/hw/net/net_tx_pkt.h
> > +++ b/hw/net/net_tx_pkt.h
> > @@ -153,6 +153,15 @@ void net_tx_pkt_dump(struct NetTxPkt *pkt);
> >    */
> >   void net_tx_pkt_reset(struct NetTxPkt *pkt, PCIDevice *dev);
> > +/**
> > + * Unmap a fragment mapped from a PCI device.
> > + *
> > + * @context:        PCI device owning fragment
>
> Per your description ...
>
> > + * @base:           pointer to fragment
> > + * @len:            length of fragment
> > + */
> > +void net_tx_pkt_unmap_frag_pci(void *context, void *base, size_t len);
>
> ... we can directly use the stricter 'PCIDevice *dev'.

This function is intended to match the following type added later:
typedef void (*NetTxPktFreeFrag)(DeviceState *, void *, size_t);

> >   /**
> >    * Send packet to qemu. handles sw offloads if vhdr is not supported.
> >    *
> > diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> > index 8dc8568ba2..aca12ff035 100644
> > --- a/hw/net/net_tx_pkt.c
> > +++ b/hw/net/net_tx_pkt.c
> > @@ -384,10 +384,9 @@ void net_tx_pkt_setup_vlan_header_ex(struct 
> > NetTxPkt *pkt,
> >       }
> >   }
> > -bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
> > -    size_t len)
> > +static bool net_tx_pkt_add_raw_fragment_common(struct NetTxPkt *pkt,
> > +                                               void *base, size_t len)
> >   {
> > -    hwaddr mapped_len = 0;
> >       struct iovec *ventry;
> >       assert(pkt);
> > @@ -395,23 +394,12 @@ bool net_tx_pkt_add_raw_fragment(struct NetTxPkt 
> > *pkt, hwaddr pa,
> >           return false;
> >       }
> > -    if (!len) {
> > -        return true;
> > -     }
> > -
> >       ventry = &pkt->raw[pkt->raw_frags];
> > -    mapped_len = len;
> > +    ventry->iov_base = base;
> > +    ventry->iov_len = len;
> > +    pkt->raw_frags++;
> > -    ventry->iov_base = pci_dma_map(pkt->pci_dev, pa,
> > -                                   &mapped_len, 
> > DMA_DIRECTION_TO_DEVICE);
> > -
> > -    if ((ventry->iov_base != NULL) && (len == mapped_len)) {
> > -        ventry->iov_len = mapped_len;
> > -        pkt->raw_frags++;
> > -        return true;
> > -    } else {
> > -        return false;
> > -    }
> > +    return true;
> >   }
> >   bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt)
> > @@ -465,8 +453,9 @@ void net_tx_pkt_reset(struct NetTxPkt *pkt, 
> > PCIDevice *pci_dev)
> >           assert(pkt->raw);
> >           for (i = 0; i < pkt->raw_frags; i++) {
> >               assert(pkt->raw[i].iov_base);
> > -            pci_dma_unmap(pkt->pci_dev, pkt->raw[i].iov_base,
> > -                          pkt->raw[i].iov_len, 
> > DMA_DIRECTION_TO_DEVICE, 0);
> > +            net_tx_pkt_unmap_frag_pci(pkt->pci_dev,
> > +                                      pkt->raw[i].iov_base,
> > +                                      pkt->raw[i].iov_len);
> >           }
> >       }
> >       pkt->pci_dev = pci_dev;
> > @@ -476,6 +465,30 @@ void net_tx_pkt_reset(struct NetTxPkt *pkt, 
> > PCIDevice *pci_dev)
> >       pkt->l4proto = 0;
> >   }
> > +void net_tx_pkt_unmap_frag_pci(void *context, void *base, size_t len)
>
> So net_tx_pkt_unmap_frag_pci(PCIDevice *dev, ...)
>
> > +{
> > +    pci_dma_unmap(context, base, len, DMA_DIRECTION_TO_DEVICE, 0);
> > +}
> > +
> > +bool net_tx_pkt_add_raw_fragment(struct NetTxPkt *pkt, hwaddr pa,
>
> It seems other hw/net/ models use (dma_addr_t addr, dma_addr_t len).
> Similarly does the pci_dma_FOO() API.

This prototype is what net_tx_pkt_add_raw_fragment() currently has, and 
this patch only moves it here. It will be updated in the following 
patch, which replaces hwaddr with dma_addr_t.

> > +    size_t len)
> > +{
> > +    dma_addr_t mapped_len = len;
>
> See, here you use dma_addr_t.
>
> > +    void *base = pci_dma_map(pkt->pci_dev, pa, &mapped_len,
> > +                             DMA_DIRECTION_TO_DEVICE);
> > +    if (!base) {
> > +        return false;
> > +    }
> > +
> > +    if (mapped_len != len ||
> > +        !net_tx_pkt_add_raw_fragment_common(pkt, base, len)) {
> > +        net_tx_pkt_unmap_frag_pci(pkt->pci_dev, base, mapped_len);
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> > +
> >   static void net_tx_pkt_do_sw_csum(struct NetTxPkt *pkt,
> >                                     struct iovec *iov, uint32_t iov_len,
> >                                     uint16_t csl)