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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list