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