On 9/8/26 2:32 PM, Jared Rossi wrote:
>
>
> 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?
As of now, we do not care about VIRTIO_NET_F_MRG_RXBUF_BIT. I added it
because the num_buffers field in VirtioNetHdrMrgRxbuf is present when
either condition is true, and we might want to use it in the future.
>>
>> #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?
>
Since VIRTIO_NET_F_MRG_RXBUF is neither negotiated nor used today, I can
remove it entirely and simplify the check to vdev->guest_features[1] &
VIRTIO_F_VERSION_1.
> 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;
>
