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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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'.

>   /**
>    * 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.

> +    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)