[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
s/devices/device's/
> + map. (in PCI documentation, this is described as being
s/in/In/
> + 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
<rom/>
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