On 9/3/26 12:24 PM, Zhuoying Cai wrote:
The virtio-net device used a fixed header size that did not account for
the num_buffers field used in VirtIO 1.0 or for the mergeable receive
buffers feature.
Use dynamic header sizing: 10 bytes for legacy mode and 12 bytes for
VirtIO 1.0 or when VIRTIO_NET_F_MRG_RXBUF is enabled. This ensures
correct packet handling across different VirtIO configurations.
Signed-off-by: Zhuoying Cai <[email protected]>
---
pc-bios/s390-ccw/virtio-net.c | 30 ++++++++++++++++++++++++------
1 file changed, 24 insertions(+), 6 deletions(-)
diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
index f58f7ffc55..afa728bc32 100644
--- a/pc-bios/s390-ccw/virtio-net.c
+++ b/pc-bios/s390-ccw/virtio-net.c
@@ -20,6 +20,7 @@
#include "s390-ccw.h"
#include "virtio.h"
#include "virtio-ccw.h"
+#include "virtio-pci.h"
#include "s390-time.h"
#include "helper.h"
@@ -28,6 +29,7 @@
#endif
#define VIRTIO_NET_F_MAC_BIT (1 << 5)
+#define VIRTIO_NET_F_MRG_RXBUF_BIT (1 << 15)
We define this new feature bit, but it looks like we never set it.
The existing virtio_net_init() code has only:
vdev->guest_features[0] = VIRTIO_NET_F_MAC_BIT;
Should it be updated to VIRTIO_NET_F_MAC_BIT | VIRTIO_NET_F_MRG_RXBUF_BIT?
Is the new feature bit something we want to unconditionally request, only
request for PCI, or do we simply not care about it?
#define VQ_RX 0 /* Receive queue */
#define VQ_TX 1 /* Transmit queue */
@@ -43,7 +45,18 @@ struct VirtioNetHdr {
};
typedef struct VirtioNetHdr VirtioNetHdr;
+struct VirtioNetHdrMrgRxbuf {
+ struct VirtioNetHdr hdr;
+ uint16_t num_buffers; /* Only with VIRTIO_NET_F_MRG_RXBUF or VIRTIO1 */
+};
+typedef struct VirtioNetHdrMrgRxbuf VirtioNetHdrMrgRxbuf;
+
+/* Header sizes for different modes */
+#define VIRTIO_NET_HDR_SIZE_LEGACY sizeof(VirtioNetHdr)
+#define VIRTIO_NET_HDR_SIZE_V1 sizeof(VirtioNetHdrMrgRxbuf)
+
static uint16_t rx_last_idx; /* Last index in receive queue "used" ring */
+static int virtio_net_hdr_size;
int virtio_net_init(void *mac_addr)
{
@@ -62,12 +75,17 @@ int virtio_net_init(void *mac_addr)
return -1;
}
+ virtio_net_hdr_size = ((vdev->guest_features[1] & VIRTIO_F_VERSION_1) ||
+ (vdev->guest_features[0] &
VIRTIO_NET_F_MRG_RXBUF_BIT))
Because we do not set VIRTIO_NET_F_MRG_RXBUF in net_init(), only the first
half of this check can ever return true.
Maybe that is correct, but in that case we can simplify this to just
vdev->guest_features[1] & VIRTIO_F_VERSION_1, right?
Regards,
Jared Rossi
+ ? VIRTIO_NET_HDR_SIZE_V1
+ : VIRTIO_NET_HDR_SIZE_LEGACY;
+
memcpy(mac_addr, vdev->config.net.mac, ETH_ALEN);
for (i = 0; i < 64; i++) {
- buf = malloc(ETH_MTU_SIZE + sizeof(VirtioNetHdr));
+ buf = malloc(ETH_MTU_SIZE + virtio_net_hdr_size);
IPL_assert(buf != NULL, "Can not allocate memory for receive
buffers");
- vring_send_buf(rxvq, buf, ETH_MTU_SIZE + sizeof(VirtioNetHdr),
+ vring_send_buf(rxvq, buf, ETH_MTU_SIZE + virtio_net_hdr_size,
VRING_DESC_F_WRITE);
}
vring_notify(rxvq);
@@ -77,14 +95,14 @@ int virtio_net_init(void *mac_addr)
int send(int fd, const void *buf, int len, int flags)
{
- VirtioNetHdr tx_hdr;
+ VirtioNetHdrMrgRxbuf tx_hdr;
VDev *vdev = virtio_get_device();
VRing *txvq = &vdev->vrings[VQ_TX];
/* Set up header - we do not use anything special, so simply clear it */
memset(&tx_hdr, 0, sizeof(tx_hdr));
- vring_send_buf(txvq, &tx_hdr, sizeof(tx_hdr), VRING_DESC_F_NEXT);
+ vring_send_buf(txvq, &tx_hdr, virtio_net_hdr_size, VRING_DESC_F_NEXT);
vring_send_buf(txvq, (void *)buf, len, VRING_HIDDEN_IS_CHAIN);
while (!vr_poll(txvq)) {
yield();
@@ -108,13 +126,13 @@ int recv(int fd, void *buf, int maxlen, int flags)
return 0;
}
- len = rxvq->used->ring[rx_last_idx % rxvq->num].len - sizeof(VirtioNetHdr);
+ len = rxvq->used->ring[rx_last_idx % rxvq->num].len - virtio_net_hdr_size;
if (len > maxlen) {
puts("virtio-net: Receive buffer too small");
len = maxlen;
}
id = rxvq->used->ring[rx_last_idx % rxvq->num].id % rxvq->num;
- pkt = (uint8_t *)(rxvq->desc[id].addr + sizeof(VirtioNetHdr));
+ pkt = (uint8_t *)(rxvq->desc[id].addr + virtio_net_hdr_size);
#if DEBUG_VIRTIO_NET /* Dump packet */
int i;
