[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 @@
          &lt;address bus='0x06' slot='0x02' function='0x0'/&gt;
        &lt;/source&gt;
        &lt;boot order='1'/&gt;
+&lt;rom bar='0'/&gt;

s/0/off/

      &lt;/hostdev&gt;
    &lt;/devices&gt;
    ...</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 redhat com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]