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

[Thomas Huth <thuth@redhat.com>]

On 04/03/2026 21.17, Jared Rossi wrote:
> 
> 
> 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?

Ok, having the error message "Failed to locate ..." sounds good to me, so 
I'm fine if you keep this patch as it is. I was just worried that we end up 
with the bios crashing in weird ways due to the endianness issues, and that 
you could not tell by the output on the serial console what was going on. 
But since we have a clear error message instead of a crash, I think we're fine.

Thanks for checking!

  Thomas