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

[Daniel Veillard]

On Fri, Sep 30, 2011 at 01:40:46AM -0400, Laine Stump wrote:
> When support for was added for PCI multifunction cards (in commit
> 9f8baf, first included in libvirt 0.9.3), it was done by always
> turning on the multifunction bit for all PCI devices. Since that time
> it has been realized that this is not an ideal solution, and that the
> multifunction bit must be selectively turned on. For example, see
> 
>   https://bugzilla.redhat.com/show_bug.cgi?id=728174
> 
> and the discussion before and after
> 
>   https://www.redhat.com/archives/libvir-list/2011-September/msg01036.html
> 
> This patch modifies multifunction support so that the multifunction=on
> option is only added to the qemu commandline for a device if its PCI
> <address> definition has the attribute "multifunction='on'", e.g.:
> 
>   <address type='pci' domain='0x0000' bus='0x00'
>            slot='0x04' function='0x0' multifunction='on'/>
[...]
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 4972fac..cffaac2 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2118,6 +2118,14 @@
>      <attribute name="function">
>        <ref name="pciFunc"/>
>      </attribute>
> +    <optional>
> +      <attribute name="multifunction">
> +        <choice>
> +          <value>on</value>
> +          <value>off</value>
> +        </choice>
> +      </attribute>
> +    </optional>
>    </define>
>    <define name="driveaddress">
>      <optional>

 okay

[...]
> +VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti,
> +              VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST,
> +              "default",
> +              "on",
> +              "off")
> +
>  VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST,
>                "block",
>                "file",
> @@ -1652,6 +1658,10 @@ virDomainDeviceInfoFormat(virBufferPtr buf,
>                            info->addr.pci.bus,
>                            info->addr.pci.slot,
>                            info->addr.pci.function);
[...]
>  
> +    if (multi &&
> +        ((addr->multi = virDomainDeviceAddressPciMultiTypeFromString(multi)) < 0)) {
> +        virDomainReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                             _("Unknown value '%s' for <address> 'multifunction' attribute"),
> +                             multi);
> +        goto cleanup;
> +
> +    }

  This code allows mutifunction="default" input

  if you make the test <= 0 then it should reject "default" as
  expected...

>      if (!virDomainDevicePCIAddressIsValid(addr)) {
>          virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                               _("Insufficient specification for PCI address"));
[...]
> @@ -1391,11 +1397,11 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
>              virBufferAsprintf(buf, ",bus=pci.0");
>          else
>              virBufferAsprintf(buf, ",bus=pci");
> -        if (qemuCapsGet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION))
> -            virBufferAsprintf(buf, ",multifunction=on,addr=0x%x.0x%x",
> -                              info->addr.pci.slot, info->addr.pci.function);
> -        else
> -            virBufferAsprintf(buf, ",addr=0x%x", info->addr.pci.slot);
> +        if (info->addr.pci.multi == VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_ON)
> +            virBufferAddLit(buf, ",multifunction=on");


  Hum seems to me that if the users explicitely specified mutifunction="off"
then that ought to be saved, and hence
       else if (info->addr.pci.multi == VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_OFF)
           virBufferAddLit(buf, ",multifunction=off");

need to be added (since it's not the default which is 0 that won't
pollute XML where it's not specified).

> +        virBufferAsprintf(buf, ",addr=0x%x", info->addr.pci.slot);
> +        if (info->addr.pci.function != 0)
> +           virBufferAsprintf(buf, ".0x%x", info->addr.pci.function);
>      } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB) {
>          virBufferAsprintf(buf, ",bus=");
>          qemuUsbId(buf, info->addr.usb.bus);

  ACK with those 2 problems fixed

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/