Re: [libvirt] [PATCH v3 18/18] [RFC] qemu: try to put ich9 sound device at 00:1B.0
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 aud
Re: [libvirt] [PATCH v3 18/18] [RFC] qemu: try to put ich9 sound device at 00:1B.0
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 :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 :00:1B.0 because of the
outer check) why wouldn't we want to try assigning the next
one to :00:1B.0?
> +if (virDomainPCIAddressReserveSlot(addrs, &tmp_addr, flags) < 0)
> +goto cleanup;
Add an empty line here, please.
> +sound->info.type = VIR_DOM
Re: [libvirt] [PATCH v3 18/18] [RFC] qemu: try to put ich9 sound device at 00:1B.0
Hi, > 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. While being at it: Ethernet is in the same boat. It is a integrated device at slot 00:19.0. Some e1000 version on older chipsets, e1000e on newer ones (here my T530): nilsson root ~# lspci -vs19 00:19.0 Ethernet controller: Intel Corporation 82579LM Gigabit Network Connection (rev 04) Subsystem: Lenovo Device 21f3 Flags: bus master, fast devsel, latency 0, IRQ 27 Memory at f250 (32-bit, non-prefetchable) [size=128K] Memory at f253b000 (32-bit, non-prefetchable) [size=4K] I/O ports at 5080 [size=32] Capabilities: [c8] Power Management version 2 Capabilities: [d0] MSI: Enable+ Count=1/1 Maskable- 64bit+ Capabilities: [e0] PCI Advanced Features Kernel driver in use: e1000e Kernel modules: e1000e So we might do the same here instead of placing it in a pcie root port (patch 12/18). I think we don't emulate the ich9 e1000 variant though. > 2) convince qemu to add a PCI Express sound device (I'm not sure which >one would be most appropriate). For reference: It's still @ 00:1b.0 in recent chipsets as integrated device, it is still the same hardware (again my T530): nilsson root ~# lspci -vs1b 00:1b.0 Audio device: Intel Corporation 7 Series/C210 Series Chipset Family High Definition Audio Controller (rev 04) Subsystem: Lenovo Device 21f6 Flags: bus master, fast devsel, latency 0, IRQ 32 Memory at f253 (64-bit, non-prefetchable) [size=16K] Capabilities: [50] Power Management version 2 Capabilities: [60] MSI: Enable+ Count=1/1 Maskable- 64bit+ Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00 Capabilities: [100] Virtual Channel Capabilities: [130] Root Complex Link Kernel driver in use: snd_hda_intel Kernel modules: snd_hda_intel cheers, Gerd -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
