[libvirt] [PATCHv2] qemu: add ability to set PCI device "rombar" on or off

[Eric Blake]

On 09/22/2011 10:05 AM, Laine Stump wrote:
> (This addresses Eric's comments in his review of V1. BTW, I'm a bit
> surprised nobody has commented about the choice of name/placement of
> the new attribute - speak now or forever hold your peace :-))

I was okay with the xml naming - but I agree that if anyone else has a 
better suggestion, now is the time to speak up :)

>
> This patch was made in response to:
>
>    https://bugzilla.redhat.com/show_bug.cgi?id=738095
>
> In short, qemu's default for the rombar setting (which makes the
> firmware ROM of a PCI device visible/not on the guest) was previously
> 0 (not visible), but they recently changed the default to 1
> (visible). Unfortunately, there are some PCI devices that fail in the
> guest when rombar is 1, so the setting must be exposed in libvirt to
> prevent a regression in behavior (it will still require explicitly
> setting<rom bar='off'/>  in the guest XML).
>
> rombar is forced on/off by adding:
>
>    <rom bar='on|off'/>
>
> inside a<hostdev>  element that defines a PCI device. It is currently
> ignored for all other types of devices.
>

> +++ b/docs/formatdomain.html.in
> @@ -1371,6 +1371,7 @@
>           <address bus='0x06' slot='0x02' function='0x0'/>
>         </source>
>         <boot order='1'/>
> +<rom bar='0'/>

s/0/off/

>       </hostdev>
>     </devices>
>     ...</pre>
> @@ -1402,6 +1403,18 @@
>         used together with general boot elements in
>         <a href="#elementsOSBIOS">BIOS bootloader</a>  section.
>         <span class="since">Since 0.8.8</span></dd>
> +<dt><code>rom</code></dt>
> +<dd>The<code>rom</code>  element is used to change how a PCI
> +        device's ROM is presented to the guest. The<code>bar</code>
> +        attribute can be set to "on" or "off", and determines whether
> +        or not the device's ROM will be visible in the guest's memory
> +        map. (In PCI documentation, the "rombar" setting controls the
> +        presence of the Base Address Register for the ROM). If no rom
> +        bar is specified, the qemu default will be used (older
> +        versions of qemu used a default of "off", while newer qemus
> +        have a default of "on").<span class="since">Since
> +        0.9.6</span>

looks much nicer, now that my v1 comments are incorporated, but still 
one nit:

s/0.9.6/0.9.7/

> @@ -10387,6 +10407,18 @@ virDomainHostdevDefFormat(virBufferPtr buf,
>       if (virDomainDeviceInfoFormat(buf,&def->info, flags)<  0)
>           return -1;
>
> +    if (def->rombar) {
> +        const char *rombar
> +            = virDomainPciRombarModeTypeToString(def->rombar);
> +        if (!rombar) {
> +            virDomainReportError(VIR_ERR_INTERNAL_ERROR,
> +                                 _("unexpected rom bar value %d"),
> +                                 def->rombar);
> +            return -1;
> +        }
> +        virBufferAsprintf(buf, "<rom bar='%s'/>\n", rombar);

Aargh - Thunderbird strikes me again.  When will they EVER fix their 
horrible whitespace munging bug?

This will conflict with my <snapshotdomain> reindentation patch series - 
so whoever applies first gets the easier side of the bargain :)

> +    }
> +
>       virBufferAddLit(buf, "</hostdev>\n");
>
>       return 0;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 371f270..262c970 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -936,6 +936,14 @@ enum virDomainHostdevSubsysType {
>       VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_LAST
>   };
>
> +enum virDomainPciRombarMode {
> +    VIR_DOMAIN_PCI_ROMBAR_DEFAULT = 0,
> +    VIR_DOMAIN_PCI_ROMBAR_ON,
> +    VIR_DOMAIN_PCI_ROMBAR_OFF,
> +
> +    VIR_DOMAIN_PCI_ROMBAR_LAST
> +};
> +
>   typedef struct _virDomainHostdevDef virDomainHostdevDef;
>   typedef virDomainHostdevDef *virDomainHostdevDefPtr;
>   struct _virDomainHostdevDef {
> @@ -964,6 +972,7 @@ struct _virDomainHostdevDef {
>       } source;
>       int bootIndex;
>       virDomainDeviceInfo info; /* Guest address */
> +    int rombar;               /* rombar on/off/unspecified */

Your comment could go out of date if the enum grows. Lately, I've been 
listing fields like this as:

int rombar; /* enum virDomainPciRombarMode */

which stays correct even if we later add new values to the enum.

> +++ b/src/qemu/qemu_capabilities.h
> @@ -110,6 +110,7 @@ enum qemuCapsFlags {
>       QEMU_CAPS_PCI_OHCI          = 71, /* -device pci-ohci */
>       QEMU_CAPS_USB_REDIR         = 72, /* -device usb-redir */
>       QEMU_CAPS_USB_HUB           = 73, /* -device usb-hub */
> +    QEMU_CAPS_PCI_ROMBAR        = 74, /* -device rombar=0|1 */

Needs an obvious rebase for NO_SHUTDOWN and DRIVE_CACHE_UNSAFE.

ACK to the idea.  I think my findings are small enough that I'm okay if 
you push with nits fixed rather than posting a v3 patch, although you 
may still want to wait for any other comments on the proposed xml spelling.

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