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 9:39 AM, Thomas Huth wrote:
> On 04/03/2026 15.29, Jared Rossi wrote:
>>
>>
>> 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?
>
> Ah, right, I missed the initialization in virtio_blk_setup_device(), 
> so we should be fine here, indeed!
>
>>>
>>>> +    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.
>
> No, I think it's fine for the default case. I'm just wondering what 
> happens when someone uses "disable-modern=on" ... I guess the code 
> will currently behave in weird ways since the endianness is wrong ... 
> thus I thought it might be better to check VIRTIO_F_VERSION_1 again 
> and emit a proper error message in this case?
>

I tried running with "disable-moden=on" and it failed very early in the 
virtio-pci setup when trying to read the PCI configuration space.

    Failed to locate PCI common configuration
    Invalid virtio PCI capabilities
    ERROR: No suitable device for IPL. Halting...


Actually that happens before we even try to negotiate 
VIRTIO_F_VERSION_1.  From the virtio spec, it looks like the legacy 
interface requires the common configuration to be in BAR0 (4.1.4.10), 
while we normally expect BAR15 to specify the layout. Typically we need 
to read the capabilities config in BAR15 to determine which BAR the 
common config is in, then that location is used when negotiating the 
features, etc.  My guess is BAR15 isn't populated when 
"disable-modern=on" so it bails out when there is no capabilities 
configuration.

But as far as I can tell it isn't an endianness issue since we are 
trying to read single byte fields at this point anyway.  What are your 
thoughts?

Regards,
Jared Rossi