[libvirt] [PATCH] Ignore virtio-mmio disks in qemuAssignDevicePCISlots()
Fixes the following error when attempting to add a disk with bus='virtio': virtio only support device address type 'PCI' Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_command.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 38104da..408b249 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2189,12 +2189,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->disks[i]->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) continue; - /* don't touch s390 devices */ + /* don't touch s390 and virtio-mmio devices */ if (def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 || def->disks[i]->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + def->disks[i]->info.type == + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) continue; if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { -- 1.9.5.msysgit.0
[Please don't Cc random people when they did not request it, all developers are subscribed to the list] On Tue, Sep 08, 2015 at 03:23:27PM +0300, Pavel Fedin wrote:
Fixes the following error when attempting to add a disk with bus='virtio':
virtio only support device address type 'PCI'
How did you manage to do that? I think we are not handlind virtio-mmio addressing properly as we won't add some controller qemu will then be missing. Shouldn't that be fixed as well? And isn't virtio-mmio only supported on some platforms? Otherwise the change makes sense.
Signed-off-by: Pavel Fedin <p.fedin@samsung.com> --- src/qemu/qemu_command.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 38104da..408b249 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2189,12 +2189,14 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, if (def->disks[i]->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) continue;
- /* don't touch s390 devices */ + /* don't touch s390 and virtio-mmio devices */ if (def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI || def->disks[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_S390 || def->disks[i]->info.type == - VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW) + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCW || + def->disks[i]->info.type == + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO) continue;
if (def->disks[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { -- 1.9.5.msysgit.0
Hello!
How did you manage to do that?
Probably i forgot to explicitly specify that it affects ARM 'virt' machine. Before we added PCIe support we were able to use only virtio-mmio there. Now we can use both, because virtio-mmio support in qemu didn't go anywhere. It worked before PCIe support introduction just because qemuAssignDevicePCISlots() was never called.
I think we are not handlind virtio-mmio addressing properly as we won't add some controller qemu will then be missing. Shouldn't that be fixed as well?
Didn't understand exactly what you mean, but <address type='virtio-mmio'/> perfectly works. Even more, this is the default on ARM for backwards compatibility, as we decided before. virtio-mmio is assigned either manually or automatically for certain device kinds by qemuDomainAssignARMVirtioMMIOAddresses(). After it, qemuDomainAssignPCIAddresses() is called, which before PCIe introduction just did nothing. Now it tries to assign PCI addresses to anything which still doesn't have addresses at all. For the majority of devices it checks only for info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE, but for VirtIO disks it is a bit more picky for some reason. The problem is easy to reproduce with virt-manager. Just try to add a disk (or modify existing disk) and specify bus = virtio. By default, since we now have PCIe, it will add virtio-scsi disk on top of SCSI controller, so the default works flawlessly. Because virtio-scsi != virtio-block.
And isn't virtio-mmio only supported on some platforms?
Yes. But for platforms where virtio-mmio is not supported we can not get VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO addresses, because qemuDomainAssignARMVirtioMMIOAddresses() will not do anything, because QEMU_CAPS_DEVICE_VIRTIO_MMIO will not be set. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
On Wed, Sep 09, 2015 at 09:38:44AM +0300, Pavel Fedin wrote:
Hello!
How did you manage to do that?
Probably i forgot to explicitly specify that it affects ARM 'virt' machine. Before we added PCIe support we were able to use only virtio-mmio there. Now we can use both, because virtio-mmio support in qemu didn't go anywhere. It worked before PCIe support introduction just because qemuAssignDevicePCISlots() was never called.
OK, good that I tried it on an arm machine :)
I think we are not handlind virtio-mmio addressing properly as we won't add some controller qemu will then be missing. Shouldn't that be fixed as well?
Didn't understand exactly what you mean, but <address type='virtio-mmio'/> perfectly works. Even more, this is the default on ARM for backwards compatibility, as we decided before. virtio-mmio is assigned either manually or automatically for certain device kinds by qemuDomainAssignARMVirtioMMIOAddresses(). After it, qemuDomainAssignPCIAddresses() is called, which before PCIe introduction just did nothing. Now it tries to assign PCI addresses to anything which still doesn't have addresses at all. For the majority of devices it checks only for info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE, but for VirtIO disks it is a bit more picky for some reason.
The problem is easy to reproduce with virt-manager. Just try to add a disk (or modify existing disk) and specify bus = virtio. By default, since we now have PCIe, it will add virtio-scsi disk on top of SCSI controller, so the default works flawlessly. Because virtio-scsi != virtio-block.
So that's what I was missing. virt-manager probably automatically adds the controller, but libvirt itself doesn't. That's what I meant that we should probably add that too (see *Add*Maybe() functions), but that's not to be done in this patch, though.
And isn't virtio-mmio only supported on some platforms?
Yes. But for platforms where virtio-mmio is not supported we can not get VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_MMIO addresses, because qemuDomainAssignARMVirtioMMIOAddresses() will not do anything, because QEMU_CAPS_DEVICE_VIRTIO_MMIO will not be set.
Well, without your patch, I get a clear message for non-arm (e.g. x86_64) machine that says "virtio disk cannot have an address of type 'virtio-mmio'" (even though I have en_GB locale in which the string should be very badly translated, but that's another problem). And that is what we should keep for architectures without virtio-mmio support, shouldn't we?
Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
Hello!
Well, without your patch, I get a clear message for non-arm (e.g. x86_64) machine that says "virtio disk cannot have an address of type 'virtio-mmio'" (even though I have en_GB locale in which the string should be very badly translated, but that's another problem). And that is what we should keep for architectures without virtio-mmio support, shouldn't we?
Agree, thanks for testing. I don't have x86 installation here. I will make v2 which will check for QEMU_CAPS_DEVICE_VIRTIO_MMIO capability. Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia
participants (2)
-
Martin Kletzander -
Pavel Fedin