Re: [PATCH v4 12/15] pc-bios/s390-ccw: Add support for virtio-blk-pci IPL

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

On 3/4/26 4:53 AM, Thomas Huth wrote:
> On 04/03/2026 03.59, jrossi@linux.ibm.com wrote:
>> From: Jared Rossi <jrossi@linux.ibm.com>
>>
>> Add little-endian virt-queue configuration and support for 
>> virtio-blk-pci IPL
>> devices.
>>
>> Signed-off-by: Jared Rossi <jrossi@linux.ibm.com>
>> ---
> ...
>> +static int virtio_pci_get_blk_config(void)
>> +{
>> +    VirtioBlkConfig *cfg = &virtio_get_device()->config.blk;
>> +    int rc = vpci_read_flex(d_cap.off, d_cap.bar, cfg, 
>> sizeof(VirtioBlkConfig));
>> +
>> +    /* single byte fields are not touched */
>> +    cfg->capacity = bswap64(cfg->capacity);
>> +    cfg->size_max = bswap32(cfg->size_max);
>> +    cfg->seg_max = bswap32(cfg->seg_max);
>> +
>> +    cfg->geometry.cylinders = bswap16(cfg->geometry.cylinders);
>> +
>> +    cfg->blk_size = bswap32(cfg->blk_size);
>> +    cfg->min_io_size = bswap16(cfg->min_io_size);
>> +    cfg->opt_io_size = bswap32(cfg->opt_io_size);
>
> So it looks like you read a bunch of optional config fields here ...
>
>> +    return rc;
>> +}
> ...
>> +int virtio_pci_setup(VDev *vdev)
>> +{
>> +    VRing *vr;
>> +    int rc;
>> +    uint8_t status;
>> +    uint16_t vq_size;
>> +    int i = 0;
>> +
>> +    vdev->guessed_disk_nature = VIRTIO_GDN_NONE;
>> +    vdev->cmd_vr_idx = 0;
>> +
>> +    if (virtio_pci_read_pci_cap_config()) {
>> +        puts("Invalid virtio PCI capabilities");
>> +        return -EIO;
>> +    }
>> +
>> +    if (enable_pci_bus_master()) {
>> +        return -EIO;
>> +    }
>> +
>> +    if (virtio_reset(vdev)) {
>> +        return -EIO;
>> +    }
>> +
>> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE;
>> +    if (virtio_pci_set_status(status)) {
>> +        puts("Virtio-pci device Failed to ACKNOWLEDGE");
>> +        return -EIO;
>> +    }
>
> ... so I think you should enable the corresponding feature bits in 
> vdev->guest_features[0] here? QEMU might be very forgiving and provide 
> you with the fields anyway, but let's better play safe. Something like:
>
>     vdev->guest_features[0] = VIRTIO_BLK_F_SIZE_MAX |
>                               VIRTIO_BLK_F_SEG_MAX |
>                               VIRTIO_BLK_F_GEOMETRY |
>                               VIRTIO_BLK_F_BLK_SIZE;
>
> ?

VIRTIO_BLK_F_GEOMETRY and VIRTIO_BLK_F_BLK_SIZE are already set during 
the virtio-blk setup.  I actually don't think the other two fields are 
used, I jut swapped any fields larger than 1 byte.  I suppose the 
feature bits should be enabled though... otherwise we could just just 
not touch the unused fields at all?

>
>> +    vdev->guest_features[1] = VIRTIO_F_VERSION_1;
>> +    if (virtio_pci_negotiate()) {
>> +        panic("Virtio feature negotation failed!");
>> +    }
>
> Maybe double-check whether VIRTIO_F_VERSION_1 has really been 
> negotiated? Otherwise, what happens if a user runs QEMU with "-device 
> virtio-blk-pci,disable-modern=on" ?

I haven't tried running it with "disable-modern=on" (I will test that 
next) but the config is big endian if I don't negotiate that feature 
bit, and little endian if I do, which I think is the expected behavior 
when VIRTIO_F_VERSION_1 is set.

Just for my understanding, do you see something that makes you suspect 
the negotiation isn't actually happening?  I will try running with 
"disable-modern=on" and let you know the results.

Thanks for the reviews,
Jared Rossi