Re: [PATCH v1 4/7] pc-bios/s390-ccw: Add support for virtio-net-pci IPL

[Zhuoying Cai <zycai@linux.ibm.com>]

Thanks for the feedback!

On 8/26/26 12:44 PM, Jared Rossi wrote:
> 
> 
> On 8/18/26 4:53 PM, Zhuoying Cai wrote:
>> Enable network booting via virtio-net-pci by implementing PCI transport
>> support for virtio-net.
>>
>> This patch also adds endianness handling for virtio PCI ring operations in
>> little-endian to ensure correct behavior on s390x.
> 
> Endianness handling was actually added in the previous patch.  Is this a
> misplaced message? Or perhaps it should say extends, rather than adds?
> 

Good catch. It should be "extends" rather than "adds". I'll update the
commit message.

>> Signed-off-by: Zhuoying Cai <zycai@linux.ibm.com>
>> ---
>>   pc-bios/s390-ccw/main.c       |  1 +
>>   pc-bios/s390-ccw/virtio-net.c | 37 +++++++++++++++++++++++-------
>>   pc-bios/s390-ccw/virtio-pci.c | 43 +++++++++++++++++++++++++++++++++++
>>   pc-bios/s390-ccw/virtio-pci.h |  2 ++
>>   4 files changed, 75 insertions(+), 8 deletions(-)
>>
>> diff --git a/pc-bios/s390-ccw/main.c b/pc-bios/s390-ccw/main.c
>> index 8bc6e8eaa3..0eeb53a2bf 100644
>> --- a/pc-bios/s390-ccw/main.c
>> +++ b/pc-bios/s390-ccw/main.c
>> @@ -328,6 +328,7 @@ static void ipl_pci_device(void)
>>       }
>>   
>>       switch (vdev->dev_type) {
>> +    case VIRTIO_ID_NET:
>>       case VIRTIO_ID_BLOCK:
>>           if (virtio_setup() == 0) {
>>               zipl_load(); /* only return on error */
>> diff --git a/pc-bios/s390-ccw/virtio-net.c b/pc-bios/s390-ccw/virtio-net.c
>> index 3a9ae789cf..209f98a0e3 100644
>> --- a/pc-bios/s390-ccw/virtio-net.c
>> +++ b/pc-bios/s390-ccw/virtio-net.c
>> @@ -62,7 +62,16 @@ int virtio_net_init(void *mac_addr)
>>       rx_last_idx = 0;
>>   
>>       vdev->guest_features[0] = VIRTIO_NET_F_MAC_BIT;
>> -    virtio_ccw_setup(vdev);
>> +    switch (virtio_get_device()->ipl_type) {
>> +    case S390_IPL_TYPE_CCW:
>> +        virtio_ccw_setup(vdev);
>> +        break;
>> +    case S390_IPL_TYPE_PCI:
>> +        virtio_pci_setup(vdev);
>> +        break;
>> +    default:
>> +        return -1;
>> +    }
>>   
>>       if (!(vdev->guest_features[0] & VIRTIO_NET_F_MAC_BIT)) {
>>           puts("virtio-net device does not support the MAC address feature");
>> @@ -115,22 +124,30 @@ int recv(int fd, void *buf, int maxlen, int flags)
>>       VRing *rxvq = &vdev->vrings[VQ_RX];
>>       int len, id;
>>       uint8_t *pkt;
>> +    uint16_t rx_used_idx, rx_avail_idx;
>> +    uint32_t rx_used_len, rx_used_id;
>> +    uint64_t rx_desc_addr;
>>   
>> -    if (rx_last_idx == rxvq->used->idx) {
>> +    rx_used_idx = virtio_tswap16(rxvq->used->idx);
>> +    if (rx_last_idx == rx_used_idx) {
>>           return 0;
>>       }
>>   
>> -    len = rxvq->used->ring[rx_last_idx % rxvq->num].len - virtio_net_hdr_size;
>> +    rx_used_len = virtio_tswap32(rxvq->used->ring[rx_last_idx % rxvq->num].len);
>> +    rx_used_id = virtio_tswap32(rxvq->used->ring[rx_last_idx % rxvq->num].id);
>> +
>> +    len = rx_used_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 + virtio_net_hdr_size);
>> +    id = rx_used_id % rxvq->num;
>> +    rx_desc_addr = virtio_tswap64(rxvq->desc[id].addr);
>> +    pkt = (uint8_t *)(rx_desc_addr + virtio_net_hdr_size);
>>   
>>   #if DEBUG_VIRTIO_NET   /* Dump packet */
>>       int i;
>> -    printf("\nbuf %p: len=%i\n", (void *)rxvq->desc[id].addr, len);
>> +    printf("\nbuf %p: len=%i\n", (void *)rx_desc_addr, len);
>>       for (i = 0; i < 64; i++) {
>>           printf(" %02x", pkt[i]);
>>           if ((i % 16) == 15) {
>> @@ -144,8 +161,10 @@ int recv(int fd, void *buf, int maxlen, int flags)
>>       memcpy(buf, pkt, len);
>>   
>>       /* Mark buffer as available to the host again */
>> -    rxvq->avail->ring[rxvq->avail->idx % rxvq->num] = id;
>> -    rxvq->avail->idx = rxvq->avail->idx + 1;
>> +    rx_avail_idx = virtio_tswap16(rxvq->avail->idx);
>> +    rxvq->avail->ring[rx_avail_idx % rxvq->num] = virtio_tswap16(id);
>> +    rx_avail_idx++;
>> +    rxvq->avail->idx = virtio_tswap16(rx_avail_idx);
>>       vring_notify(rxvq);
>>   
>>       /* Move index to next entry */
>> @@ -164,6 +183,8 @@ bool virtio_net_setup(void)
>>       switch (virtio_get_device()->ipl_type) {
>>       case S390_IPL_TYPE_CCW:
>>           return virtio_ccw_net_setup();
>> +    case S390_IPL_TYPE_PCI:
>> +        return virtio_pci_net_setup();
>>       default:
>>           return false;
>>       }
>> diff --git a/pc-bios/s390-ccw/virtio-pci.c b/pc-bios/s390-ccw/virtio-pci.c
>> index f501252c81..c7d62766b8 100644
>> --- a/pc-bios/s390-ccw/virtio-pci.c
>> +++ b/pc-bios/s390-ccw/virtio-pci.c
>> @@ -52,6 +52,10 @@ void virtio_pci_id2type(VDev *vdev, uint16_t device_id)
>>       case 0x1001:
>>           vdev->dev_type = VIRTIO_ID_BLOCK;
>>           break;
>> +    case 0x1041:
>> +    case 0x1000:
>> +        vdev->dev_type = VIRTIO_ID_NET;
>> +        break;
>>       default:
>>           vdev->dev_type = 0;
>>       }
>> @@ -199,6 +203,14 @@ static int virtio_pci_get_blk_config(void)
>>       return rc;
>>   }
>>   
>> +static int virtio_pci_get_net_config(void)
>> +{
>> +    VirtioNetConfig *cfg = &virtio_get_device()->config.net;
>> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, sizeof(VirtioNetConfig));
>> +
>> +    return rc;
>> +}
>> +
>>   static int virtio_pci_negotiate(void)
>>   {
>>       int i, rc;
>> @@ -330,6 +342,7 @@ bool virtio_pci_is_supported(VDev *vdev)
>>       if (vdev->vendor_id == PCI_VENDOR_VIRTIO) {
>>           switch (vdev->dev_type) {
>>           case VIRTIO_ID_BLOCK:
>> +        case VIRTIO_ID_NET:
>>               return true;
>>           default:
>>               return false;
>> @@ -384,6 +397,11 @@ int virtio_pci_setup(VDev *vdev)
>>           vdev->cmd_vr_idx = 0;
>>           virtio_pci_get_blk_config();
>>           break;
>> +    case VIRTIO_ID_NET:
>> +        vdev->nr_vqs = 2;
>> +        vdev->cmd_vr_idx = 0;
>> +        virtio_pci_get_net_config();
>> +        break;
>>       default:
>>           puts("Unsupported virtio device");
>>           return -ENODEV;
>> @@ -458,3 +476,28 @@ int virtio_pci_setup_device(void)
>>   
>>       return 0;
>>   }
>> +
>> +static bool find_pci_net_dev(void)
>> +{
>> +    if (!virtio_is_supported(virtio_get_device())) {
>> +        return false;
>> +    }
>> +
>> +    if (virtio_get_device_type() != VIRTIO_ID_NET) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>> +}
>> +
>> +bool virtio_pci_net_setup(void)
>> +{
>> +    bool found = false;
>> +
>> +    if (have_iplb || store_iplb(&iplb)) {
>> +        IPL_assert(iplb.pbt == S390_IPL_TYPE_PCI, "IPL_TYPE_PCI expected");
>> +        found = find_pci_net_dev();
>> +    }
>> +
>> +    return found;
>> +}
> 
> As far as I can tell the net specific setup/find are not needed for PCI
> devices.  By the time we reach the the call to virtio_pci_net_setup() we
> know vdev->ipl_type == S390_IPL_TYPE_PCI because that is the prerequisite
> to call the function in the first place, and vdev->ipl_type was previously
> set based on the PBT.  We also already know that our device type is
> VIRTIO_ID_NET because otherwise the call to virtio_setup() in main.c
> would have fallen through to the default case and failed and/or netmain()
> would not be called from bootmap.c.
> 
> Although we haven't checked virtio_is_supported() yet, we will do that
> verification in virtio_net_init(), which is the very next step.
> 
> For PCI devices, everything checked from virtio_net_setup() appears to be
> guaranteed by the call chain or validated by subsequent net code. Maybe
> virtio_net_setup() only needs to be called for CCW devices?
> 

I was trying to align the CCW setup flow with the PCI devices, but I
agree that it isn't really necessary.

If virtio_net_setup() only needs to be called for CCW devices, is the
generic virtio_net_setup() wrapper still needed? Something like the
following seems sufficient:

if (virtio_get_device()->ipl_type == S390_IPL_TYPE_CCW &&
    !virtio_ccw_net_setup()) {
    puts("No valid virtio ccw net device found.");
}

Alternatively, we could simply add return true for the S390_IPL_TYPE_PCI
case in virtio_net_setup().

One thing worth noting is that virtio_net_init() calls virtio_*_setup(),
which performs virtio_is_supported() as its first step. However, the
return value of virtio_*_setup() is currently ignored in
virtio_net_init(). This is pre-existing behavior and applies to both the
CCW and PCI paths. If setup fails, execution still proceeds to MAC
negotiation and buffer allocation.

Since we are removing the network-specific setup from the PCI path,
would it make sense to also fix virtio_net_init() to check and propagate
the return value of virtio_*_setup()? The error is already reported by
virtio_*_setup(), but execution currently continues into MAC negotiation
and buffer allocation even when device initialization has failed.

> Regards,
> Jared Rossi
>> diff --git a/pc-bios/s390-ccw/virtio-pci.h b/pc-bios/s390-ccw/virtio-pci.h
>> index 33b683bd92..2c3136fd05 100644
>> --- a/pc-bios/s390-ccw/virtio-pci.h
>> +++ b/pc-bios/s390-ccw/virtio-pci.h
>> @@ -70,6 +70,8 @@ long virtio_pci_notify(VRing *vr);
>>   bool virtio_pci_is_supported(VDev *vdev);
>>   int virtio_pci_setup(VDev *vdev);
>>   int virtio_pci_setup_device(void);
>> +bool virtio_pci_is_supported(VDev *vdev);
>> +bool virtio_pci_net_setup(void);
>>   
>>   int vpci_read_flex(uint64_t offset, uint8_t pcias, void *buf, int len);
>>   int vpci_read_bswap64(uint64_t offset, uint8_t pcias, uint64_t *buf);
>