[libvirt] [PATCHv2 2/2] qemu: make PCI multifunction support more manual

[Eric Blake]

On 09/30/2011 12:02 PM, Laine Stump wrote:
> (V2: address Daniel Veillard's two review points (don't allow
> "multifunction='default'", and add "multifunction=off" to the qemu
> commandline when that's what the XML says), and modify the checks for
> duplicate PCI address usage attempts to account for multifunction=off
> on a device's function 0, per IRC discussion with Dan Berrange (see
> new qemuCollectPCIAddress()). As a side effect, attempts to re-use the
> same PCI address are now logged rather than silently generating an
> error.)

Yep, I can see those enhancements over v1.

>
> A side effect of this patch is that attempts to use the same PCI
> address for two different devices will now log an error (previously
> this would cause the domain define operation to fail, but there would
> be no log message generated). Because the function doing this log was
> almost completely rewritten, I didn't think it worthwhile to make a
> separate patch for that fix (the entire patch would immediately be
> obsoleted).

Agree - no easy way to split it.

> @@ -1118,10 +1118,14 @@
>           The<code>type</code>  attribute is mandatory, and is typically
>           "pci" or "drive".  For a "pci" controller, additional
>           attributes for<code>bus</code>,<code>slot</code>,
> -        and<code>function</code>  must be present, as well as an
> -        optional<code>domain</code>.  For a "drive" controller,
> -        additional attributes<code>controller</code>,<code>bus</code>,
> +        and<code>function</code>  must be present, as well as
> +        optional<code>domain</code>
> +        and<code>multifunction</code><span class="since">since
> +        0.9.7, requires QEMU 0.13</span>  (defaults to 'off').  For a

This wording makes it sound like both domain and multifunction were 
first introduced in 0.9.7, rather than the intended fact that 
'multifunction' is the only newcomer attribute.  Maybe reword along the 
lines of:

...as well as optional 'domain' and 'multifunction'.  Multifunction 
defaults to 'off', any other value requires QEMU 0.13 and <span 
class="since">libvirt 0.9.7</span>.

> +++ b/tests/qemuxml2argvdata/qemuxml2argv-usb-ich9-ehci-addr.args
> @@ -1 +1 @@
> -LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device ich9-usb-ehci1,id=usb,bus=pci.0,multifunction=on,addr=0x4.0x7 -device virtio-balloon-pci,id=balloon0,bus=pci.0,multifunction=on,addr=0x3.0x0
> +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M pc -m 214 -smp 1 -nographic -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3

These are some rather long lines.  While you are touching them, it would 
be worth splitting them with \-newline pairs to fit in more reasonable 
limits.

ACK with those nits fixed.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org