[libvirt] [PATCH 0/6] Several Q35 related tweaks
These patches are all in response to the issues that Gerd Hoffman raised in https://bugzilla.redhat.com/show_bug.cgi?id=1003983 Three of the six are simple refactoring of existing code, done separately so that the patches that change functionality would be simpler to review. None of them is very long. Laine Stump (6): qemu: eliminate redundant if clauses in qemuCollectPCIAddress qemu: allow some PCI devices to be attached to PCIe slots qemu: replace multiple strcmps with a switch on an enum qemu: support ich9-intel-hda audio device qemu: turn if into switch in qemuDomainValidateDevicePCISlotsQ35 qemu: prefer to put a Q35 machine's dmi-to-pci-bridge at 00:1E.0 docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 6 +- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 197 +++++++++++++++------ src/qemu/qemu_command.h | 4 + tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- .../qemuxml2argv-pcihole64-q35.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- .../qemuxml2argv-sound-device.args | 7 +- .../qemuxml2argvdata/qemuxml2argv-sound-device.xml | 5 + tests/qemuxml2argvtest.c | 3 +- 13 files changed, 175 insertions(+), 58 deletions(-) -- 1.8.3.1
Replace them with switch cases. This will make it more efficient when we add exceptions more controller types, and other device types. This is a prerequisite for patches to resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1003983 --- src/qemu/qemu_command.c | 57 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 25 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0376611..2683a9c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1731,37 +1731,44 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, /* Change flags according to differing requirements of different * devices. */ - if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && - device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_PCI) { - switch (device->data.controller->model) { - case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: - /* pci-bridge needs a PCI slot, but it isn't - * hot-pluggable, so it doesn't need a hot-pluggable slot. - */ - flags = QEMU_PCI_CONNECT_TYPE_PCI; + switch (device->type) { + case VIR_DOMAIN_DEVICE_CONTROLLER: + switch (device->data.controller->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + switch (device->data.controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_PCI_BRIDGE: + /* pci-bridge needs a PCI slot, but it isn't + * hot-pluggable, so it doesn't need a hot-pluggable slot. + */ + flags = QEMU_PCI_CONNECT_TYPE_PCI; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: + /* pci-bridge needs a PCIe slot, but it isn't + * hot-pluggable, so it doesn't need a hot-pluggable slot. + */ + flags = QEMU_PCI_CONNECT_TYPE_PCIE; + break; + default: + break; + } break; - case VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE: - /* pci-bridge needs a PCIe slot, but it isn't - * hot-pluggable, so it doesn't need a hot-pluggable slot. + + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + /* SATA controllers aren't hot-plugged, and can be put in + * either a PCI or PCIe slot */ - flags = QEMU_PCI_CONNECT_TYPE_PCIE; - break; - default: + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; break; } - } - /* SATA controllers aren't hot-plugged, and can be put in either a - * PCI or PCIe slot - */ - if (device->type == VIR_DOMAIN_DEVICE_CONTROLLER && - device->data.controller->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA) - flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; + break; - /* video cards aren't hot-plugged, and can be put in either a PCI - * or PCIe slot - */ - if (device->type == VIR_DOMAIN_DEVICE_VIDEO) + case VIR_DOMAIN_DEVICE_VIDEO: + /* video cards aren't hot-plugged, and can be put in either a + * PCI or PCIe slot + */ flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; + break; + } /* Ignore implicit controllers on slot 0:0:1.0: * implicit IDE controller on 0:0:1.1 (no qemu command line) -- 1.8.3.1
On 09/25/2013 02:30 PM, Laine Stump wrote:
Replace them with switch cases. This will make it more efficient when we add exceptions more controller types, and other device types.
s/more/for more/
This is a prerequisite for patches to resolve:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983 --- src/qemu/qemu_command.c | 57 +++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 25 deletions(-)
ACK Jan
Part of the resolution to: https://bugzilla.redhat.com/show_bug.cgi?id=1003983 Although most devices available in qemu area defined as PCI devices, and strictly speaking should only be attached via a PCI slot, in practice qemu allows them to be attached to a PCIe slot and sometimes this makes sense. For example, The UHCI and EHCI USB controllers are usually attached directly to the PCIe "root complex" (i.e. PCIe slots) on real hardware, so that should be possible for a Q35-based qemu virtual machine as well. We still want to prefer a standard PCi slot when auto-assigning addresses, though, and in general to disallow attaching PCI devices via PCIe slots. This patch makes that possible by adding a new QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG flag. Three things are done with this flag: 1) It is set for the "pcie-root" controller 2) qemuCollectPCIAddress() now has a set of nested switches that set this "EITHER" flag for devices that we want to allow connecting to pcie-root when specifically requested in the config. 3) qemuDomainPCIAddressFlagsCompatible() adds this new flag to the "flagsMatchMask" if the address being checked came from config rather than being newly auto-allocated by libvirt (this knowledge is conveniently already available in the "fromConfig" arg). Now any device having the EITHER flag set can be connected to pcie-root if explicitly requested, but auto-allocated addresses for those devices will still be standard PCI slots instead. This patch only loosens the restrictions on devices that have been specifically requested, but the setup is such that it should be fairly easy to add new devices. --- src/qemu/qemu_command.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 4 ++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2683a9c..280d8d2 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1496,12 +1496,16 @@ qemuDomainPCIAddressFlagsCompatible(virDevicePCIAddressPtr addr, { virErrorNumber errType = (fromConfig ? VIR_ERR_XML_ERROR : VIR_ERR_INTERNAL_ERROR); + qemuDomainPCIConnectFlags flagsMatchMask = QEMU_PCI_CONNECT_TYPES_MASK; + + if (fromConfig) + flagsMatchMask |= QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG; /* If this bus doesn't allow the type of connection (PCI * vs. PCIe) required by the device, or if the device requires * hot-plug and this bus doesn't have it, return false. */ - if (!(devFlags & busFlags & QEMU_PCI_CONNECT_TYPES_MASK)) { + if (!(devFlags & busFlags & flagsMatchMask)) { if (reportError) { if (devFlags & QEMU_PCI_CONNECT_TYPE_PCI) { virReportError(errType, @@ -1620,8 +1624,12 @@ qemuDomainPCIAddressBusSetModel(qemuDomainPCIAddressBusPtr bus, bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; case VIR_DOMAIN_CONTROLLER_MODEL_PCIE_ROOT: - /* slots 1 - 31, PCIe devices only, no hotplug */ - bus->flags = QEMU_PCI_CONNECT_TYPE_PCIE; + /* slots 1 - 31, no hotplug, PCIe only unless the address was + * specified in user config *and* the particular device being + * attached also allows it + */ + bus->flags = (QEMU_PCI_CONNECT_TYPE_PCIE | + QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG); bus->minSlot = 1; bus->maxSlot = QEMU_PCI_ADDRESS_SLOT_LAST; break; @@ -1759,6 +1767,42 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, */ flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; break; + + case VIR_DOMAIN_CONTROLLER_TYPE_USB: + /* allow UHCI and EHCI controllers to be manually placed on + * the PCIe bus (but don't put them there automatically) + */ + switch (device->data.controller->model) { + case VIR_DOMAIN_CONTROLLER_MODEL_USB_EHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_EHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI1: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI2: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_ICH9_UHCI3: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_VT82C686B_UHCI: + flags = (QEMU_PCI_CONNECT_TYPE_PCI | + QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG); + break; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_NEC_XHCI: + /* should this be PCIE-only? Or do we need to allow PCI + * for backward compatibility? + */ + flags = QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_PCIE; + break; + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PCI_OHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX3_UHCI: + case VIR_DOMAIN_CONTROLLER_MODEL_USB_PIIX4_UHCI: + /* Allow these for PCI only */ + break; + } + } + break; + + case VIR_DOMAIN_DEVICE_SOUND: + switch (device->data.sound->model) { + case VIR_DOMAIN_SOUND_MODEL_ICH6: + flags = (QEMU_PCI_CONNECT_TYPE_PCI | + QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG); + break; } break; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 0e16a3d..2e2acfb 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -239,6 +239,10 @@ typedef enum { /* PCI devices can connect to this bus */ QEMU_PCI_CONNECT_TYPE_PCIE = 1 << 3, /* PCI Express devices can connect to this bus */ + QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG = 1 << 4, + /* PCI *and* PCIe devices allowed, if the address + * was specified in the config by the user + */ } qemuDomainPCIConnectFlags; /* a combination of all bit that describe the type of connections -- 1.8.3.1
On 09/25/2013 02:30 PM, Laine Stump wrote:
Part of the resolution to:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983
Although most devices available in qemu area defined as PCI devices, and strictly speaking should only be attached via a PCI slot, in practice qemu allows them to be attached to a PCIe slot and sometimes this makes sense.
For example, The UHCI and EHCI USB controllers are usually attached directly to the PCIe "root complex" (i.e. PCIe slots) on real hardware, so that should be possible for a Q35-based qemu virtual machine as well.
We still want to prefer a standard PCi slot when auto-assigning
*PCI
addresses, though, and in general to disallow attaching PCI devices via PCIe slots.
This patch makes that possible by adding a new QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG flag. Three things are done with this flag:
1) It is set for the "pcie-root" controller
2) qemuCollectPCIAddress() now has a set of nested switches that set this "EITHER" flag for devices that we want to allow connecting to pcie-root when specifically requested in the config.
3) qemuDomainPCIAddressFlagsCompatible() adds this new flag to the "flagsMatchMask" if the address being checked came from config rather than being newly auto-allocated by libvirt (this knowledge is conveniently already available in the "fromConfig" arg).
Now any device having the EITHER flag set can be connected to pcie-root if explicitly requested, but auto-allocated addresses for those devices will still be standard PCI slots instead.
This patch only loosens the restrictions on devices that have been specifically requested, but the setup is such that it should be fairly easy to add new devices. --- src/qemu/qemu_command.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 4 ++++ 2 files changed, 51 insertions(+), 3 deletions(-)
The code looks OK, and it seems to work with my limited testing. ACK, but I don't know the answer to that USB_NEC_XHCI question. Jan
I'm not sure why this code was written to compare the strings that it had just retrieved from an enum->string conversion, rather than just look at the original enum values, but this yields the same results, and is much more efficient (especially as you add more devices). This is a prerequisite for patches to resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1003983 --- src/qemu/qemu_command.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 280d8d2..3156cff 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5269,13 +5269,18 @@ qemuBuildSoundDevStr(virDomainDefPtr def, goto error; } - /* Hack for weirdly unusual devices name in QEMU */ - if (STREQ(model, "es1370")) + /* Hack for devices with different names in QEMU and libvirt */ + switch (sound->model) { + case VIR_DOMAIN_SOUND_MODEL_ES1370: model = "ES1370"; - else if (STREQ(model, "ac97")) + break; + case VIR_DOMAIN_SOUND_MODEL_AC97: model = "AC97"; - else if (STREQ(model, "ich6")) + break; + case VIR_DOMAIN_SOUND_MODEL_ICH6: model = "intel-hda"; + break; + } virBufferAsprintf(&buf, "%s,id=%s", model, sound->info.alias); if (qemuBuildDeviceAddressStr(&buf, def, &sound->info, qemuCaps) < 0) -- 1.8.3.1
On 09/25/2013 02:30 PM, Laine Stump wrote:
I'm not sure why this code was written to compare the strings that it had just retrieved from an enum->string conversion, rather than just look at the original enum values, but this yields the same results, and is much more efficient (especially as you add more devices).
This is a prerequisite for patches to resolve:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983 --- src/qemu/qemu_command.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
ACK Jan
This resolves one of the issues in: https://bugzilla.redhat.com/show_bug.cgi?id=1003983 This device is identical to qemu's "intel-hda" device (known as "ich6" in libvirt), but has a different PCI device ID (which matches the ID of the hda audio built into the ich9 chipset, of course). It's not supported int earlier versions of qemu, so it requires a capability bit. --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 16 ++++++++++++++-- tests/qemuxml2argvdata/qemuxml2argv-sound-device.args | 7 ++++++- tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml | 5 +++++ tests/qemuxml2argvtest.c | 3 ++- 9 files changed, 36 insertions(+), 6 deletions(-) diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index fb6a26c..5c5301d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -2947,6 +2947,7 @@ <value>pcspk</value> <value>ac97</value> <value>ich6</value> + <value>ich9</value> </choice> </attribute> <interleave> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 240f318..4110127 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -462,7 +462,8 @@ VIR_ENUM_IMPL(virDomainSoundModel, VIR_DOMAIN_SOUND_MODEL_LAST, "es1370", "pcspk", "ac97", - "ich6") + "ich6", + "ich9") VIR_ENUM_IMPL(virDomainMemDump, VIR_DOMAIN_MEM_DUMP_LAST, "default", @@ -8451,7 +8452,8 @@ virDomainSoundDefParseXML(const xmlNodePtr node, goto error; } - if (def->model == VIR_DOMAIN_SOUND_MODEL_ICH6) { + if (def->model == VIR_DOMAIN_SOUND_MODEL_ICH6 || + def->model == VIR_DOMAIN_SOUND_MODEL_ICH9) { int ncodecs; xmlNodePtr *codecNodes = NULL; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5c33e08..f20a916 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1253,6 +1253,7 @@ enum virDomainSoundModel { VIR_DOMAIN_SOUND_MODEL_PCSPK, VIR_DOMAIN_SOUND_MODEL_AC97, VIR_DOMAIN_SOUND_MODEL_ICH6, + VIR_DOMAIN_SOUND_MODEL_ICH9, VIR_DOMAIN_SOUND_MODEL_LAST }; diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index d830e2a..dc8f0be 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -241,6 +241,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "usb-storage", /* 155 */ "usb-storage.removable", "virtio-mmio", + "ich9-intel-hda", ); struct _virQEMUCaps { @@ -1391,6 +1392,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "i82801b11-bridge", QEMU_CAPS_DEVICE_DMI_TO_PCI_BRIDGE }, { "usb-storage", QEMU_CAPS_DEVICE_USB_STORAGE }, { "virtio-mmio", QEMU_CAPS_DEVICE_VIRTIO_MMIO }, + { "ich9-intel-hda", QEMU_CAPS_DEVICE_ICH9_INTEL_HDA }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBlk[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index f3c8fa8..f8dc728 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -196,6 +196,7 @@ enum virQEMUCapsFlags { QEMU_CAPS_DEVICE_USB_STORAGE = 155, /* -device usb-storage */ QEMU_CAPS_USB_STORAGE_REMOVABLE = 156, /* usb-storage.removable */ QEMU_CAPS_DEVICE_VIRTIO_MMIO = 157, /* -device virtio-mmio */ + QEMU_CAPS_DEVICE_ICH9_INTEL_HDA = 158, /* -device ich9-intel-hda */ QEMU_CAPS_LAST, /* this must always be the last item */ }; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 3156cff..c8a5c8b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1800,6 +1800,7 @@ qemuCollectPCIAddress(virDomainDefPtr def ATTRIBUTE_UNUSED, case VIR_DOMAIN_DEVICE_SOUND: switch (device->data.sound->model) { case VIR_DOMAIN_SOUND_MODEL_ICH6: + case VIR_DOMAIN_SOUND_MODEL_ICH9: flags = (QEMU_PCI_CONNECT_TYPE_PCI | QEMU_PCI_CONNECT_TYPE_EITHER_IF_CONFIG); break; @@ -5280,6 +5281,15 @@ qemuBuildSoundDevStr(virDomainDefPtr def, case VIR_DOMAIN_SOUND_MODEL_ICH6: model = "intel-hda"; break; + case VIR_DOMAIN_SOUND_MODEL_ICH9: + model = "ich9-intel-hda"; + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_ICH9_INTEL_HDA)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("The ich9-intel-hda audio controller " + "is not supported in this QEMU binary")); + goto error; + } + break; } virBufferAsprintf(&buf, "%s,id=%s", model, sound->info.alias); @@ -9065,7 +9075,8 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, str); - if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6) { + if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6 || + sound->model == VIR_DOMAIN_SOUND_MODEL_ICH9) { char *codecstr = NULL; for (j = 0; j < sound->ncodecs; j++) { @@ -9111,7 +9122,8 @@ qemuBuildCommandLine(virConnectPtr conn, goto error; } - if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6) { + if (sound->model == VIR_DOMAIN_SOUND_MODEL_ICH6 || + sound->model == VIR_DOMAIN_SOUND_MODEL_ICH9) { VIR_FREE(modstr); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("this QEMU binary lacks hda support")); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args index d358453..1511389 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.args @@ -9,4 +9,9 @@ id=sound4-codec0,bus=sound4.0,cad=0 \ -device intel-hda,id=sound5,bus=pci.0,addr=0x6 \ -device hda-micro,id=sound5-codec0,bus=sound5.0,cad=0 \ -device hda-duplex,id=sound5-codec1,bus=sound5.0,cad=1 \ --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x7 +-device ich9-intel-hda,id=sound6,bus=pci.0,addr=0x7 -device hda-duplex,\ +id=sound6-codec0,bus=sound6.0,cad=0 \ +-device ich9-intel-hda,id=sound7,bus=pci.0,addr=0x8 \ +-device hda-micro,id=sound7-codec0,bus=sound7.0,cad=0 \ +-device hda-duplex,id=sound7-codec1,bus=sound7.0,cad=1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x9 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml index 7bf9ff9..8ce718e 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml @@ -31,6 +31,11 @@ <codec type='micro'/> <codec type='duplex'/> </sound> + <sound model='ich9'/> + <sound model='ich9'> + <codec type='micro'/> + <codec type='duplex'/> + </sound> <memballoon model='virtio'/> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index ec4a6e5..0b808a4 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -855,7 +855,8 @@ mymain(void) DO_TEST("sound", NONE); DO_TEST("sound-device", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, - QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_HDA_MICRO); + QEMU_CAPS_HDA_DUPLEX, QEMU_CAPS_HDA_MICRO, + QEMU_CAPS_DEVICE_ICH9_INTEL_HDA); DO_TEST("fs9p", QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_FSDEV, QEMU_CAPS_FSDEV_WRITEOUT); -- 1.8.3.1
On 09/25/2013 02:30 PM, Laine Stump wrote:
This resolves one of the issues in:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983
This device is identical to qemu's "intel-hda" device (known as "ich6" in libvirt), but has a different PCI device ID (which matches the ID of the hda audio built into the ich9 chipset, of course). It's not supported int earlier versions of qemu, so it requires a capability
*in
bit. --- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 6 ++++-- src/conf/domain_conf.h | 1 + src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 16 ++++++++++++++-- tests/qemuxml2argvdata/qemuxml2argv-sound-device.args | 7 ++++++- tests/qemuxml2argvdata/qemuxml2argv-sound-device.xml | 5 +++++ tests/qemuxml2argvtest.c | 3 ++- 9 files changed, 36 insertions(+), 6 deletions(-)
ACK Jan
This will make it simpler to add checks for other types of controllers. This is a prerequisite for patches to resolve: https://bugzilla.redhat.com/show_bug.cgi?id=1003983 --- src/qemu/qemu_command.c | 41 +++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c8a5c8b..9baed56 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2534,27 +2534,32 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, char *addrStr = NULL; qemuDomainPCIConnectFlags flags = QEMU_PCI_CONNECT_TYPE_PCIE; - /* Verify that the first SATA controller is at 00:1F.2 */ - /* the q35 machine type *always* has a SATA controller at this address */ for (i = 0; i < def->ncontrollers; i++) { - if (def->controllers[i]->type == VIR_DOMAIN_CONTROLLER_TYPE_SATA && - def->controllers[i]->idx == 0) { - if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { - if (def->controllers[i]->info.addr.pci.domain != 0 || - def->controllers[i]->info.addr.pci.bus != 0 || - def->controllers[i]->info.addr.pci.slot != 0x1F || - def->controllers[i]->info.addr.pci.function != 2) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("Primary SATA controller must have PCI address 0:0:1f.2")); - goto cleanup; + switch (def->controllers[i]->type) { + case VIR_DOMAIN_CONTROLLER_TYPE_SATA: + /* Verify that the first SATA controller is at 00:1F.2 the + * q35 machine type *always* has a SATA controller at this + * address. + */ + if (def->controllers[i]->idx == 0) { + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { + if (def->controllers[i]->info.addr.pci.domain != 0 || + def->controllers[i]->info.addr.pci.bus != 0 || + def->controllers[i]->info.addr.pci.slot != 0x1F || + def->controllers[i]->info.addr.pci.function != 2) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Primary SATA controller must have PCI address 0:0:1f.2")); + goto cleanup; + } + } else { + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = 0x1F; + def->controllers[i]->info.addr.pci.function = 2; } - } else { - def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; - def->controllers[i]->info.addr.pci.domain = 0; - def->controllers[i]->info.addr.pci.bus = 0; - def->controllers[i]->info.addr.pci.slot = 0x1F; - def->controllers[i]->info.addr.pci.function = 2; } + break; } } -- 1.8.3.1
On 09/25/2013 02:30 PM, Laine Stump wrote:
This will make it simpler to add checks for other types of controllers.
This is a prerequisite for patches to resolve:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983 --- src/qemu/qemu_command.c | 41 +++++++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 18 deletions(-)
ACK Jan
This resolves one of the issues listed in: https://bugzilla.redhat.com/show_bug.cgi?id=1003983 00:1E.0 is the location of this controller on at least some actual Q35 hardware, so we try to replicate the placement. The bridge should work just as well in any other location though, so if 00:1E.0 isn't available, just allow it to be auto-assigned anywhere appropriate. --- src/qemu/qemu_command.c | 22 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- .../qemuxml2argv-pcihole64-q35.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 9baed56..38dd451 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -2560,6 +2560,28 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, } } break; + + case VIR_DOMAIN_CONTROLLER_TYPE_PCI: + if (def->controllers[i]->model == VIR_DOMAIN_CONTROLLER_MODEL_DMI_TO_PCI_BRIDGE && + def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) { + /* Try to assign this bridge to 00:1E.0 (because that + * is its standard location on real hardware) unless + * it's already taken, but don't insist on it. + */ + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 0x1E; + if (!qemuDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + if (qemuDomainPCIAddressReserveAddr(addrs, &tmp_addr, + flags, true, false) < 0) + goto cleanup; + def->controllers[i]->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + def->controllers[i]->info.addr.pci.domain = 0; + def->controllers[i]->info.addr.pci.bus = 0; + def->controllers[i]->info.addr.pci.slot = 0x1E; + def->controllers[i]->info.addr.pci.function = 0; + } + } + break; } } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args index 010e089..48c21cd 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args @@ -2,5 +2,5 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/libexec/qemu-kvm \ -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args index 175c0dc..6855cd2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-pcihole64-q35.args @@ -2,7 +2,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi \ -boot c -global q35-pcihost.pci-hole64-size=1048576K \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-q35.args b/tests/qemuxml2argvdata/qemuxml2argv-q35.args index 6bd5f4c..8cc5874 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-q35.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-q35.args @@ -1,7 +1,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ /usr/libexec/qemu-kvm -S -M q35 -m 2048 -smp 2 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ --device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x2 \ +-device i82801b11-bridge,id=pci.1,bus=pcie.0,addr=0x1e \ -device pci-bridge,chassis_nr=2,id=pci.2,bus=pci.1,addr=0x1 \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-sata0-0-0 \ -device ide-drive,bus=ide.0,drive=drive-sata0-0-0,id=sata0-0-0 \ -- 1.8.3.1
On 09/25/2013 02:30 PM, Laine Stump wrote:
This resolves one of the issues listed in:
https://bugzilla.redhat.com/show_bug.cgi?id=1003983
00:1E.0 is the location of this controller on at least some actual Q35 hardware, so we try to replicate the placement. The bridge should work just as well in any other location though, so if 00:1E.0 isn't available, just allow it to be auto-assigned anywhere appropriate. --- src/qemu/qemu_command.c | 22 ++++++++++++++++++++++ tests/qemuxml2argvdata/qemuxml2argv-pcie-root.args | 2 +- .../qemuxml2argv-pcihole64-q35.args | 2 +- tests/qemuxml2argvdata/qemuxml2argv-q35.args | 2 +- 4 files changed, 25 insertions(+), 3 deletions(-)
ACK Jan
On 09/25/2013 08:30 AM, Laine Stump wrote:
These patches are all in response to the issues that Gerd Hoffman raised in
https://bugzilla.redhat.com/show_bug.cgi?id=1003983
Three of the six are simple refactoring of existing code, done separately so that the patches that change functionality would be simpler to review. None of them is very long.
Laine Stump (6): qemu: eliminate redundant if clauses in qemuCollectPCIAddress qemu: allow some PCI devices to be attached to PCIe slots qemu: replace multiple strcmps with a switch on an enum qemu: support ich9-intel-hda audio device qemu: turn if into switch in qemuDomainValidateDevicePCISlotsQ35 qemu: prefer to put a Q35 machine's dmi-to-pci-bridge at 00:1E.0
Thanks for the reviews. I've pushed the series.
participants (2)
-
Ján Tomko -
Laine Stump