8:47 a.m.
This capability was not handled correctly, causing aarch64
guests having
<interface type='network'>
...
<rom bar='off'/>
</interface>
to fail at startup with
error: unsupported configuration: rombar and romfile not
supported in this QEMU binary
I'm not sure whether we should drop the QEMU_CAPS_PCI_ROMBAR
capability entirely now that it's enabled unconditionally and
there is no code checking for it; as I understand it, we don't
ever drop capabilities, hence the current implementation.
Andrea Bolognani (2):
qemu: Enable QEMU_CAPS_PCI_ROMBAR unconditionally
qemu: Stop checking for QEMU_CAPS_PCI_ROMBAR
src/qemu/qemu_capabilities.c | 46 ++++++++++++----------
src/qemu/qemu_command.c | 12 ++----
tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml | 1 +
6 files changed, 33 insertions(+), 29 deletions(-)
--
2.5.5
8:47 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Enable QEMU_CAPS_PCI_ROMBAR unconditionally
We already do this when parsing the help string (QEMU versions
between 0.12.0 and 1.2.0), but not when using QMP.
Stop checking the QMP data, and enable it unconditionally even
when using QMP instead.
The QMP check was never accurate anyway, since it was based on
the availability of the 'rombar' property for the {kvm-}pci-assign
device, ignoring the fact that the same property exists and can be
used for devices such as virtio-net-pci.
Since a few other capabilities are enabled based on a version
check, but the version in question is older than 0.12.0, move
those to a new virQEMUCapsInitHelpBasic() function and get rid
of the version checks altogether.
---
src/qemu/qemu_capabilities.c | 46 ++++++++++++----------
tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml | 1 +
5 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 1bddf43..a6cce30 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1110,6 +1110,29 @@ virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache)
}
+/* Capabilities that we assume are always enabled
+ * for QEMU >= 0.12.0 */
+static void
+virQEMUCapsInitHelpBasic(virQEMUCapsPtr qemuCaps)
+{
+
+ /* Although very new versions of qemu advertise the presence of
+ * the rombar option in the output of "qemu -device pci-assign,?",
+ * this advertisement was added to the code long after the option
+ * itself. According to qemu developers, though, rombar is
+ * available in all qemu binaries from release 0.12 onward.
+ * Setting the capability this way makes it available in more
+ * cases where it might be needed, and shouldn't cause any false
+ * positives (in the case that it did, qemu would produce an error
+ * log and refuse to start, so it would be immediately obvious).
+ */
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_ROMBAR);
+
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO);
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_HOST);
+}
+
+
static int
virQEMUCapsComputeCmdFlags(const char *help,
unsigned int version,
@@ -1257,9 +1280,6 @@ virQEMUCapsComputeCmdFlags(const char *help,
if (strstr(help, "-machine"))
virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACHINE_OPT);
- if (version >= 11000)
- virQEMUCapsSet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO);
-
/* While JSON mode was available in 0.12.0, it was too
* incomplete to contemplate using. The 0.13.0 release
* is good enough to use, even though it lacks one or
@@ -1301,22 +1321,6 @@ virQEMUCapsComputeCmdFlags(const char *help,
if (version >= 13000)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_MULTIFUNCTION);
- /* Although very new versions of qemu advertise the presence of
- * the rombar option in the output of "qemu -device pci-assign,?",
- * this advertisement was added to the code long after the option
- * itself. According to qemu developers, though, rombar is
- * available in all qemu binaries from release 0.12 onward.
- * Setting the capability this way makes it available in more
- * cases where it might be needed, and shouldn't cause any false
- * positives (in the case that it did, qemu would produce an error
- * log and refuse to start, so it would be immediately obvious).
- */
- if (version >= 12000)
- virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_ROMBAR);
-
- if (version >= 11000)
- virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_HOST);
-
if (version >= 1001000) {
virQEMUCapsSet(qemuCaps, QEMU_CAPS_IPV6_MIGRATION);
virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY);
@@ -1436,6 +1440,8 @@ int virQEMUCapsParseHelpStr(const char *qemu,
goto cleanup;
}
+ virQEMUCapsInitHelpBasic(qemuCaps);
+
if (virQEMUCapsComputeCmdFlags(help, *version,
qemuCaps, check_yajl) < 0)
goto cleanup;
@@ -1612,7 +1618,6 @@ static struct virQEMUCapsStringFlags
virQEMUCapsObjectPropsVirtioSCSI[] = {
};
static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsPCIAssign[] = {
- { "rombar", QEMU_CAPS_PCI_ROMBAR },
{ "configfd", QEMU_CAPS_PCI_CONFIGFD },
{ "bootindex", QEMU_CAPS_PCI_BOOTINDEX },
};
@@ -3415,6 +3420,7 @@ virQEMUCapsInitQMPBasic(virQEMUCapsPtr qemuCaps)
virQEMUCapsSet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE);
virQEMUCapsSet(qemuCaps, QEMU_CAPS_VNC_SHARE_POLICY);
virQEMUCapsSet(qemuCaps, QEMU_CAPS_HOST_PCI_MULTIDOMAIN);
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_ROMBAR);
}
/* Capabilities that are architecture depending
diff --git a/tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml
b/tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml
index 257a123..715bc5a 100644
--- a/tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml
+++ b/tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml
@@ -49,6 +49,7 @@
<flag name='usb-hub'/>
<flag name='no-shutdown'/>
<flag name='cache-unsafe'/>
+ <flag name='rombar'/>
<flag name='ich9-ahci'/>
<flag name='no-acpi'/>
<flag name='fsdev-readonly'/>
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml
index f4e6db1..37670a5 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml
@@ -43,6 +43,7 @@
<flag name='usb-hub'/>
<flag name='no-shutdown'/>
<flag name='cache-unsafe'/>
+ <flag name='rombar'/>
<flag name='ich9-ahci'/>
<flag name='fsdev-readonly'/>
<flag name='virtio-blk-pci.scsi'/>
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml
b/tests/qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml
index 96a02a5..59af475 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml
@@ -41,6 +41,7 @@
<flag name='usb-hub'/>
<flag name='no-shutdown'/>
<flag name='cache-unsafe'/>
+ <flag name='rombar'/>
<flag name='ich9-ahci'/>
<flag name='fsdev-readonly'/>
<flag name='virtio-blk-pci.scsi'/>
diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml
b/tests/qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml
index f4d60d1..8823363 100644
--- a/tests/qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml
+++ b/tests/qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml
@@ -43,6 +43,7 @@
<flag name='usb-hub'/>
<flag name='no-shutdown'/>
<flag name='cache-unsafe'/>
+ <flag name='rombar'/>
<flag name='ich9-ahci'/>
<flag name='fsdev-readonly'/>
<flag name='virtio-blk-pci.scsi'/>
--
2.5.5
11 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Enable QEMU_CAPS_PCI_ROMBAR unconditionally
On 05/13/2016 09:47 AM, Andrea Bolognani wrote:
We already do this when parsing the help string (QEMU versions
between 0.12.0 and 1.2.0), but not when using QMP.
Stop checking the QMP data, and enable it unconditionally even
when using QMP instead.
The QMP check was never accurate anyway, since it was based on
the availability of the 'rombar' property for the {kvm-}pci-assign
device, ignoring the fact that the same property exists and can be
used for devices such as virtio-net-pci.
Since a few other capabilities are enabled based on a version
check, but the version in question is older than 0.12.0, move
those to a new virQEMUCapsInitHelpBasic() function and get rid
of the version checks altogether.
---
ACK.
12:51 p.m.
New subject: [libvirt] [PATCH 1/2] qemu: Enable QEMU_CAPS_PCI_ROMBAR unconditionally
On 05/13/2016 09:47 AM, Andrea Bolognani wrote:
We already do this when parsing the help string (QEMU versions
between 0.12.0 and 1.2.0), but not when using QMP.
Stop checking the QMP data, and enable it unconditionally even
when using QMP instead.
The QMP check was never accurate anyway, since it was based on
the availability of the 'rombar' property for the {kvm-}pci-assign
device, ignoring the fact that the same property exists and can be
used for devices such as virtio-net-pci.
Since a few other capabilities are enabled based on a version
check, but the version in question is older than 0.12.0, move
those to a new virQEMUCapsInitHelpBasic() function and get rid
of the version checks altogether.
---
src/qemu/qemu_capabilities.c | 46 ++++++++++++----------
tests/qemucapabilitiesdata/caps_1.2.2-1.x86_64.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-1.aarch64.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-1.ppc64le.xml | 1 +
.../qemucapabilitiesdata/caps_2.6.0-2.aarch64.xml | 1 +
5 files changed, 30 insertions(+), 20 deletions(-)
diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
index 1bddf43..a6cce30 100644
--- a/src/qemu/qemu_capabilities.c
+++ b/src/qemu/qemu_capabilities.c
@@ -1110,6 +1110,29 @@ virCapsPtr virQEMUCapsInit(virQEMUCapsCachePtr cache)
}
+/* Capabilities that we assume are always enabled
+ * for QEMU >= 0.12.0 */
+static void
+virQEMUCapsInitHelpBasic(virQEMUCapsPtr qemuCaps)
+{
+
+ /* Although very new versions of qemu advertise the presence of
+ * the rombar option in the output of "qemu -device pci-assign,?",
+ * this advertisement was added to the code long after the option
+ * itself. According to qemu developers, though, rombar is
+ * available in all qemu binaries from release 0.12 onward.
+ * Setting the capability this way makes it available in more
+ * cases where it might be needed, and shouldn't cause any false
+ * positives (in the case that it did, qemu would produce an error
+ * log and refuse to start, so it would be immediately obvious).
+ */
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_ROMBAR);
+
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO);
+ virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_HOST);
+}
As Pavel said, the approach we've taken for other flags is to rename them to
X_$NAME, and remove all code usages. Moving these flags to their own function
confuses that approach, so I suggest either give the X_$NAME treatment to each
of those flags, or just using this series to disable PCI_ROMBAR and leave the
other flags for a later series.
(FWIW there's a list of features needing similar treatment on the
BiteSizedTasks page:
http://wiki.libvirt.org/page/BiteSizedTasks#Remove_unused_QEMU_feature_flags )
- Cole
2:23 a.m.
New subject: [libvirt] [PATCH 1/2] qemu: Enable QEMU_CAPS_PCI_ROMBAR unconditionally
On Fri, 2016-05-13 at 13:51 -0400, Cole Robinson wrote:
>
> +/* Capabilities that we assume are always enabled
> + * for QEMU >= 0.12.0 */
> +static void
> +virQEMUCapsInitHelpBasic(virQEMUCapsPtr qemuCaps)
> +{
> +
> + /* Although very new versions of qemu advertise the presence of
> + * the rombar option in the output of "qemu -device pci-assign,?",
> + * this advertisement was added to the code long after the option
> + * itself. According to qemu developers, though, rombar is
> + * available in all qemu binaries from release 0.12 onward.
> + * Setting the capability this way makes it available in more
> + * cases where it might be needed, and shouldn't cause any false
> + * positives (in the case that it did, qemu would produce an error
> + * log and refuse to start, so it would be immediately obvious).
> + */
> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_PCI_ROMBAR);
> +
> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO);
> + virQEMUCapsSet(qemuCaps, QEMU_CAPS_CPU_HOST);
> +}
As Pavel said, the approach we've taken for other flags is to rename them to
X_$NAME, and remove all code usages. Moving these flags to their own function
confuses that approach, so I suggest either give the X_$NAME treatment to each
of those flags, or just using this series to disable PCI_ROMBAR and leave the
other flags for a later series.
I hadn't noticed that pattern, and it makes much more sense
than what I'm doing here - setting a capability unconditionally
and never checking for its presence seemed kinda off, glad you
guys caught it and suggested a better solution before I had a
chance to push :)
--
Andrea Bolognani
Software Engineer - Virtualization Team
8:47 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Stop checking for QEMU_CAPS_PCI_ROMBAR
This capability is enabled unconditionally these days, so we can
get rid of the check and carry one less translatable string.
---
src/qemu/qemu_command.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 0d6d5f8..e48e722 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -493,8 +493,7 @@ qemuBuildDeviceAddressStr(virBufferPtr buf,
static int
qemuBuildRomStr(virBufferPtr buf,
- virDomainDeviceInfoPtr info,
- virQEMUCapsPtr qemuCaps)
+ virDomainDeviceInfoPtr info)
{
if (info->rombar || info->romfile) {
if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) {
@@ -502,11 +501,6 @@ qemuBuildRomStr(virBufferPtr buf,
"%s", _("rombar and romfile are supported only
for PCI devices"));
return -1;
}
- if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PCI_ROMBAR)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s", _("rombar and romfile not supported in
this QEMU binary"));
- return -1;
- }
switch (info->rombar) {
case VIR_TRISTATE_SWITCH_OFF:
@@ -3373,7 +3367,7 @@ qemuBuildNicDevStr(virDomainDefPtr def,
virMacAddrFormat(&net->mac, macaddr));
if (qemuBuildDeviceAddressStr(&buf, def, &net->info, qemuCaps) < 0)
goto error;
- if (qemuBuildRomStr(&buf, &net->info, qemuCaps) < 0)
+ if (qemuBuildRomStr(&buf, &net->info) < 0)
goto error;
if (bootindex && virQEMUCapsGet(qemuCaps, QEMU_CAPS_BOOTINDEX))
virBufferAsprintf(&buf, ",bootindex=%u", bootindex);
@@ -4442,7 +4436,7 @@ qemuBuildPCIHostdevDevStr(const virDomainDef *def,
virBufferAsprintf(&buf, ",bootindex=%u", bootIndex);
if (qemuBuildDeviceAddressStr(&buf, def, dev->info, qemuCaps) < 0)
goto error;
- if (qemuBuildRomStr(&buf, dev->info, qemuCaps) < 0)
+ if (qemuBuildRomStr(&buf, dev->info) < 0)
goto error;
if (virBufferCheckError(&buf) < 0)
--
2.5.5
11:02 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Stop checking for QEMU_CAPS_PCI_ROMBAR
On 05/13/2016 09:47 AM, Andrea Bolognani wrote:
This capability is enabled unconditionally these days, so we can
get rid of the check and carry one less translatable string.
---
Considering the two patches together in isolation, it seems like an
exercise in.... nothing. But I understand the reason for keep the flag
around (e.g. when migrating back to an older libvirt), and also why we
no longer need to check it.
ACK.
12:13 p.m.
On Fri, May 13, 2016 at 03:47:57PM +0200, Andrea Bolognani wrote:
This capability was not handled correctly, causing aarch64
guests having
<interface type='network'>
...
<rom bar='off'/>
</interface>
to fail at startup with
error: unsupported configuration: rombar and romfile not
supported in this QEMU binary
I'm not sure whether we should drop the QEMU_CAPS_PCI_ROMBAR
capability entirely now that it's enabled unconditionally and
there is no code checking for it; as I understand it, we don't
ever drop capabilities, hence the current implementation.
We cannot drop capabilities, but you can rename that capability to
X_QEMU_CAPS_PCI_ROMBAR and don't even set that capability. Check the
src/qemu/qemu_capabilities.h, there is a lot of those X_* capabilities. Even
though you have already ACKs for this series, I would suggest to send a v2 and
use X_QEMU_CAPS_PCI_ROMBAR.
Pavel
3142
days inactive
3145
days old
7 comments
4 participants
participants (4)
-
Andrea Bolognani -
Cole Robinson -
Laine Stump -
Pavel Hrdina