Re: [libvirt] [PATCH v3 18/18] [RFC] qemu: try to put ich9 sound device at 00:1B.0

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.

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.

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.

2) convince qemu to add a PCI Express sound device (I'm not sure which     one would be most appropriate).

---   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;

+            if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0) +                goto cleanup;

+            sound->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI; +            sound->info.addr.pci = tmp_addr; +            break; +        } +    } +       ret = 0;    cleanup:       VIR_FREE(addrStr);