[libvirt] [PATCH v3 11/18] qemu: assign virtio devices to PCIe slot when appropriate

[Laine Stump]

On 10/06/2016 10:13 AM, Andrea Bolognani wrote:
> On Tue, 2016-09-20 at 15:14 -0400, Laine Stump wrote:
>> libvirt previously assigned nearly all devices to a "hotpluggable"
>> legacy PCI slot even on machines with a PCIe root bus (and even though
>> most such machines don't even support hotplug on legacy PCI slots!)
>> Forcing all devices onto legacy PCI slots means that the domain will
>> need a dmi-to-pci-bridge (to convert from PCIe to legacy PCI) and a
>> pci-bridge (to provide hotpluggable legacy PCI slots which, again,
>> usually aren't hotpluggable anyway).
>>   
>> To help reduce the need for these legacy controllers, this patch tries
>> to assign virtio-1.0-capable devices to PCIe slots whenever possible,
>> by setting appropriate connectFlags in
>> virDomainDeviceConnectFlagsInternal(). Happily, when that function was
>> written (just a few commits ago) it was created with a "virtioFlags"
>> argument, set by both of its callers, which is the proper connectFlags
>> to set for any virtio-*-pci device - depending on the arch/machinetype
>> of the domain, and whether or not the qemu binary supports virtio-1.0,
>> that flag will have either been set to PCI or PCIE. This patch merely
>> enables the functionality by setting the flags for the device to
>> whatever is in virtioFlags if the device is a virtio-*-pci device.
>>   
>> NB: since the slot must be hotpluggable, and pcie-root (the PCIe root
>> complex) does *not* support hotplug, this means that suitable
>> controllers must also be in the config (i.e. either pcie-root-port, or
>> pcie-downstream-port). For now, libvirt doesn't add those
>> automatically, so if you put virtio devices in a config for a qemu
>> that has PCIe-capable virtio devices, you'll need to add extra
>> pcie-root-ports yourself. That requirement will be eliminated in a
>> future patch, but for now, it's simple to do this:
>>   
>>      <controller type='pci' model='pcie-root-port'/>
>>      <controller type='pci' model='pcie-root-port'/>
>>      <controller type='pci' model='pcie-root-port'/>
>>      ...
>>   
>> Partially Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1330024
> [...]
>
>> +    <video>
>> +      <model type='virtio' heads='1' primary='yes'/>
>> +      <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x0'/>
> I was initially baffled by this, because I expected it to
> be assigned to one of the available pcie-root-ports just
> like all the other virtio devices.
>
> However, according to qemuDomainValidateDevicePCISlotsQ35()
> this is intentional, so I guess we're good :)

Actually, you bring up an interesting point in light of the "Should PCIe 
devices ever be placed directly on pcie-root?" debate on qemu-devel (I 
think it was in the thread about the PCI topology document that Marcel 
is writing). We currently always put the primary video device at 00:1 
just because "we always have", and it has the nice side effect of 
eliminating the need for legacy-PCI controllers. But in this one case 
the device is PCIe - to follow Marcel's recommendation of putting only 
legacy devices on pcie-root, we should be putting the virtio video 
device on a root-port.

As I recall, Marcel and Alex were the most vocal on this subject, so I'm 
Cc'ing them, with this bit of context - this patch auto-assigns the 
addresses for virtio devices to be on Express ports rather than legacy 
slots when appropriate, but there is a bit of Q35-specific code that 
overrides any of that and always places the primary video device at 
00:01.0 - should we still do that for the primary video if it's virtio? 
Or should we put it behind a root-port?


>
> One improvement I can recommend to this test case is to add
> one more pcie-root-port, so that it becomes quite clear
> that virtio-vga has been assigned to pcie-root on purpose
> and *not* due to a lack of pcie-root-ports.

Or possibly change the behavior for virtio video even if it's the 
primary (note to self - check if someone has done the patch to allow 
multiple virtio video devices yet)