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?
Regards,
Jared Rossi
