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@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org