In e1000e_write_packet_to_guest() we don't write data for RX descriptors
where the buffer address is NULL (as required by the i82574 datasheet
section 7.1.7.2). However, when we do this we still update desc_offset
by the amount of data we would have written to the RX descriptor if
it had a valid buffer pointer, resulting in our dropping that data
entirely. The data sheet is not 100% clear on the subject, but this
seems unlikely to be the correct behaviour.

Rearrange the null-descriptor logic so that we don't treat these
do-nothing descriptors as if we'd really written the data.

This both fixes a bug and also is a prerequisite to cleaning up
the size calculation logic in the next patch.

(Cc to stable largely because it will be needed for the next patch,
which fixes a more serious bug.)

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/net/e1000e_core.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index 8fef598b498..ba77cb6011f 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -1481,7 +1481,6 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
     PCIDevice *d = core->owner;
     dma_addr_t base;
     union e1000_rx_desc_union desc;
-    size_t desc_size;
     size_t desc_offset = 0;
     size_t iov_ofs = 0;
 
@@ -1500,12 +1499,6 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
         E1000EBAState bastate = { { 0 } };
         bool is_last = false;
 
-        desc_size = total_size - desc_offset;
-
-        if (desc_size > core->rx_desc_buf_size) {
-            desc_size = core->rx_desc_buf_size;
-        }
-
         if (e1000e_ring_empty(core, rxi)) {
             return;
         }
@@ -1519,6 +1512,12 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
         e1000e_read_rx_descr(core, &desc, ba);
 
         if (ba[0]) {
+            size_t desc_size = total_size - desc_offset;
+
+            if (desc_size > core->rx_desc_buf_size) {
+                desc_size = core->rx_desc_buf_size;
+            }
+
             if (desc_offset < size) {
                 static const uint32_t fcs_pad;
                 size_t iov_copy;
@@ -1582,13 +1581,13 @@ e1000e_write_packet_to_guest(E1000ECore *core, struct NetRxPkt *pkt,
                           (const char *) &fcs_pad, e1000x_fcs_len(core->mac));
                 }
             }
+            desc_offset += desc_size;
+            if (desc_offset >= total_size) {
+                is_last = true;
+            }
         } else { /* as per intel docs; skip descriptors with null buf addr */
             trace_e1000e_rx_null_descriptor();
         }
-        desc_offset += desc_size;
-        if (desc_offset >= total_size) {
-            is_last = true;
-        }
 
         e1000e_write_rx_descr(core, &desc, is_last ? core->rx_pkt : NULL,
                            rss_info, do_ps ? ps_hdr_len : 0, &bastate.written);
-- 
2.43.0

On 3/11/25 18:58, Peter Maydell wrote:
> In e1000e_write_packet_to_guest() we don't write data for RX descriptors
> where the buffer address is NULL (as required by the i82574 datasheet
> section 7.1.7.2). However, when we do this we still update desc_offset
> by the amount of data we would have written to the RX descriptor if
> it had a valid buffer pointer, resulting in our dropping that data
> entirely. The data sheet is not 100% clear on the subject, but this
> seems unlikely to be the correct behaviour.
> 
> Rearrange the null-descriptor logic so that we don't treat these
> do-nothing descriptors as if we'd really written the data.
> 
> This both fixes a bug and also is a prerequisite to cleaning up
> the size calculation logic in the next patch.
> 
> (Cc to stable largely because it will be needed for the next patch,
> which fixes a more serious bug.)
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   hw/net/e1000e_core.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)

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

On 2025/11/04 2:58, Peter Maydell wrote:
> In e1000e_write_packet_to_guest() we don't write data for RX descriptors
> where the buffer address is NULL (as required by the i82574 datasheet
> section 7.1.7.2). However, when we do this we still update desc_offset
> by the amount of data we would have written to the RX descriptor if
> it had a valid buffer pointer, resulting in our dropping that data
> entirely. The data sheet is not 100% clear on the subject, but this
> seems unlikely to be the correct behaviour.
> 
> Rearrange the null-descriptor logic so that we don't treat these
> do-nothing descriptors as if we'd really written the data.

Please make a corresponding change for igb too so that the code of these 
two devices will not diverge further.

Regards,
Akihiko Odaki

On Tue, 4 Nov 2025 at 06:11, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp> wrote:
>
> On 2025/11/04 2:58, Peter Maydell wrote:
> > In e1000e_write_packet_to_guest() we don't write data for RX descriptors
> > where the buffer address is NULL (as required by the i82574 datasheet
> > section 7.1.7.2). However, when we do this we still update desc_offset
> > by the amount of data we would have written to the RX descriptor if
> > it had a valid buffer pointer, resulting in our dropping that data
> > entirely. The data sheet is not 100% clear on the subject, but this
> > seems unlikely to be the correct behaviour.
> >
> > Rearrange the null-descriptor logic so that we don't treat these
> > do-nothing descriptors as if we'd really written the data.
>
> Please make a corresponding change for igb too so that the code of these
> two devices will not diverge further.

The igb_core.c version of this function seems to be
rather different (and rather easier to read). It has a
kind of related bug where igb_write_packet_to_guest()
calls igb_write_to_rx_buffers() and assumes that that
function has correctly set pdma_st.desc_size to the
amount of data written to that descriptor, but the
early-return cases from igb_write_to_rx_buffers()
don't actually update that field so it will be whatever
junk was present from the previous iteration.

So it looks to me like there are similar bugs but the
code in these two devices is already pretty different
and the fixes don't transpose one-to-one. (For instance,
the problem with the assert location fixed in patch 3
here doesn't seem to be present in igb, which already
does the assert() at the top of its loop in
igb_write_payload_frag_to_rx_buffers().)

In any case, I don't think it makes sense to look at
fixing igb before this patchset has been reviewed
and we're confident the fixes are actually right :-)

thanks
-- PMM