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

[Laine Stump]

On 10/13/2016 02:07 PM, Andrea Bolognani wrote:
> 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.

Yep. I just had it in the commit message because that's when I was 
thinking about it, and I didn't want to forget later (or even worse - 
have to *re*-type it because something went wrong and I decided to abort 
the send-email and start over).

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

Okay, that's 2.5 votes in favor and 0 against, so I guess I'll go for it.

>> 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),

I'm not sure if virt-manager is getting soundcard info from libosinfo or 
just creating it from thin air, but in the case of Q35, it probably 
doesn't apply, because I think libosinfo doesn't know about Q35; most 
likely it's using all the same info as it does for 440fx (and I don't 
think we want it to default to the ich9 audio device on 440fx, since the 
ich9 chipset was used first in the era of Q35, ie it was never put into 
the same motherboard as a 440fx chipset.

But yeah, virt-manager needs to change its default. I had asked Cole 
about it on IRC one day, but maybe I should do that in a more formal manner.

>   but it will not need to hardcode
> this kind of knowledge, which sounds perfectly fine to me.

Yeah, I agree that (1) is a bad idea. I just put it there to be thorough :-)

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

Yeah, I think I agree with that.

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

Good point. I'm changing it to continue.

>
>> +            if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
>> +                goto cleanup;
> Add an empty line here, please.

Done.

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