Re: [PATCH v2 2/7] pc-bios/s390-ccw: Add dynamic net header size handling

[Jared Rossi <jrossi@linux.ibm.com>]

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 <zycai@linux.ibm.com>
> ---
>   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;