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, [email protected] wrote:
From: Jared Rossi <[email protected]>
Add little-endian virt-queue configuration and support for virtio-blk-
pci IPL
devices.
Signed-off-by: Jared Rossi <[email protected]>
---
...
+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
