[PATCH 0/4] hw/net/npcm_gmac: Various fixes to gmac_try_send_next_packet()

[PATCH 0/4] hw/net/npcm_gmac: Various fixes to gmac_try_send_next_packet()

[Peter Maydell]

There is a Coverity issue in gmac_try_send_next_packet() relating
to it setting a variable to a value that is always overwritten.
While I was looking at that, I noticed a number of oddities in
this function. This patchseries collects those up. Note that I
have only tested this with 'make check' and 'make check-functional',
and since I couldn't find a datasheet I have some questions about
the hardware behaviour. Review and testing would be appreciated.

The first patch is about something I noticed last year, but never
got a reply on:
https://lore.kernel.org/qemu-devel/CAFEAcA8Jx=CrXCSzOtjtEky5ikvtatk8N2gz=nKccpwEwO13+w@mail.gmail.com/
The code appears to mishandle the case where the function ends up
sending more than one packet, such that the second packet gets
sent with the contents of the first one.

When that bug has been fixed, the length and prev_buf_size
variables become exact copies of each other, so patch 2 gets rid
of the unnecessary 'length' variable. Here I have a question about
the hardware behaviour: what does it do if the guest attempts to
assemble a very large packet ? In this patch I retain the current
behaviour ("wrap the length at 64K and send a short packet"), but
this seems to me unlikely to be correct. Should we be detecting
the huge packet and flagging an error somewhere?

The third patch fixes a classic C thinko where we do sizeof(some pointer)
when we meant to check against the size of the dynamically allocated
buffer.

The fourth patch fixes the Coverity error by dropping the
not-very-useful 'buf' local variable entirely.

thanks
-- PMM

Peter Maydell (4):
  hw/net/npcm_gmac.c: Send the right data for second packet in a row
  hw/net/npcm_gmac.c: Unify length and prev_buf_size variables
  hw/net/npcm_gmac.c: Correct test for when to reallocate packet buffer
  hw/net/npcm_gmac.c: Drop 'buf' local variable

 hw/net/npcm_gmac.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH 1/4] hw/net/npcm_gmac.c: Send the right data for second packet in a row

[Peter Maydell]

The transmit loop in gmac_try_send_next_packet() is constructed in a
way that means it will send incorrect data if it it sends more than
one packet.

The function assembles the outbound data in a dynamically allocated
block of memory which is pointed to by tx_send_buffer.  We track the
first point in this block of memory which is not yet used with the
prev_buf_size offset, initially zero.  We track the size of the
packet we're sending with the length variable, also initially zero.

As we read chunks of data out of guest memory, we write them to
tx_send_buffer[prev_buf_size], and then increment both prev_buf_size
and length.  (We might dynamically reallocate the buffer if needed.)

When we send a packet, we checksum and send length bytes, starting at
tx_send_buffer, and then we reset length to 0.  This gives the right
data for the first packet.  But we don't reset prev_buf_size.  This
means that if we process more descriptors with further data for the
next packet, that data will continue to accumulate at offset
prev_buf_size, i.e.  after the data for the first packet.  But when
we transmit that second packet, we send length bytes from
tx_send_buffer, so we will send a packet which has the length of the
second packet but the data of the first one.

The fix for this is to also clear prev_buf_size after the packet has
been sent -- we never need the data from packet one after we've sent
it, so we can write packet two's data starting at the beginning of
the buffer.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Disclaimer: this bugfix is based on reading the source code; I
don't have a test setup to confirm or test it.
---
 hw/net/npcm_gmac.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
index a4341125809..921327dd8ca 100644
--- a/hw/net/npcm_gmac.c
+++ b/hw/net/npcm_gmac.c
@@ -615,6 +615,7 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
             trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length);
             buf = tx_send_buffer;
             length = 0;
+            prev_buf_size = 0;
         }
 
         /* step 6 */
-- 
2.43.0

[PATCH 2/4] hw/net/npcm_gmac.c: Unify length and prev_buf_size variables

[Peter Maydell]

After the bug fix in the previous commit, the length and prev_buf_size
variables are identical, except that prev_buf_size is uint32_t and
length is uint16_t. We can therefore unify them. The only place where
the type makes a difference is that we will truncate the packet
at 64K when sending it; this commit preserves that behaviour
by using a local variable when doing the packet send.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Should the behaviour really be to truncate the packet at 64K
rather than flagging an error if the guest assembles descriptors
that combine to give a too-large packet?
---
 hw/net/npcm_gmac.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
index 921327dd8ca..a0050a7725f 100644
--- a/hw/net/npcm_gmac.c
+++ b/hw/net/npcm_gmac.c
@@ -516,7 +516,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
     uint32_t desc_addr;
     struct NPCMGMACTxDesc tx_desc;
     uint32_t tx_buf_addr, tx_buf_len;
-    uint16_t length = 0;
     uint8_t *buf = tx_send_buffer;
     uint32_t prev_buf_size = 0;
     int csum = 0;
@@ -583,7 +582,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
                         __func__, tx_buf_addr);
             return;
         }
-        length += tx_buf_len;
         prev_buf_size += tx_buf_len;
 
         /* If not chained we'll have a second buffer. */
@@ -606,15 +604,18 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
                               __func__, tx_buf_addr);
                 return;
             }
-            length += tx_buf_len;
             prev_buf_size += tx_buf_len;
         }
         if (tx_desc.tdes1 & TX_DESC_TDES1_LAST_SEG_MASK) {
+            /*
+             * This will truncate the packet at 64K.
+             * TODO: find out if this is the correct behaviour.
+             */
+            uint16_t length = prev_buf_size;
             net_checksum_calculate(tx_send_buffer, length, csum);
             qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length);
             trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length);
             buf = tx_send_buffer;
-            length = 0;
             prev_buf_size = 0;
         }
 
-- 
2.43.0

[PATCH 3/4] hw/net/npcm_gmac.c: Correct test for when to reallocate packet buffer

[Peter Maydell]

In gmac_try_send_next_packet() we have code that does "if this block
of data won't fit in the buffer, reallocate it".  However, the
condition it uses is
  if ((prev_buf_size + tx_buf_len) > sizeof(buf))
where buf is a uint8_t *.

This means that sizeof(buf) is always 8 bytes, and the condition will
almost always be true, so we will reallocate the buffer more often
than we need to.

Correct the condition to test against tx_buffer_size, which is
where we track how big the allocated buffer is.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/npcm_gmac.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
index a0050a7725f..0c17ae9b2a9 100644
--- a/hw/net/npcm_gmac.c
+++ b/hw/net/npcm_gmac.c
@@ -569,7 +569,7 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
         tx_buf_len = TX_DESC_TDES1_BFFR1_SZ_MASK(tx_desc.tdes1);
         buf = &tx_send_buffer[prev_buf_size];
 
-        if ((prev_buf_size + tx_buf_len) > sizeof(buf)) {
+        if ((prev_buf_size + tx_buf_len) > tx_buffer_size) {
             tx_buffer_size = prev_buf_size + tx_buf_len;
             tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size);
             buf = &tx_send_buffer[prev_buf_size];
@@ -591,7 +591,7 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
             tx_buf_len = TX_DESC_TDES1_BFFR2_SZ_MASK(tx_desc.tdes1);
             buf = &tx_send_buffer[prev_buf_size];
 
-            if ((prev_buf_size + tx_buf_len) > sizeof(buf)) {
+            if ((prev_buf_size + tx_buf_len) > tx_buffer_size) {
                 tx_buffer_size = prev_buf_size + tx_buf_len;
                 tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size);
                 buf = &tx_send_buffer[prev_buf_size];
-- 
2.43.0

[PATCH 4/4] hw/net/npcm_gmac.c: Drop 'buf' local variable

[Peter Maydell]

We use the local variable 'buf' only when we call dma_memory_read(),
and it is always set to &tx_send_buffer[prev_buf_size] immediately
before both of those calls.  So remove the variable and pass
tx_send_buffer + prev_buf_size to dma_memory_read().

This fixes in passing a place where we set buf = tx_send_buffer
but never used that value because we always updated buf to
something else later before using it.

Coverity: CID 1534027
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/npcm_gmac.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/hw/net/npcm_gmac.c b/hw/net/npcm_gmac.c
index 0c17ae9b2a9..5e32cd3edf3 100644
--- a/hw/net/npcm_gmac.c
+++ b/hw/net/npcm_gmac.c
@@ -516,7 +516,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
     uint32_t desc_addr;
     struct NPCMGMACTxDesc tx_desc;
     uint32_t tx_buf_addr, tx_buf_len;
-    uint8_t *buf = tx_send_buffer;
     uint32_t prev_buf_size = 0;
     int csum = 0;
 
@@ -567,16 +566,15 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
         tx_buf_addr = tx_desc.tdes2;
         gmac->regs[R_NPCM_DMA_CUR_TX_BUF_ADDR] = tx_buf_addr;
         tx_buf_len = TX_DESC_TDES1_BFFR1_SZ_MASK(tx_desc.tdes1);
-        buf = &tx_send_buffer[prev_buf_size];
 
         if ((prev_buf_size + tx_buf_len) > tx_buffer_size) {
             tx_buffer_size = prev_buf_size + tx_buf_len;
             tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size);
-            buf = &tx_send_buffer[prev_buf_size];
         }
 
         /* step 5 */
-        if (dma_memory_read(&address_space_memory, tx_buf_addr, buf,
+        if (dma_memory_read(&address_space_memory, tx_buf_addr,
+                            tx_send_buffer + prev_buf_size,
                             tx_buf_len, MEMTXATTRS_UNSPECIFIED)) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: Failed to read packet @ 0x%x\n",
                         __func__, tx_buf_addr);
@@ -589,15 +587,14 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
             tx_buf_addr = tx_desc.tdes3;
             gmac->regs[R_NPCM_DMA_CUR_TX_BUF_ADDR] = tx_buf_addr;
             tx_buf_len = TX_DESC_TDES1_BFFR2_SZ_MASK(tx_desc.tdes1);
-            buf = &tx_send_buffer[prev_buf_size];
 
             if ((prev_buf_size + tx_buf_len) > tx_buffer_size) {
                 tx_buffer_size = prev_buf_size + tx_buf_len;
                 tx_send_buffer = g_realloc(tx_send_buffer, tx_buffer_size);
-                buf = &tx_send_buffer[prev_buf_size];
             }
 
-            if (dma_memory_read(&address_space_memory, tx_buf_addr, buf,
+            if (dma_memory_read(&address_space_memory, tx_buf_addr,
+                                tx_send_buffer + prev_buf_size,
                                 tx_buf_len, MEMTXATTRS_UNSPECIFIED)) {
                 qemu_log_mask(LOG_GUEST_ERROR,
                               "%s: Failed to read packet @ 0x%x\n",
@@ -615,7 +612,6 @@ static void gmac_try_send_next_packet(NPCMGMACState *gmac)
             net_checksum_calculate(tx_send_buffer, length, csum);
             qemu_send_packet(qemu_get_queue(gmac->nic), tx_send_buffer, length);
             trace_npcm_gmac_packet_sent(DEVICE(gmac)->canonical_path, length);
-            buf = tx_send_buffer;
             prev_buf_size = 0;
         }
 
-- 
2.43.0

Re: [PATCH 4/4] hw/net/npcm_gmac.c: Drop 'buf' local variable

[Philippe Mathieu-Daudé]

On 14/7/25 18:55, Peter Maydell wrote:
> We use the local variable 'buf' only when we call dma_memory_read(),
> and it is always set to &tx_send_buffer[prev_buf_size] immediately
> before both of those calls.  So remove the variable and pass
> tx_send_buffer + prev_buf_size to dma_memory_read().
> 
> This fixes in passing a place where we set buf = tx_send_buffer
> but never used that value because we always updated buf to
> something else later before using it.
> 
> Coverity: CID 1534027
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/npcm_gmac.c | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/4] hw/net/npcm_gmac.c: Correct test for when to reallocate packet buffer

[Philippe Mathieu-Daudé]

On 14/7/25 18:55, Peter Maydell wrote:
> In gmac_try_send_next_packet() we have code that does "if this block
> of data won't fit in the buffer, reallocate it".  However, the
> condition it uses is
>    if ((prev_buf_size + tx_buf_len) > sizeof(buf))
> where buf is a uint8_t *.
> 
> This means that sizeof(buf) is always 8 bytes, and the condition will
> almost always be true, so we will reallocate the buffer more often
> than we need to.
> 
> Correct the condition to test against tx_buffer_size, which is
> where we track how big the allocated buffer is.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/npcm_gmac.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 3/4] hw/net/npcm_gmac.c: Correct test for when to reallocate packet buffer

[Hao Wu]

[-- Attachment #1: Type: text/plain, Size: 994 bytes --]

Thank you for catching that!

On Mon, Jul 14, 2025 at 1:08 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:

> On 14/7/25 18:55, Peter Maydell wrote:
> > In gmac_try_send_next_packet() we have code that does "if this block
> > of data won't fit in the buffer, reallocate it".  However, the
> > condition it uses is
> >    if ((prev_buf_size + tx_buf_len) > sizeof(buf))
> > where buf is a uint8_t *.
> >
> > This means that sizeof(buf) is always 8 bytes, and the condition will
> > almost always be true, so we will reallocate the buffer more often
> > than we need to.
> >
> > Correct the condition to test against tx_buffer_size, which is
> > where we track how big the allocated buffer is.
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/net/npcm_gmac.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
> Reviewed-by: Hao Wu <wuhaotsh@google.com>

[-- Attachment #2: Type: text/html, Size: 1604 bytes --]

Re: [PATCH 2/4] hw/net/npcm_gmac.c: Unify length and prev_buf_size variables

[Peter Maydell]

On Mon, 14 Jul 2025 at 17:55, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> After the bug fix in the previous commit, the length and prev_buf_size
> variables are identical, except that prev_buf_size is uint32_t and
> length is uint16_t. We can therefore unify them. The only place where
> the type makes a difference is that we will truncate the packet
> at 64K when sending it; this commit preserves that behaviour
> by using a local variable when doing the packet send.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Should the behaviour really be to truncate the packet at 64K
> rather than flagging an error if the guest assembles descriptors
> that combine to give a too-large packet?

I see Jason has picked this up in a pullreq, which is OK given
this isn't a behaviour change -- but it would still be good
if somebody with access to the datasheet for this device could
confirm what the actual correct behaviour for attempts to tx
very large packets should be.

thanks
-- PMM