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

Eric Blake eblake at redhat.com
Wed Sep 21 21:01:22 UTC 2011

On 09/21/2011 10:25 AM, Laine Stump wrote:
> 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.

I'm leaning towards deferring this until after a quick 0.9.6 release 
that works around the qemu shutdown issue.  That said, I'll still review 
the patch.

> At the moment there is no clean method to determine whether or not the
> rombar option is supported by QEMU - this patch uses the advice of a
> QEMU developer to assume support for qemu-0.12+. There is currently a
> patch in the works to put this information in the output of "qemu-kvm
> -device pci-assign,?", but of course if we switch to keying off that,
> we would lose support for setting rombar on all the versions of qemu
> between 0.12 and whatever version gets that patch.

Hopefully the assumption is close enough to correct, and qemu will give 
us error messages if our request is inappropriate.

> +<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 devices ROM will be visible in the guest's memory


> +        map. (in PCI documentation, this is described as being


> +        controlled by the "rombar" setting). If no rom bar is

Maybe mention what the BAR acronym expands to (am I correct that it 
means Base Address Register?), as in:

(In PCI documentation, the "rombar" setting controls the presence of the 
Base Address Register for the ROM.)

> +        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>
> +</dd>
>         <dt><code>address</code></dt>
>         <dd>The<code>address</code>  element for USB devices has a
>         <code>bus</code>  and<code>device</code>  attribute to specify the
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index d0da41c..0ba35ef 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -2059,6 +2059,16 @@
>         <optional>
>           <ref name="address"/>
>         </optional>
> +<optional>
> +<element name="rom">
> +<attribute name="bar">

I think you need another layer of <optional> around the <attribute>; 
that is, the user can do


which should be just as valid XML as:

<rom bar="on"/>

even if we never generate it ourselves.

> +<choice>
> +<value>on</value>
> +<value>off</value>
> +</choice>
> +</attribute>

I think you need <empty/> here as well.

> @@ -5485,6 +5491,20 @@ virDomainHostdevDefParseXML(const xmlNodePtr node,
>                   if (virDomainDeviceBootParseXML(cur,&def->bootIndex,
>                                                   bootMap))
>                       goto error;
> +            } else if (xmlStrEqual(cur->name, BAD_CAST "rom")) {
> +                char *rombar = virXMLPropString(cur, "bar");
> +                if (!rombar) {
> +                    virDomainReportError(VIR_ERR_XML_ERROR,
> +                                         "%s", _("missing rom bar attribute"));
> +                    goto error;
> +                }

Oh, I guess you're making it a hard error if bar=... is not present, in 
which case my comment about another layer of <optional> can be disregarded.

> @@ -1855,6 +1864,7 @@ VIR_ENUM_DECL(virDomainWatchdogAction)
>   VIR_ENUM_DECL(virDomainVideo)
>   VIR_ENUM_DECL(virDomainHostdevMode)
>   VIR_ENUM_DECL(virDomainHostdevSubsys)
> +VIR_ENUM_DECL(virDomainPciRombarMode)
>   VIR_ENUM_DECL(virDomainHub)
>   VIR_ENUM_DECL(virDomainRedirdevBus)
>   VIR_ENUM_DECL(virDomainInput)

Export the two new symbols (virDomainPciRombarModeType{To,From}String) 
in src/libvirt_private.syms.

> @@ -1049,6 +1050,9 @@ qemuCapsComputeCmdFlags(const char *help,
>       if (version>= 13000)
>           qemuCapsSet(flags, QEMU_CAPS_PCI_MULTIFUNCTION);
> +
> +    if (version>= 12000)
> +        qemuCapsSet(flags, QEMU_CAPS_PCI_ROMBAR);

Maybe add a comment why we use a version check rather than probing for 
pci-assign.rombar=uint32 in 'qemu -device pci-assign,?'.

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

More information about the libvir-list mailing list