On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
Real Q35 hardware has an ICH9 chip that includes several integrated devices at particular addresses (see the file docs/q35-chipset.cfg in the qemu source). libvirt already attempts to put the first two sets of ich9 USB2 controllers it finds at 00:1D.* and 00:1A.* to match the real hardware. This patch does the same for the ich9 "HD audio" device. The reason I made this patch is that currently the *only* device in a reasonable "workstation" type virtual machine config that requires a legacy PCI slot is the audio device, Without this patch, the standard Q35 machine created by virt-manager will have a dmi-to-pci-bridge and a pci-bridge just for the sound device; with the patch (and if you change the sound device model from the default "ich6" to "ich9"), the machine definition constructed by virt-manager has absolutely no legacy PCI controllers - any legacy PCI devices (e.g. video and sound) are on pcie-root as integrated devices.
Everything past this point, while valuable for the discussion, should IMHO be left out of the commit message.
As cool as it is to have virt-manager making a legacy-PCI-free config so easily, I'm undecided whether or not this is a worthwhile patch. On one hand, it's following an existing convention of trying to place devices that are known to be integrated chipset devices on Q35 hardware at the same address they appear in real life (but doesn't insist on this address, and doesn't add any new device if one isn't already present in the config). On the other hand it creates yet another exception to "follow the same formula for everything", while it's probably better for us to be trying to *get away* from that.
I'm for merging this: it's straighforward enough, and if we ever decide to start moving stuff off pcie.0 we can just get rid of the code you're adding without affecting existing guests at all.
Two alternate solutions: 1) convince virt-manager to use "ich9" as the default sound for Q35, and explicitly place it at 00:1B.0 in the definition it sends to libvirt.
After this has been merged, virt-manager will still want to change its default to ich9 (based on information retrieved from libosinfo, I assume), but it will not need to hardcode this kind of knowledge, which sounds perfectly fine to me.
2) convince qemu to add a PCI Express sound device (I'm not sure which one would be most appropriate).
That would probably be nice to have, but I'd rather have generic PCI/PCIe controllers than a PCIe sound card, if any QEMU developer is reading this ;)
--- src/qemu/qemu_domain_address.c | 25 +++++ .../qemuxml2argv-q35-virt-manager-basic.args | 56 ++++++++++ .../qemuxml2argv-q35-virt-manager-basic.xml | 76 ++++++++++++++ tests/qemuxml2argvtest.c | 31 ++++++ .../qemuxml2xmlout-q35-virt-manager-basic.xml | 116 +++++++++++++++++++++ tests/qemuxml2xmltest.c | 23 ++++ 6 files changed, 327 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-q35-virt-manager-basic.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-q35-virt-manager-basic.xml diff --git a/src/qemu/qemu_domain_address.c b/src/qemu/qemu_domain_address.c index a9c4c32..6cfd710 100644 --- a/src/qemu/qemu_domain_address.c +++ b/src/qemu/qemu_domain_address.c @@ -1218,6 +1218,31 @@ qemuDomainValidateDevicePCISlotsQ35(virDomainDefPtr def, goto cleanup; } } + + memset(&tmp_addr, 0, sizeof(tmp_addr)); + tmp_addr.slot = 0x1B; + if (!virDomainPCIAddressSlotInUse(addrs, &tmp_addr)) { + /* Since real Q35 hardware has an ICH9 chip that has an + * integrated HD audio device at 0000:00:1B.0 put any + * unaddressed ICH9 audio device at that address if it's not + * already taken. If there's something already there, let the + * normal device addressing assign something later. + */ + for (i = 0; i < def->nsounds; i++) { + virDomainSoundDefPtr sound = def->sounds[i]; + + if (sound->model != VIR_DOMAIN_SOUND_MODEL_ICH9) + continue; + if (!virDeviceInfoPCIAddressWanted(&sound->info)) + break;
Mh, why break instead of continue here? I'm assuming people won't have more than one ich9 sound card per guest, but if they did and one of them already had a PCI address assigned (which is not 0000:00:1B.0 because of the outer check) why wouldn't we want to try assigning the next one to 0000:00:1B.0?
+ if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) + goto cleanup;
Add an empty line here, please.
+ sound->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; + sound->info.addr.pci = tmp_addr; + break; + } + } + ret = 0; cleanup: VIR_FREE(addrStr);
ACK -- Andrea Bolognani / Red Hat / Virtualization