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

[Andrea Bolognani]

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