10:42 a.m.
This series is the result of shaving many a yak while implementing
support for pSeries optional features (first attempt at [1], respin
coming right after I send this).
Quick summary of the changes:
* handle more sanely cases where the GIC capability has been
enabled but no GIC version has been provided;
* use the switch construct more, so that the compiler can warn us
when we forget to handle some feature;
* where possible (eg. the value is mandatory and there is only a
limited number of options to choose from) avoid storing whether
a feature is enabled and its actual value separately.
Save for the first one, none of the changes should be visible to
the user, but they will hopefully improve things for developers.
[1] https://www.redhat.com/archives/libvir-list/2018-January/msg00779.html
Andrea Bolognani (11):
qemu: Move feature verification from PostParse() to Validate()
qemu: Use switch in qemuDomainDefValidateFeatures()
qemu: Move GIC checks to qemuDomainDefValidateFeatures()
conf: Use switch in virDomainDefFeaturesCheckABIStability()
conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly
conf: Integrate all features ABI checks in the switch
tests: Improve GIC tests
qemu: Fix GIC behavior for the default case
conf: Improve IOAPIC feature handling
conf: Improve HPT feature handling
tests: Clean up HPT tests
src/conf/domain_conf.c | 174 +++++++++++++--------
src/conf/domain_conf.h | 15 +-
src/libvirt_private.syms | 2 +
src/qemu/qemu_command.c | 16 +-
src/qemu/qemu_domain.c | 117 +++++++++-----
.../qemuxml2argvdata/aarch64-gic-default-both.args | 1 +
.../qemuxml2argvdata/aarch64-gic-default-both.xml | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v2.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 +
...hpt-resizing.args => pseries-features-hpt.args} | 1 -
.../pseries-features-hpt.xml} | 0
...ne.xml => pseries-features-invalid-machine.xml} | 2 +-
tests/qemuxml2argvdata/pseries-hpt-resizing.xml | 19 ---
tests/qemuxml2argvtest.c | 29 ++--
.../aarch64-gic-default-both.xml | 1 +
.../qemuxml2xmloutdata/aarch64-gic-default-v2.xml | 1 +
.../qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 1 +
tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 +
tests/qemuxml2xmltest.c | 9 +-
21 files changed, 230 insertions(+), 164 deletions(-)
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.xml
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args => pseries-features-hpt.args}
(96%)
rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml =>
qemuxml2argvdata/pseries-features-hpt.xml} (100%)
rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml =>
pseries-features-invalid-machine.xml} (86%)
delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
create mode 120000 tests/qemuxml2xmloutdata/pseries-features-hpt.xml
--
2.14.3
10:42 a.m.
New subject: [libvirt] [PATCH 01/11] qemu: Move feature verification from PostParse() to Validate()
We want to perform all feature verification in a single spot, but
some of it (eg. GIC) is currently being performed at command line
generation time, and moving it to PostParse() would cause guests
to disappear. Moving verification to Validate() allows us to
side-step the issue.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 27 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index df433c2f0..27a4751e9 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3113,30 +3113,6 @@ qemuDomainDefCPUPostParse(virDomainDefPtr def)
}
-static int
-qemuDomainDefVerifyFeatures(const virDomainDef *def)
-{
- if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON &&
- !ARCH_IS_X86(def->os.arch)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("I/O APIC tuning is not supported "
- "for '%s' architecture"),
- virArchToString(def->os.arch));
- return -1;
- }
-
- if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON &&
- !qemuDomainIsPSeries(def)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s",
- _("HPT tuning is only supported for pSeries guests"));
- return -1;
- }
-
- return 0;
-}
-
-
static int
qemuDomainDefPostParseBasic(virDomainDefPtr def,
virCapsPtr caps,
@@ -3195,9 +3171,6 @@ qemuDomainDefPostParse(virDomainDefPtr def,
qemuDomainDefEnableDefaultFeatures(def, qemuCaps);
- if (qemuDomainDefVerifyFeatures(def) < 0)
- goto cleanup;
-
if (qemuDomainRecheckInternalPaths(def, cfg, parseFlags) < 0)
goto cleanup;
@@ -3250,6 +3223,30 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def)
#define QEMU_MAX_VCPUS_WITHOUT_EIM 255
+static int
+qemuDomainDefValidateFeatures(const virDomainDef *def)
+{
+ if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON &&
+ !ARCH_IS_X86(def->os.arch)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("I/O APIC tuning is not supported "
+ "for '%s' architecture"),
+ virArchToString(def->os.arch));
+ return -1;
+ }
+
+ if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON &&
+ !qemuDomainIsPSeries(def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ "%s",
+ _("HPT tuning is only supported for pSeries guests"));
+ return -1;
+ }
+
+ return 0;
+}
+
+
static int
qemuDomainDefValidate(const virDomainDef *def,
virCapsPtr caps ATTRIBUTE_UNUSED,
@@ -3362,6 +3359,9 @@ qemuDomainDefValidate(const virDomainDef *def,
}
}
+ if (qemuDomainDefValidateFeatures(def) < 0)
+ goto cleanup;
+
ret = 0;
cleanup:
--
2.14.3
11:54 a.m.
New subject: [libvirt] [PATCH 01/11] qemu: Move feature verification from PostParse() to Validate()
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
We want to perform all feature verification in a single spot, but
some of it (eg. GIC) is currently being performed at command line
generation time, and moving it to PostParse() would cause guests
to disappear. Moving verification to Validate() allows us to
side-step the issue.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++-------------------------
1 file changed, 27 insertions(+), 27 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:42 a.m.
New subject: [libvirt] [PATCH 02/11] qemu: Use switch in qemuDomainDefValidateFeatures()
The compiler can make sure we are handling all features.
While reworking the logic, also change error messages to a more
consistent style.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/libvirt_private.syms | 2 ++
src/qemu/qemu_domain.c | 57 ++++++++++++++++++++++++++++++++++++------------
2 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0bce0bbfb..17c3b71e0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -345,6 +345,8 @@ virDomainDiskSetDriver;
virDomainDiskSetFormat;
virDomainDiskSetSource;
virDomainDiskSetType;
+virDomainFeatureTypeFromString;
+virDomainFeatureTypeToString;
virDomainFSDefFree;
virDomainFSDefNew;
virDomainFSIndexByName;
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 27a4751e9..dd36b42eb 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3226,21 +3226,50 @@ qemuDomainDefGetVcpuHotplugGranularity(const virDomainDef *def)
static int
qemuDomainDefValidateFeatures(const virDomainDef *def)
{
- if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON &&
- !ARCH_IS_X86(def->os.arch)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("I/O APIC tuning is not supported "
- "for '%s' architecture"),
- virArchToString(def->os.arch));
- return -1;
- }
+ size_t i;
- if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON &&
- !qemuDomainIsPSeries(def)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- "%s",
- _("HPT tuning is only supported for pSeries guests"));
- return -1;
+ for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) {
+ const char *featureName = virDomainFeatureTypeToString(i);
+
+ switch ((virDomainFeature) i) {
+ case VIR_DOMAIN_FEATURE_IOAPIC:
+ if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ !ARCH_IS_X86(def->os.arch)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The '%s' feature is only supported for %s
guests"),
+ featureName, "x86");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_FEATURE_HPT:
+ if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ !qemuDomainIsPSeries(def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The '%s' feature is only supported for %s
guests"),
+ featureName, "pSeries");
+ return -1;
+ }
+ break;
+
+ case VIR_DOMAIN_FEATURE_ACPI:
+ case VIR_DOMAIN_FEATURE_APIC:
+ case VIR_DOMAIN_FEATURE_PAE:
+ case VIR_DOMAIN_FEATURE_HAP:
+ case VIR_DOMAIN_FEATURE_VIRIDIAN:
+ case VIR_DOMAIN_FEATURE_PRIVNET:
+ case VIR_DOMAIN_FEATURE_HYPERV:
+ case VIR_DOMAIN_FEATURE_KVM:
+ case VIR_DOMAIN_FEATURE_PVSPINLOCK:
+ case VIR_DOMAIN_FEATURE_CAPABILITIES:
+ case VIR_DOMAIN_FEATURE_PMU:
+ case VIR_DOMAIN_FEATURE_VMPORT:
+ case VIR_DOMAIN_FEATURE_GIC:
+ case VIR_DOMAIN_FEATURE_SMM:
+ case VIR_DOMAIN_FEATURE_VMCOREINFO:
+ case VIR_DOMAIN_FEATURE_LAST:
+ break;
+ }
}
return 0;
--
2.14.3
11:55 a.m.
New subject: [libvirt] [PATCH 02/11] qemu: Use switch in qemuDomainDefValidateFeatures()
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
The compiler can make sure we are handling all features.
While reworking the logic, also change error messages to a more
consistent style.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/libvirt_private.syms | 2 ++
src/qemu/qemu_domain.c | 57 ++++++++++++++++++++++++++++++++++++------------
2 files changed, 45 insertions(+), 14 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 0bce0bbfb..17c3b71e0 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -345,6 +345,8 @@ virDomainDiskSetDriver;
virDomainDiskSetFormat;
virDomainDiskSetSource;
virDomainDiskSetType;
You'll have a merge conflict right about here...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:42 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()
Keep them along with other arch/machine type checks for
features instead of waiting until command line generation
time.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 7 -------
src/qemu/qemu_domain.c | 11 ++++++++++-
tests/qemuxml2argvtest.c | 12 ++++++------
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b434a45..529079be0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
if (def->gic_version != VIR_GIC_VERSION_NONE) {
- if (!qemuDomainIsVirt(def)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("gic-version option is available "
- "only for ARM virt machine"));
- goto cleanup;
- }
-
/* The default GIC version (GICv2) should not be specified on
* the QEMU commandline for backwards compatibility reasons */
if (def->gic_version != VIR_GIC_VERSION_2) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dd36b42eb..98cba8789 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
}
break;
+ case VIR_DOMAIN_FEATURE_GIC:
+ if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ !qemuDomainIsVirt(def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The '%s' feature is only supported for %s
guests"),
+ featureName, "mach-virt");
+ return -1;
+ }
+ break;
+
case VIR_DOMAIN_FEATURE_ACPI:
case VIR_DOMAIN_FEATURE_APIC:
case VIR_DOMAIN_FEATURE_PAE:
@@ -3264,7 +3274,6 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
case VIR_DOMAIN_FEATURE_CAPABILITIES:
case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_VMPORT:
- case VIR_DOMAIN_FEATURE_GIC:
case VIR_DOMAIN_FEATURE_SMM:
case VIR_DOMAIN_FEATURE_VMCOREINFO:
case VIR_DOMAIN_FEATURE_LAST:
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index c8739909d..b7afb6980 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2656,12 +2656,12 @@ mymain(void)
DO_TEST_PARSE_ERROR("aarch64-gic-invalid",
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
- DO_TEST_FAILURE("aarch64-gic-not-virt",
- QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
- QEMU_CAPS_MACH_VIRT_GIC_VERSION);
- DO_TEST_FAILURE("aarch64-gic-not-arm",
- QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
- QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST_PARSE_ERROR("aarch64-gic-not-virt",
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
+ DO_TEST_PARSE_ERROR("aarch64-gic-not-arm",
+ QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
+ QEMU_CAPS_MACH_VIRT_GIC_VERSION);
DO_TEST("aarch64-kvm-32-on-64",
QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
QEMU_CAPS_DEVICE_PL011,
--
2.14.3
1:59 p.m.
New subject: [libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Keep them along with other arch/machine type checks for
features instead of waiting until command line generation
time.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_command.c | 7 -------
src/qemu/qemu_domain.c | 11 ++++++++++-
tests/qemuxml2argvtest.c | 12 ++++++------
3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 24b434a45..529079be0 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7192,13 +7192,6 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON) {
if (def->gic_version != VIR_GIC_VERSION_NONE) {
- if (!qemuDomainIsVirt(def)) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
- _("gic-version option is available "
- "only for ARM virt machine"));
- goto cleanup;
- }
-
/* The default GIC version (GICv2) should not be specified on
* the QEMU commandline for backwards compatibility reasons */
if (def->gic_version != VIR_GIC_VERSION_2) {
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index dd36b42eb..98cba8789 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
}
break;
+ case VIR_DOMAIN_FEATURE_GIC:
+ if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ !qemuDomainIsVirt(def)) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("The '%s' feature is only supported for %s
guests"),
s/for %s guests/for '%s' guests/ ??
+ featureName, "mach-virt");
Not that I think it matters greatly, the error message changes from ARM
specifically to mach-virt... I guess I'm just used to seeing 'ARM' or
'aarch64' and not 'mach-virt' (although the XML would be AIUI
"<type
arch='aarch64' machine='virt'>"). Suffice to say it caused me to
wonder
and I have to imagine it would do the same for anyone getting that message.
I don't have a strong opinion one way or other and it's not really
easily "decode-able" based on someone just reading the "<os... <type
...
machine=''..." in the docs.
Still - the change otherwise seems fine. I trust you'll come up with the
right magical phrase ;-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2:50 a.m.
New subject: [libvirt] [PATCH 03/11] qemu: Move GIC checks to qemuDomainDefValidateFeatures()
On Mon, 2018-02-12 at 14:59 -0500, John Ferlan wrote:
> @@ -3252,6 +3252,16 @@ qemuDomainDefValidateFeatures(const
virDomainDef *def)
> }
> break;
>
> + case VIR_DOMAIN_FEATURE_GIC:
> + if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
> + !qemuDomainIsVirt(def)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("The '%s' feature is only supported
for %s guests"),
s/for %s guests/for '%s' guests/ ??
Sure, why not :)
> + featureName,
"mach-virt");
Not that I think it matters greatly, the error message changes from ARM
specifically to mach-virt... I guess I'm just used to seeing 'ARM' or
'aarch64' and not 'mach-virt' (although the XML would be AIUI
"<type
arch='aarch64' machine='virt'>"). Suffice to say it caused me to
wonder
and I have to imagine it would do the same for anyone getting that message.
I don't have a strong opinion one way or other and it's not really
easily "decode-able" based on someone just reading the "<os... <type
...
machine=''..." in the docs.
Yeah, we're being quite inconsistent throughout both the code and
the test suite.
The root of the problem is that, while other architectures use very
recognizable or at least distinct names for the machine types we
care about, such as pseries or q35, on aarch64 the machine type is
just... virt. Which is not really a great name. But we can't just
use the architecture name either, because there are a lot of arm
and aarch64 machine types out there.
Using mach-virt is an attempt to sidestep the problem, and it's
used (at least internally) in QEMU. But I agree it can be confusing.
Maybe the solution in this case is to use the approach originally
used for the <ioapic/> feature and just say
The '%s' feature is not supported for architecture '%s' or
machine type '%s'
though that's slightly less helpful for users...
--
Andrea Bolognani / Red Hat / Virtualization
10:42 a.m.
New subject: [libvirt] [PATCH 04/11] conf: Use switch in virDomainDefFeaturesCheckABIStability()
The compiler can make sure we are handling all features.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 34aae82f1..e4d01b869 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21324,14 +21324,39 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
size_t i;
for (i = 0; i < VIR_DOMAIN_FEATURE_LAST; i++) {
- if (src->features[i] != dst->features[i]) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("State of feature '%s' differs: "
- "source: '%s', destination:
'%s'"),
- virDomainFeatureTypeToString(i),
- virTristateSwitchTypeToString(src->features[i]),
- virTristateSwitchTypeToString(dst->features[i]));
- return false;
+ const char *featureName = virDomainFeatureTypeToString(i);
+
+ switch ((virDomainFeature) i) {
+ case VIR_DOMAIN_FEATURE_ACPI:
+ case VIR_DOMAIN_FEATURE_APIC:
+ case VIR_DOMAIN_FEATURE_PAE:
+ case VIR_DOMAIN_FEATURE_HAP:
+ case VIR_DOMAIN_FEATURE_VIRIDIAN:
+ case VIR_DOMAIN_FEATURE_PRIVNET:
+ case VIR_DOMAIN_FEATURE_HYPERV:
+ case VIR_DOMAIN_FEATURE_KVM:
+ case VIR_DOMAIN_FEATURE_PVSPINLOCK:
+ case VIR_DOMAIN_FEATURE_CAPABILITIES:
+ case VIR_DOMAIN_FEATURE_PMU:
+ case VIR_DOMAIN_FEATURE_VMPORT:
+ case VIR_DOMAIN_FEATURE_GIC:
+ case VIR_DOMAIN_FEATURE_SMM:
+ case VIR_DOMAIN_FEATURE_IOAPIC:
+ case VIR_DOMAIN_FEATURE_HPT:
+ case VIR_DOMAIN_FEATURE_VMCOREINFO:
+ if (src->features[i] != dst->features[i]) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s' differs: "
+ "source: '%s', destination:
'%s'"),
+ featureName,
+ virTristateSwitchTypeToString(src->features[i]),
+ virTristateSwitchTypeToString(dst->features[i]));
+ return false;
+ }
+ break;
+
+ case VIR_DOMAIN_FEATURE_LAST:
+ break;
}
}
--
2.14.3
2:19 p.m.
New subject: [libvirt] [PATCH 04/11] conf: Use switch in virDomainDefFeaturesCheckABIStability()
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
The compiler can make sure we are handling all features.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 41 +++++++++++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 8 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:42 a.m.
New subject: [libvirt] [PATCH 05/11] conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly
Unlike most other features, VIR_DOMAIN_FEATURE_CAPABILITIES is
of type virDomainCapabilitiesPolicy instead of virTristateSwitch,
so we need to handle it separately for the error message to make
sense.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e4d01b869..9f019c906 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21336,7 +21336,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
case VIR_DOMAIN_FEATURE_HYPERV:
case VIR_DOMAIN_FEATURE_KVM:
case VIR_DOMAIN_FEATURE_PVSPINLOCK:
- case VIR_DOMAIN_FEATURE_CAPABILITIES:
case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_VMPORT:
case VIR_DOMAIN_FEATURE_GIC:
@@ -21355,6 +21354,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
}
break;
+ case VIR_DOMAIN_FEATURE_CAPABILITIES:
+ if (src->features[i] != dst->features[i]) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s', destination:
'%s'"),
+ featureName, "policy",
+
virDomainCapabilitiesPolicyTypeToString(src->features[i]),
+
virDomainCapabilitiesPolicyTypeToString(dst->features[i]));
+ return false;
+ }
+ break;
+
case VIR_DOMAIN_FEATURE_LAST:
break;
}
--
2.14.3
2:18 p.m.
New subject: [libvirt] [PATCH 05/11] conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Unlike most other features, VIR_DOMAIN_FEATURE_CAPABILITIES is
of type virDomainCapabilitiesPolicy instead of virTristateSwitch,
so we need to handle it separately for the error message to make
sense.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index e4d01b869..9f019c906 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21336,7 +21336,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
case VIR_DOMAIN_FEATURE_HYPERV:
case VIR_DOMAIN_FEATURE_KVM:
case VIR_DOMAIN_FEATURE_PVSPINLOCK:
- case VIR_DOMAIN_FEATURE_CAPABILITIES:
case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_VMPORT:
case VIR_DOMAIN_FEATURE_GIC:
@@ -21355,6 +21354,18 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
}
break;
+ case VIR_DOMAIN_FEATURE_CAPABILITIES:
+ if (src->features[i] != dst->features[i]) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s', destination:
'%s'"),
+ featureName, "policy",
OK - based on what I saw Peter ACK for Michal's patches related to his
similar usage, fine. Still looks strange especially since we're talking
about a named XML attribute which we document as "policy". I just hope
there isn't some language variant that alters the meaning. It does look
strange to me "State of feature 'capabilities:policy' differs: "... I
almost wonder if "State of feature 'capabilities' attribute 'policy'
differs: " would help (or really matter).
Also, based on the tests/domainschemadata/domain-caps-features.xml:
<capabilities policy="deny">
<mknod state="on"/>
</capabilities>
/me wonders how many more yaks might get shaved if we needed to check
differences w/r/t the caps_features array?
IOW: If src->features[i] != VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT, then
should we be checking each of the VIR_DOMAIN_CAPS_FEATURE_LAST to ensure
that they're not different too.
Not something required for this patch, but perhaps a follow-up...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
+
virDomainCapabilitiesPolicyTypeToString(src->features[i]),
+
virDomainCapabilitiesPolicyTypeToString(dst->features[i]));
+ return false;
+ }
+ break;
+
case VIR_DOMAIN_FEATURE_LAST:
break;
}
3:11 a.m.
New subject: [libvirt] [PATCH 05/11] conf: Validate VIR_DOMAIN_FEATURE_CAPABILITIES properly
On Mon, 2018-02-12 at 15:18 -0500, John Ferlan wrote:
> + case VIR_DOMAIN_FEATURE_CAPABILITIES:
> + if (src->features[i] != dst->features[i]) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("State of feature '%s:%s' differs:
"
> + "source: '%s', destination:
'%s'"),
> + featureName, "policy",
OK - based on what I saw Peter ACK for Michal's patches related to his
similar usage, fine. Still looks strange especially since we're talking
about a named XML attribute which we document as "policy". I just hope
there isn't some language variant that alters the meaning. It does look
strange to me "State of feature 'capabilities:policy' differs: "... I
almost wonder if "State of feature 'capabilities' attribute
'policy'
differs: " would help (or really matter).
Hm. Maybe it could look like
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s' differs: "
"source: '%s,%s=%s', destination:
'%s,%s=%s'"),
featureName,
virTristateSwitchTypeToString(src->features[i]),
"version", virGICVersionTypeToString(src->gic_version),
virTristateSwitchTypeToString(dst->features[i]),
"version", virGICVersionTypeToString(dst->gic_version));
so that the state of the feature itself and that of its various
attributes, when present, would be displayed without ambiguity.
Also, based on the tests/domainschemadata/domain-caps-features.xml:
<capabilities policy="deny">
<mknod state="on"/>
</capabilities>
/me wonders how many more yaks might get shaved if we needed to check
differences w/r/t the caps_features array?
IOW: If src->features[i] != VIR_DOMAIN_CAPABILITIES_POLICY_DEFAULT, then
should we be checking each of the VIR_DOMAIN_CAPS_FEATURE_LAST to ensure
that they're not different too.
Not something required for this patch, but perhaps a follow-up...
For some reason, I thought caps_features were handled later on along
with kvm_features and hyperv_features, but I see now that's not the
case. I agree that's something that should be addressed.
--
Andrea Bolognani / Red Hat / Virtualization
10:42 a.m.
New subject: [libvirt] [PATCH 06/11] conf: Integrate all features ABI checks in the switch
There are a few stray checks which still live outside of the
switch in virDomainDefFeaturesCheckABIStability() for no good
reason. Move them inside the switch, and update the error
messages to be consistent while at it.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 105 ++++++++++++++++++++++++++++---------------------
1 file changed, 60 insertions(+), 45 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9f019c906..170c56665 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21328,7 +21328,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
switch ((virDomainFeature) i) {
case VIR_DOMAIN_FEATURE_ACPI:
- case VIR_DOMAIN_FEATURE_APIC:
case VIR_DOMAIN_FEATURE_PAE:
case VIR_DOMAIN_FEATURE_HAP:
case VIR_DOMAIN_FEATURE_VIRIDIAN:
@@ -21338,10 +21337,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
case VIR_DOMAIN_FEATURE_PVSPINLOCK:
case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_VMPORT:
- case VIR_DOMAIN_FEATURE_GIC:
case VIR_DOMAIN_FEATURE_SMM:
- case VIR_DOMAIN_FEATURE_IOAPIC:
- case VIR_DOMAIN_FEATURE_HPT:
case VIR_DOMAIN_FEATURE_VMCOREINFO:
if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -21366,28 +21362,69 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
}
break;
- case VIR_DOMAIN_FEATURE_LAST:
+ case VIR_DOMAIN_FEATURE_GIC:
+ if (src->features[i] != dst->features[i] ||
+ src->gic_version != dst->gic_version) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s:%s', destination:
'%s:%s'"),
+ featureName, "version",
+ virTristateSwitchTypeToString(src->features[i]),
+ virGICVersionTypeToString(src->gic_version),
+ virTristateSwitchTypeToString(dst->features[i]),
+ virGICVersionTypeToString(dst->gic_version));
+ return false;
+ }
break;
- }
- }
- /* APIC EOI */
- if (src->apic_eoi != dst->apic_eoi) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("State of APIC EOI differs: "
- "source: '%s', destination: '%s'"),
- virTristateSwitchTypeToString(src->apic_eoi),
- virTristateSwitchTypeToString(dst->apic_eoi));
- return false;
- }
+ case VIR_DOMAIN_FEATURE_HPT:
+ if (src->features[i] != dst->features[i] ||
+ src->hpt_resizing != dst->hpt_resizing) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s:%s', destination:
'%s:%s'"),
+ featureName, "resizing",
+ virTristateSwitchTypeToString(src->features[i]),
+ virDomainHPTResizingTypeToString(src->hpt_resizing),
+ virTristateSwitchTypeToString(dst->features[i]),
+ virDomainHPTResizingTypeToString(dst->hpt_resizing));
+ return false;
+ }
+ break;
- /* GIC version */
- if (src->gic_version != dst->gic_version) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("Source GIC version '%s' does not match destination
'%s'"),
- virGICVersionTypeToString(src->gic_version),
- virGICVersionTypeToString(dst->gic_version));
- return false;
+ case VIR_DOMAIN_FEATURE_APIC:
+ if (src->features[i] != dst->features[i] ||
+ src->apic_eoi != dst->apic_eoi) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s:%s', destination:
'%s:%s'"),
+ featureName, "eoi",
+ virTristateSwitchTypeToString(src->features[i]),
+ virTristateSwitchTypeToString(src->apic_eoi),
+ virTristateSwitchTypeToString(dst->features[i]),
+ virTristateSwitchTypeToString(dst->apic_eoi));
+ return false;
+ }
+ break;
+
+ case VIR_DOMAIN_FEATURE_IOAPIC:
+ if (src->features[i] != dst->features[i] ||
+ src->ioapic != dst->ioapic) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s:%s', destination:
'%s:%s'"),
+ featureName, "driver",
+ virTristateSwitchTypeToString(src->features[i]),
+ virDomainIOAPICTypeToString(src->ioapic),
+ virTristateSwitchTypeToString(dst->features[i]),
+ virDomainIOAPICTypeToString(dst->ioapic));
+ return false;
+ }
+ break;
+
+ case VIR_DOMAIN_FEATURE_LAST:
+ break;
+ }
}
/* hyperv */
@@ -21468,28 +21505,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
}
}
- /* ioapic */
- if (src->ioapic != dst->ioapic) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("State of ioapic differs: "
- "source: '%s', destination: '%s'"),
- virDomainIOAPICTypeToString(src->ioapic),
- virDomainIOAPICTypeToString(dst->ioapic));
- return false;
- }
-
- /* HPT resizing */
- if (src->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) {
- if (src->hpt_resizing != dst->hpt_resizing) {
- virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
- _("HPT resizing configuration differs: "
- "source: '%s', destination:
'%s'"),
- virDomainHPTResizingTypeToString(src->hpt_resizing),
- virDomainHPTResizingTypeToString(dst->hpt_resizing));
- return false;
- }
- }
-
return true;
}
--
2.14.3
2:20 p.m.
New subject: [libvirt] [PATCH 06/11] conf: Integrate all features ABI checks in the switch
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
There are a few stray checks which still live outside of the
switch in virDomainDefFeaturesCheckABIStability() for no good
reason. Move them inside the switch, and update the error
messages to be consistent while at it.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 105 ++++++++++++++++++++++++++++---------------------
1 file changed, 60 insertions(+), 45 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9f019c906..170c56665 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -21328,7 +21328,6 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
switch ((virDomainFeature) i) {
case VIR_DOMAIN_FEATURE_ACPI:
- case VIR_DOMAIN_FEATURE_APIC:
case VIR_DOMAIN_FEATURE_PAE:
case VIR_DOMAIN_FEATURE_HAP:
case VIR_DOMAIN_FEATURE_VIRIDIAN:
@@ -21338,10 +21337,7 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
case VIR_DOMAIN_FEATURE_PVSPINLOCK:
case VIR_DOMAIN_FEATURE_PMU:
case VIR_DOMAIN_FEATURE_VMPORT:
- case VIR_DOMAIN_FEATURE_GIC:
case VIR_DOMAIN_FEATURE_SMM:
- case VIR_DOMAIN_FEATURE_IOAPIC:
- case VIR_DOMAIN_FEATURE_HPT:
case VIR_DOMAIN_FEATURE_VMCOREINFO:
if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -21366,28 +21362,69 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
}
break;
- case VIR_DOMAIN_FEATURE_LAST:
+ case VIR_DOMAIN_FEATURE_GIC:
+ if (src->features[i] != dst->features[i] ||
+ src->gic_version != dst->gic_version) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("State of feature '%s:%s' differs: "
+ "source: '%s:%s', destination:
'%s:%s'"),
+ featureName, "version",
+ virTristateSwitchTypeToString(src->features[i]),
+ virGICVersionTypeToString(src->gic_version),
+ virTristateSwitchTypeToString(dst->features[i]),
+ virGICVersionTypeToString(dst->gic_version));
+ return false;
Similar to previous, should these become "State of feature '%s'
attribute 'version' differs: "
likewise for other error messages.... Leave it up to you to decide though.
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:42 a.m.
New subject: [libvirt] [PATCH 07/11] tests: Improve GIC tests
Account for the fact that the default might change based on what
GIC versions are supported by QEMU. That's not the case at the
moment, but it will be soon.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
tests/qemuxml2argvdata/aarch64-gic-default-both.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-both.xml | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v2.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 +
tests/qemuxml2argvtest.c | 6 +++---
tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml | 1 +
tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml | 1 +
tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 1 +
tests/qemuxml2xmltest.c | 6 +++---
11 files changed, 15 insertions(+), 6 deletions(-)
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.xml
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.args
b/tests/qemuxml2argvdata/aarch64-gic-default-both.args
new file mode 120000
index 000000000..04ecd4ce7
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.args
@@ -0,0 +1 @@
+aarch64-gic-v2.args
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.xml
b/tests/qemuxml2argvdata/aarch64-gic-default-both.xml
new file mode 120000
index 000000000..3e2183c92
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.xml
@@ -0,0 +1 @@
+aarch64-gic-default.xml
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v2.args
b/tests/qemuxml2argvdata/aarch64-gic-default-v2.args
new file mode 120000
index 000000000..04ecd4ce7
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-v2.args
@@ -0,0 +1 @@
+aarch64-gic-v2.args
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
b/tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
new file mode 120000
index 000000000..3e2183c92
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
@@ -0,0 +1 @@
+aarch64-gic-default.xml
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
new file mode 120000
index 000000000..04ecd4ce7
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
@@ -0,0 +1 @@
+aarch64-gic-v2.args
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
b/tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
new file mode 120000
index 000000000..3e2183c92
--- /dev/null
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
@@ -0,0 +1 @@
+aarch64-gic-default.xml
\ No newline at end of file
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index b7afb6980..dd64772e2 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -2602,13 +2602,13 @@ mymain(void)
DO_TEST_GIC("aarch64-gic-default", GIC_NONE,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
- DO_TEST_GIC("aarch64-gic-default", GIC_V2,
+ DO_TEST_GIC("aarch64-gic-default-v2", GIC_V2,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
- DO_TEST_GIC("aarch64-gic-default", GIC_V3,
+ DO_TEST_GIC("aarch64-gic-default-v3", GIC_V3,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
- DO_TEST_GIC("aarch64-gic-default", GIC_BOTH,
+ DO_TEST_GIC("aarch64-gic-default-both", GIC_BOTH,
QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACH_VIRT_GIC_VERSION);
DO_TEST_GIC("aarch64-gic-v2", GIC_NONE,
diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
new file mode 120000
index 000000000..ee470fb1f
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/aarch64-gic-v2.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
b/tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
new file mode 120000
index 000000000..ee470fb1f
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/aarch64-gic-v2.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
new file mode 120000
index 000000000..ee470fb1f
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/aarch64-gic-v2.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index d3544a1ef..83809574c 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -1265,9 +1265,9 @@ mymain(void)
DO_TEST_FULL("aarch64-gic-none-both", WHEN_BOTH, GIC_BOTH, NONE);
DO_TEST_FULL("aarch64-gic-none-tcg", WHEN_BOTH, GIC_BOTH, NONE);
DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_NONE, NONE);
- DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V2, NONE);
- DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_V3, NONE);
- DO_TEST_FULL("aarch64-gic-default", WHEN_BOTH, GIC_BOTH, NONE);
+ DO_TEST_FULL("aarch64-gic-default-v2", WHEN_BOTH, GIC_V2, NONE);
+ DO_TEST_FULL("aarch64-gic-default-v3", WHEN_BOTH, GIC_V3, NONE);
+ DO_TEST_FULL("aarch64-gic-default-both", WHEN_BOTH, GIC_BOTH, NONE);
DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_NONE, NONE);
DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V2, NONE);
DO_TEST_FULL("aarch64-gic-v2", WHEN_BOTH, GIC_V3, NONE);
--
2.14.3
2:46 p.m.
New subject: [libvirt] [PATCH 07/11] tests: Improve GIC tests
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Account for the fact that the default might change based on what
GIC versions are supported by QEMU. That's not the case at the
moment, but it will be soon.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
tests/qemuxml2argvdata/aarch64-gic-default-both.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-both.xml | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v2.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v2.xml | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 1 +
tests/qemuxml2argvdata/aarch64-gic-default-v3.xml | 1 +
tests/qemuxml2argvtest.c | 6 +++---
tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml | 1 +
tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml | 1 +
tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 1 +
tests/qemuxml2xmltest.c | 6 +++---
11 files changed, 15 insertions(+), 6 deletions(-)
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-both.xml
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v2.xml
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.args
create mode 120000 tests/qemuxml2argvdata/aarch64-gic-default-v3.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v2.xml
create mode 120000 tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
10:42 a.m.
New subject: [libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case
When no GIC version is specified, we currently default to GIC v2;
however, that's not a great default, since guests will fail to
start if the hardware only supports GIC v3.
Change the behavior so that a sensible default is chosen instead.
That basically means using the same algorithm whether the user
didn't explicitly enable the GIC feature or they explicitly
enabled it but didn't specify any GIC version.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 25 +++++++++++-----------
.../qemuxml2argvdata/aarch64-gic-default-both.args | 2 +-
tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 2 +-
.../aarch64-gic-default-both.xml | 2 +-
.../qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 2 +-
5 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 98cba8789..ee720d328 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2838,13 +2838,14 @@ static void
qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
{
- virGICVersion version;
-
- /* The virt machine type always uses GIC: if the relevant element
+ /* The virt machine type always uses GIC: if the relevant information
* was not included in the domain XML, we need to choose a suitable
* GIC version ourselves */
- if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT
&&
- qemuDomainIsVirt(def)) {
+ if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT
&&
+ qemuDomainIsVirt(def)) ||
+ (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
+ def->gic_version == VIR_GIC_VERSION_NONE)) {
+ virGICVersion version;
VIR_DEBUG("Looking for usable GIC version in domain capabilities");
for (version = VIR_GIC_VERSION_LAST - 1;
@@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
}
}
+ /* Use the default GIC version (GICv2) as a last-ditch attempt
+ * if no match could be found above */
+ if (def->gic_version == VIR_GIC_VERSION_NONE) {
+ VIR_DEBUG("Using GIC version 2 (default)");
+ def->gic_version = VIR_GIC_VERSION_2;
+ }
+
/* Even if we haven't found a usable GIC version in the domain
* capabilities, we still want to enable this */
def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
}
-
- /* Use the default GIC version (GICv2) if no version was specified */
- if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
- def->gic_version == VIR_GIC_VERSION_NONE) {
- VIR_DEBUG("Using GIC version 2 (default)");
- def->gic_version = VIR_GIC_VERSION_2;
- }
}
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-both.args
b/tests/qemuxml2argvdata/aarch64-gic-default-both.args
index 04ecd4ce7..6209eff4b 120000
--- a/tests/qemuxml2argvdata/aarch64-gic-default-both.args
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-both.args
@@ -1 +1 @@
-aarch64-gic-v2.args
\ No newline at end of file
+aarch64-gic-v3.args
\ No newline at end of file
diff --git a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
index 04ecd4ce7..6209eff4b 120000
--- a/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
+++ b/tests/qemuxml2argvdata/aarch64-gic-default-v3.args
@@ -1 +1 @@
-aarch64-gic-v2.args
\ No newline at end of file
+aarch64-gic-v3.args
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
index ee470fb1f..bf9d58c38 120000
--- a/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
+++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-both.xml
@@ -1 +1 @@
-../qemuxml2argvdata/aarch64-gic-v2.xml
\ No newline at end of file
+../qemuxml2argvdata/aarch64-gic-v3.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
index ee470fb1f..bf9d58c38 120000
--- a/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
+++ b/tests/qemuxml2xmloutdata/aarch64-gic-default-v3.xml
@@ -1 +1 @@
-../qemuxml2argvdata/aarch64-gic-v2.xml
\ No newline at end of file
+../qemuxml2argvdata/aarch64-gic-v3.xml
\ No newline at end of file
--
2.14.3
3:05 p.m.
New subject: [libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
When no GIC version is specified, we currently default to GIC v2;
however, that's not a great default, since guests will fail to
start if the hardware only supports GIC v3.
Change the behavior so that a sensible default is chosen instead.
That basically means using the same algorithm whether the user
didn't explicitly enable the GIC feature or they explicitly
enabled it but didn't specify any GIC version.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_domain.c | 25 +++++++++++-----------
.../qemuxml2argvdata/aarch64-gic-default-both.args | 2 +-
tests/qemuxml2argvdata/aarch64-gic-default-v3.args | 2 +-
.../aarch64-gic-default-both.xml | 2 +-
.../qemuxml2xmloutdata/aarch64-gic-default-v3.xml | 2 +-
5 files changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index 98cba8789..ee720d328 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -2838,13 +2838,14 @@ static void
qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
virQEMUCapsPtr qemuCaps)
{
- virGICVersion version;
-
- /* The virt machine type always uses GIC: if the relevant element
+ /* The virt machine type always uses GIC: if the relevant information
* was not included in the domain XML, we need to choose a suitable
* GIC version ourselves */
- if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT
&&
- qemuDomainIsVirt(def)) {
+ if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT
&&
+ qemuDomainIsVirt(def)) ||
+ (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
And patch3 (e.g. verification) takes care of us if this is "on", but
!qemuDomainIsVirt(def) ends up being true... It's a tangled web.
+ def->gic_version == VIR_GIC_VERSION_NONE)) {
+ virGICVersion version;
VIR_DEBUG("Looking for usable GIC version in domain capabilities");
for (version = VIR_GIC_VERSION_LAST - 1;
@@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
}
}
FWIW: There's a hunk above here which notes something about bz
1414081... If one goes and reads that bz, one finds it's closed notabug.
In any case, there's a lot of verbiage there.. Can any of it be shaved?
What's being done seems reasonable, but if we can take the opportunity
to revisit the comment as well - that'd be good.
For what's here as long as the above comment doesn't cause you to have
an oh sh* moment...
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
+ /* Use the default GIC version (GICv2) as a last-ditch
attempt
+ * if no match could be found above */
+ if (def->gic_version == VIR_GIC_VERSION_NONE) {
+ VIR_DEBUG("Using GIC version 2 (default)");
+ def->gic_version = VIR_GIC_VERSION_2;
+ }
+
/* Even if we haven't found a usable GIC version in the domain
* capabilities, we still want to enable this */
def->features[VIR_DOMAIN_FEATURE_GIC] = VIR_TRISTATE_SWITCH_ON;
}
-
- /* Use the default GIC version (GICv2) if no version was specified */
- if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON &&
- def->gic_version == VIR_GIC_VERSION_NONE) {
- VIR_DEBUG("Using GIC version 2 (default)");
- def->gic_version = VIR_GIC_VERSION_2;
- }
}
[...]
3:22 a.m.
New subject: [libvirt] [PATCH 08/11] qemu: Fix GIC behavior for the default case
On Mon, 2018-02-12 at 16:05 -0500, John Ferlan wrote:
> qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
> virQEMUCapsPtr qemuCaps)
> {
> - virGICVersion version;
> -
> - /* The virt machine type always uses GIC: if the relevant element
> + /* The virt machine type always uses GIC: if the relevant information
> * was not included in the domain XML, we need to choose a suitable
> * GIC version ourselves */
> - if (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT
&&
> - qemuDomainIsVirt(def)) {
> + if ((def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ABSENT
&&
> + qemuDomainIsVirt(def)) ||
> + (def->features[VIR_DOMAIN_FEATURE_GIC] == VIR_TRISTATE_SWITCH_ON
&&
And patch3 (e.g. verification) takes care of us if this is "on", but
!qemuDomainIsVirt(def) ends up being true... It's a tangled web.
That's correct.
> + def->gic_version == VIR_GIC_VERSION_NONE)) {
> + virGICVersion version;
>
> VIR_DEBUG("Looking for usable GIC version in domain
capabilities");
> for (version = VIR_GIC_VERSION_LAST - 1;
> @@ -2878,17 +2879,17 @@ qemuDomainDefEnableDefaultFeatures(virDomainDefPtr def,
> }
> }
>
FWIW: There's a hunk above here which notes something about bz
1414081... If one goes and reads that bz, one finds it's closed notabug.
In any case, there's a lot of verbiage there.. Can any of it be shaved?
Not a lot, I don't think... Maybe
We'll want to revisit this once MSI support for GICv3 has been
implemented in QEMU.
but that's about it, everything else still applies.
--
Andrea Bolognani / Red Hat / Virtualization
10:42 a.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
Instead of storing separately whether the feature is enabled
or not and what driver should be used, store both of them in
a single place.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 29 +++++++++++++----------------
src/conf/domain_conf.h | 11 ++++++-----
src/qemu/qemu_command.c | 5 +++--
src/qemu/qemu_domain.c | 2 +-
4 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 170c56665..19884ec13 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader,
VIR_ENUM_IMPL(virDomainIOAPIC,
VIR_DOMAIN_IOAPIC_LAST,
+ "none",
"qemu",
"kvm")
@@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
if (def->iommu &&
def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
- (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON ||
- def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) {
+ def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("IOMMU interrupt remapping requires split I/O APIC "
"(ioapic driver='qemu')"));
@@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
tmp = virXMLPropString(nodes[i], "driver");
if (tmp) {
int value = virDomainIOAPICTypeFromString(tmp);
- if (value < 0) {
+ if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown driver mode: %s"),
tmp);
goto error;
}
- def->ioapic = value;
- def->features[val] = VIR_TRISTATE_SWITCH_ON;
+ def->features[val] = value;
VIR_FREE(tmp);
}
break;
@@ -21408,16 +21407,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
break;
case VIR_DOMAIN_FEATURE_IOAPIC:
- if (src->features[i] != dst->features[i] ||
- src->ioapic != dst->ioapic) {
+ if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s:%s' differs: "
- "source: '%s:%s', destination:
'%s:%s'"),
+ "source: '%s', destination:
'%s'"),
featureName, "driver",
- virTristateSwitchTypeToString(src->features[i]),
- virDomainIOAPICTypeToString(src->ioapic),
- virTristateSwitchTypeToString(dst->features[i]),
- virDomainIOAPICTypeToString(dst->ioapic));
+ virDomainIOAPICTypeToString(src->features[i]),
+ virDomainIOAPICTypeToString(dst->features[i]));
return false;
}
break;
@@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
case VIR_DOMAIN_FEATURE_IOAPIC:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
- virBufferAsprintf(buf, "<ioapic
driver='%s'/>\n",
- virDomainIOAPICTypeToString(def->ioapic));
- }
+ if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE)
+ break;
+
+ virBufferAsprintf(buf, "<ioapic
driver='%s'/>\n",
+ virDomainIOAPICTypeToString(def->features[i]));
break;
case VIR_DOMAIN_FEATURE_HPT:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 21e004515..20f0efc36 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef {
void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
typedef enum {
- VIR_DOMAIN_IOAPIC_QEMU = 0,
+ VIR_DOMAIN_IOAPIC_NONE = 0,
+ VIR_DOMAIN_IOAPIC_QEMU,
VIR_DOMAIN_IOAPIC_KVM,
VIR_DOMAIN_IOAPIC_LAST
@@ -2352,9 +2353,10 @@ struct _virDomainDef {
virDomainOSDef os;
char *emulator;
- /* These three options are of type virTristateSwitch,
- * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
- * virDomainCapabilitiesPolicy */
+ /* Most of the values in {kvm_,hyperv_,}features are of type
+ * virTristateSwitch, but there are exceptions: for example,
+ * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
+ * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
int features[VIR_DOMAIN_FEATURE_LAST];
int apic_eoi;
int hyperv_features[VIR_DOMAIN_HYPERV_LAST];
@@ -2362,7 +2364,6 @@ struct _virDomainDef {
unsigned int hyperv_spinlocks;
virGICVersion gic_version;
char *hyperv_vendor_id;
- virDomainIOAPIC ioapic;
virDomainHPTResizing hpt_resizing;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 529079be0..3aabdf7a2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7240,14 +7240,14 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
}
}
- if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON) {
+ if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_NONE) {
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("I/O APIC tuning is not supported by this "
"QEMU binary"));
goto cleanup;
}
- switch (def->ioapic) {
+ switch ((virDomainIOAPIC) def->features[VIR_DOMAIN_FEATURE_IOAPIC]) {
case VIR_DOMAIN_IOAPIC_QEMU:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -7260,6 +7260,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
case VIR_DOMAIN_IOAPIC_KVM:
virBufferAddLit(&buf, ",kernel_irqchip=on");
break;
+ case VIR_DOMAIN_IOAPIC_NONE:
case VIR_DOMAIN_IOAPIC_LAST:
break;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ee720d328..cfea1f500 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3234,7 +3234,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
switch ((virDomainFeature) i) {
case VIR_DOMAIN_FEATURE_IOAPIC:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ if (def->features[i] != VIR_DOMAIN_IOAPIC_NONE &&
!ARCH_IS_X86(def->os.arch)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The '%s' feature is only supported for %s
guests"),
--
2.14.3
3:59 p.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Instead of storing separately whether the feature is enabled
or not and what driver should be used, store both of them in
a single place.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 29 +++++++++++++----------------
src/conf/domain_conf.h | 11 ++++++-----
src/qemu/qemu_command.c | 5 +++--
src/qemu/qemu_domain.c | 2 +-
4 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 170c56665..19884ec13 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -900,6 +900,7 @@ VIR_ENUM_IMPL(virDomainLoader,
VIR_ENUM_IMPL(virDomainIOAPIC,
VIR_DOMAIN_IOAPIC_LAST,
+ "none",
"qemu",
"kvm")
@@ -5913,8 +5914,7 @@ virDomainDefValidateInternal(const virDomainDef *def)
if (def->iommu &&
def->iommu->intremap == VIR_TRISTATE_SWITCH_ON &&
- (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_TRISTATE_SWITCH_ON ||
- def->ioapic != VIR_DOMAIN_IOAPIC_QEMU)) {
+ def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_QEMU) {
virReportError(VIR_ERR_XML_ERROR, "%s",
_("IOMMU interrupt remapping requires split I/O APIC "
"(ioapic driver='qemu')"));
@@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
tmp = virXMLPropString(nodes[i], "driver");
if (tmp) {
int value = virDomainIOAPICTypeFromString(tmp);
- if (value < 0) {
+ if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
"none" wouldn't be legal XML according to "iopic" in
domaincommon.rng. I
think this is where things get tricky - I'm fine with the changes as is,
but that interaction between domain_conf xml parsing and the rng schema
gets me wondering about whether that "none" value should be in the rng.
Perhaps there's another opinion on this that may want to pipe in now...
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown driver mode: %s"),
tmp);
goto error;
}
- def->ioapic = value;
- def->features[val] = VIR_TRISTATE_SWITCH_ON;
+ def->features[val] = value;
VIR_FREE(tmp);
}
break;
@@ -21408,16 +21407,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
break;
case VIR_DOMAIN_FEATURE_IOAPIC:
- if (src->features[i] != dst->features[i] ||
- src->ioapic != dst->ioapic) {
+ if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s:%s' differs: "
- "source: '%s:%s', destination:
'%s:%s'"),
+ "source: '%s', destination:
'%s'"),
featureName, "driver",
- virTristateSwitchTypeToString(src->features[i]),
- virDomainIOAPICTypeToString(src->ioapic),
- virTristateSwitchTypeToString(dst->features[i]),
- virDomainIOAPICTypeToString(dst->ioapic));
+ virDomainIOAPICTypeToString(src->features[i]),
+ virDomainIOAPICTypeToString(dst->features[i]));
return false;
}
break;
@@ -26943,10 +26939,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
case VIR_DOMAIN_FEATURE_IOAPIC:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
- virBufferAsprintf(buf, "<ioapic
driver='%s'/>\n",
- virDomainIOAPICTypeToString(def->ioapic));
- }
+ if (def->features[i] == VIR_DOMAIN_IOAPIC_NONE)
+ break;
+
+ virBufferAsprintf(buf, "<ioapic
driver='%s'/>\n",
+ virDomainIOAPICTypeToString(def->features[i]));
break;
case VIR_DOMAIN_FEATURE_HPT:
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 21e004515..20f0efc36 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1861,7 +1861,8 @@ struct _virDomainLoaderDef {
void virDomainLoaderDefFree(virDomainLoaderDefPtr loader);
typedef enum {
- VIR_DOMAIN_IOAPIC_QEMU = 0,
+ VIR_DOMAIN_IOAPIC_NONE = 0,
+ VIR_DOMAIN_IOAPIC_QEMU,
VIR_DOMAIN_IOAPIC_KVM,
VIR_DOMAIN_IOAPIC_LAST
@@ -2352,9 +2353,10 @@ struct _virDomainDef {
virDomainOSDef os;
char *emulator;
- /* These three options are of type virTristateSwitch,
- * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
- * virDomainCapabilitiesPolicy */
+ /* Most of the values in {kvm_,hyperv_,}features are of type
{kvm_|hyperv_|caps_}features
+ * virTristateSwitch, but there are exceptions: for example,
+ * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
s/,//
+ * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so
on */
s/and so on/./
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
int features[VIR_DOMAIN_FEATURE_LAST];
int apic_eoi;
int hyperv_features[VIR_DOMAIN_HYPERV_LAST];
@@ -2362,7 +2364,6 @@ struct _virDomainDef {
unsigned int hyperv_spinlocks;
virGICVersion gic_version;
char *hyperv_vendor_id;
- virDomainIOAPIC ioapic;
virDomainHPTResizing hpt_resizing;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 529079be0..3aabdf7a2 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7240,14 +7240,14 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
}
}
- if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] == VIR_TRISTATE_SWITCH_ON) {
+ if (def->features[VIR_DOMAIN_FEATURE_IOAPIC] != VIR_DOMAIN_IOAPIC_NONE) {
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("I/O APIC tuning is not supported by this "
"QEMU binary"));
goto cleanup;
}
- switch (def->ioapic) {
+ switch ((virDomainIOAPIC) def->features[VIR_DOMAIN_FEATURE_IOAPIC]) {
case VIR_DOMAIN_IOAPIC_QEMU:
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_KERNEL_IRQCHIP_SPLIT))
{
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
@@ -7260,6 +7260,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
case VIR_DOMAIN_IOAPIC_KVM:
virBufferAddLit(&buf, ",kernel_irqchip=on");
break;
+ case VIR_DOMAIN_IOAPIC_NONE:
case VIR_DOMAIN_IOAPIC_LAST:
break;
}
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index ee720d328..cfea1f500 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3234,7 +3234,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
switch ((virDomainFeature) i) {
case VIR_DOMAIN_FEATURE_IOAPIC:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ if (def->features[i] != VIR_DOMAIN_IOAPIC_NONE &&
!ARCH_IS_X86(def->os.arch)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The '%s' feature is only supported for %s
guests"),
3:40 a.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
> @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
> tmp = virXMLPropString(nodes[i], "driver");
> if (tmp) {
> int value = virDomainIOAPICTypeFromString(tmp);
> - if (value < 0) {
> + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
"none" wouldn't be legal XML according to "iopic" in
domaincommon.rng. I
think this is where things get tricky - I'm fine with the changes as is,
but that interaction between domain_conf xml parsing and the rng schema
gets me wondering about whether that "none" value should be in the rng.
Perhaps there's another opinion on this that may want to pipe in now...
"none" should definitely *not* be in the schema, since it's not
a valid value. The problem here is that schema compliance is not
enforced by libvirt: when using virsh, you are given the option
to simply ignore schema validation failures and go ahead with
defining/modifying the guest, so the only way to actually make
sure invalid values don't make it into the virDomainDef is to do
something like the above.
I mean, it's not like accepting "none" in the parser would be a
big issue - it would just be ignored anyway. But by adding an
explicit check we can report a better error in the very unlikely
scenario where
<ioapic driver="none"/>
has been specified in the guest configuration, so I think it's
a good idea to have it there given how little effort it requires.
> @@ -2352,9 +2353,10 @@ struct _virDomainDef {
>
> virDomainOSDef os;
> char *emulator;
> - /* These three options are of type virTristateSwitch,
> - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
> - * virDomainCapabilitiesPolicy */
> + /* Most of the values in {kvm_,hyperv_,}features are of type
{kvm_|hyperv_|caps_}features
> + * virTristateSwitch, but there are exceptions: for example,
> + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
s/,//
> + * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
s/and so on/./
I see you ask for the comment to be updated in the next commit when
_HPT is changed as well, but it wasn't really my intention to list
all types there - we both know how that kind of comment can get out
of sync with reality very quickly :)
Another approach, which didn't make it to the list for some reason,
was to end the comment with
[...] and so on. See virDomainDefFeaturesCheckABIStability() for
more details.
That seems like a better way to handle the ever-changing nature of
libvirt than a comment, don't you think?
--
Andrea Bolognani / Red Hat / Virtualization
6:22 a.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
On 02/13/2018 04:40 AM, Andrea Bolognani wrote:
On Mon, 2018-02-12 at 16:59 -0500, John Ferlan wrote:
>> @@ -19224,14 +19224,13 @@ virDomainDefParseXML(xmlDocPtr xml,
>> tmp = virXMLPropString(nodes[i], "driver");
>> if (tmp) {
>> int value = virDomainIOAPICTypeFromString(tmp);
>> - if (value < 0) {
>> + if (value < 0 || value == VIR_DOMAIN_IOAPIC_NONE) {
>
> "none" wouldn't be legal XML according to "iopic" in
domaincommon.rng. I
> think this is where things get tricky - I'm fine with the changes as is,
> but that interaction between domain_conf xml parsing and the rng schema
> gets me wondering about whether that "none" value should be in the rng.
> Perhaps there's another opinion on this that may want to pipe in now...
"none" should definitely *not* be in the schema, since it's not
a valid value. The problem here is that schema compliance is not
enforced by libvirt: when using virsh, you are given the option
to simply ignore schema validation failures and go ahead with
defining/modifying the guest, so the only way to actually make
sure invalid values don't make it into the virDomainDef is to do
something like the above.
I mean, it's not like accepting "none" in the parser would be a
big issue - it would just be ignored anyway. But by adding an
explicit check we can report a better error in the very unlikely
scenario where
<ioapic driver="none"/>
has been specified in the guest configuration, so I think it's
a good idea to have it there given how little effort it requires.
>> @@ -2352,9 +2353,10 @@ struct _virDomainDef {
>>
>> virDomainOSDef os;
>> char *emulator;
>> - /* These three options are of type virTristateSwitch,
>> - * except VIR_DOMAIN_FEATURE_CAPABILITIES that is of type
>> - * virDomainCapabilitiesPolicy */
>> + /* Most of the values in {kvm_,hyperv_,}features are of type
>
> {kvm_|hyperv_|caps_}features
>
>> + * virTristateSwitch, but there are exceptions: for example,
>> + * VIR_DOMAIN_FEATURE_CAPABILITIES is of type virDomainCapabilitiesPolicy,
>
> s/,//
>
>> + * VIR_DOMAIN_FEATURE_IOAPIC is of type virDomainIOAPIC and so on */
>
> s/and so on/./
I see you ask for the comment to be updated in the next commit when
_HPT is changed as well, but it wasn't really my intention to list
all types there - we both know how that kind of comment can get out
of sync with reality very quickly :)
Another approach, which didn't make it to the list for some reason,
was to end the comment with
[...] and so on. See virDomainDefFeaturesCheckABIStability() for
more details.
That seems like a better way to handle the ever-changing nature of
libvirt than a comment, don't you think?
That's fine... I guess since you started listing them I figured adding
another in the next patch was "natural".
How about this (or something close to it):
"Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
handle support. A few assign specific data values to the option. See
virDomainDefFeaturesCheckABIStability for details."
John
7:46 a.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
On Tue, 2018-02-13 at 07:22 -0500, John Ferlan wrote:
> Another approach, which didn't make it to the list for some
reason,
> was to end the comment with
>
> [...] and so on. See virDomainDefFeaturesCheckABIStability() for
> more details.
>
> That seems like a better way to handle the ever-changing nature of
> libvirt than a comment, don't you think?
That's fine... I guess since you started listing them I figured adding
another in the next patch was "natural".
How about this (or something close to it):
"Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
handle support. A few assign specific data values to the option. See
virDomainDefFeaturesCheckABIStability for details."
I've changed it to match your suggestion.
I've also addressed all other review comments as described in my
previous replies. Do you have any objections to those? Should I
send out a v2, or should I just go ahead and push the series given
the minor nature of the adjustments?
--
Andrea Bolognani / Red Hat / Virtualization
8:34 a.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
On 02/13/2018 08:46 AM, Andrea Bolognani wrote:
On Tue, 2018-02-13 at 07:22 -0500, John Ferlan wrote:
>> Another approach, which didn't make it to the list for some reason,
>> was to end the comment with
>>
>> [...] and so on. See virDomainDefFeaturesCheckABIStability() for
>> more details.
>>
>> That seems like a better way to handle the ever-changing nature of
>> libvirt than a comment, don't you think?
>
> That's fine... I guess since you started listing them I figured adding
> another in the next patch was "natural".
>
> How about this (or something close to it):
>
> "Most {hyperv_|kvm_|cpu_}feature options utilize a virTristateSwitch to
> handle support. A few assign specific data values to the option. See
> virDomainDefFeaturesCheckABIStability for details."
I've changed it to match your suggestion.
I've also addressed all other review comments as described in my
previous replies. Do you have any objections to those? Should I
send out a v2, or should I just go ahead and push the series given
the minor nature of the adjustments?
I see no need for a v2 since the adjustments were essentially minor.
Consider some of those comments a replacement for you (or I) being able
to walk across the big pond in order to toss random thoughts or ideas
that spring to mind when I'm reading patches. Many times it's just what
I'm thinking as I'm reading. If it gives you an aha moment to describe
or do something different, then great. If it's utter nonesense, then no
big deal either ;-).
John
8:53 a.m.
New subject: [libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
On Tue, 2018-02-13 at 09:34 -0500, John Ferlan wrote:
On 02/13/2018 08:46 AM, Andrea Bolognani wrote:
> [...] Should I
> send out a v2, or should I just go ahead and push the series given
> the minor nature of the adjustments?
I see no need for a v2 since the adjustments were essentially minor.
Consider some of those comments a replacement for you (or I) being able
to walk across the big pond in order to toss random thoughts or ideas
that spring to mind when I'm reading patches. Many times it's just what
I'm thinking as I'm reading. If it gives you an aha moment to describe
or do something different, then great. If it's utter nonesense, then no
big deal either ;-).
Cool, I had assumed as much but wanted to make sure ;)
I've now pushed the series, thanks for your review!
--
Andrea Bolognani / Red Hat / Virtualization
10:42 a.m.
New subject: [libvirt] [PATCH 10/11] conf: Improve HPT feature handling
Instead of storing separately whether the feature is enabled
or not and what resizing policy should be used, store both of
them in a single place.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 26 ++++++++++++--------------
src/conf/domain_conf.h | 4 ++--
src/qemu/qemu_command.c | 4 ++--
src/qemu/qemu_domain.c | 2 +-
4 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 19884ec13..c1d549594 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC,
VIR_ENUM_IMPL(virDomainHPTResizing,
VIR_DOMAIN_HPT_RESIZING_LAST,
+ "none",
"enabled",
"disabled",
"required",
@@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml,
tmp = virXMLPropString(nodes[i], "resizing");
if (tmp) {
int value = virDomainHPTResizingTypeFromString(tmp);
- if (value < 0) {
+ if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown HPT resizing setting: %s"),
tmp);
goto error;
}
- def->hpt_resizing = value;
- def->features[val] = VIR_TRISTATE_SWITCH_ON;
+ def->features[val] = value;
VIR_FREE(tmp);
}
break;
@@ -21377,16 +21377,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
break;
case VIR_DOMAIN_FEATURE_HPT:
- if (src->features[i] != dst->features[i] ||
- src->hpt_resizing != dst->hpt_resizing) {
+ if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s:%s' differs: "
- "source: '%s:%s', destination:
'%s:%s'"),
+ "source: '%s', destination:
'%s'"),
featureName, "resizing",
- virTristateSwitchTypeToString(src->features[i]),
- virDomainHPTResizingTypeToString(src->hpt_resizing),
- virTristateSwitchTypeToString(dst->features[i]),
- virDomainHPTResizingTypeToString(dst->hpt_resizing));
+ virDomainHPTResizingTypeToString(src->features[i]),
+ virDomainHPTResizingTypeToString(dst->features[i]));
return false;
}
break;
@@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
case VIR_DOMAIN_FEATURE_HPT:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
- virBufferAsprintf(buf, "<hpt
resizing='%s'/>\n",
-
virDomainHPTResizingTypeToString(def->hpt_resizing));
- }
+ if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE)
+ break;
+
+ virBufferAsprintf(buf, "<hpt resizing='%s'/>\n",
+
virDomainHPTResizingTypeToString(def->features[i]));
break;
/* coverity[dead_error_begin] */
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 20f0efc36..4e9044ae6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1871,7 +1871,8 @@ typedef enum {
VIR_ENUM_DECL(virDomainIOAPIC);
typedef enum {
- VIR_DOMAIN_HPT_RESIZING_ENABLED = 0,
+ VIR_DOMAIN_HPT_RESIZING_NONE = 0,
+ VIR_DOMAIN_HPT_RESIZING_ENABLED,
VIR_DOMAIN_HPT_RESIZING_DISABLED,
VIR_DOMAIN_HPT_RESIZING_REQUIRED,
@@ -2364,7 +2365,6 @@ struct _virDomainDef {
unsigned int hyperv_spinlocks;
virGICVersion gic_version;
char *hyperv_vendor_id;
- virDomainHPTResizing hpt_resizing;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */
int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3aabdf7a2..faf09a599 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -7266,7 +7266,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
}
}
- if (def->features[VIR_DOMAIN_FEATURE_HPT] == VIR_TRISTATE_SWITCH_ON) {
+ if (def->features[VIR_DOMAIN_FEATURE_HPT] != VIR_DOMAIN_HPT_RESIZING_NONE) {
const char *str;
if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT)) {
@@ -7276,7 +7276,7 @@ qemuBuildMachineCommandLine(virCommandPtr cmd,
goto cleanup;
}
- str = virDomainHPTResizingTypeToString(def->hpt_resizing);
+ str =
virDomainHPTResizingTypeToString(def->features[VIR_DOMAIN_FEATURE_HPT]);
if (!str) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
_("Invalid setting for HPT resizing"));
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
index cfea1f500..5d21d3703 100644
--- a/src/qemu/qemu_domain.c
+++ b/src/qemu/qemu_domain.c
@@ -3244,7 +3244,7 @@ qemuDomainDefValidateFeatures(const virDomainDef *def)
break;
case VIR_DOMAIN_FEATURE_HPT:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON &&
+ if (def->features[i] != VIR_DOMAIN_HPT_RESIZING_NONE &&
!qemuDomainIsPSeries(def)) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("The '%s' feature is only supported for %s
guests"),
--
2.14.3
4:04 p.m.
New subject: [libvirt] [PATCH 10/11] conf: Improve HPT feature handling
On 02/06/2018 11:42 AM, Andrea Bolognani wrote:
Instead of storing separately whether the feature is enabled
or not and what resizing policy should be used, store both of
them in a single place.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/conf/domain_conf.c | 26 ++++++++++++--------------
src/conf/domain_conf.h | 4 ++--
src/qemu/qemu_command.c | 4 ++--
src/qemu/qemu_domain.c | 2 +-
4 files changed, 17 insertions(+), 19 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 19884ec13..c1d549594 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -906,6 +906,7 @@ VIR_ENUM_IMPL(virDomainIOAPIC,
VIR_ENUM_IMPL(virDomainHPTResizing,
VIR_DOMAIN_HPT_RESIZING_LAST,
+ "none",
"enabled",
"disabled",
"required",
@@ -19239,14 +19240,13 @@ virDomainDefParseXML(xmlDocPtr xml,
tmp = virXMLPropString(nodes[i], "resizing");
if (tmp) {
int value = virDomainHPTResizingTypeFromString(tmp);
- if (value < 0) {
+ if (value < 0 || value == VIR_DOMAIN_HPT_RESIZING_NONE) {
Similar note as w/ previous and rng schema. Here too I'm fine with this
way, but if someone else isn't hopefully they speak up.
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("Unknown HPT resizing setting: %s"),
tmp);
goto error;
}
- def->hpt_resizing = value;
- def->features[val] = VIR_TRISTATE_SWITCH_ON;
+ def->features[val] = value;
VIR_FREE(tmp);
}
break;
@@ -21377,16 +21377,13 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src,
break;
case VIR_DOMAIN_FEATURE_HPT:
- if (src->features[i] != dst->features[i] ||
- src->hpt_resizing != dst->hpt_resizing) {
+ if (src->features[i] != dst->features[i]) {
virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("State of feature '%s:%s' differs: "
- "source: '%s:%s', destination:
'%s:%s'"),
+ "source: '%s', destination:
'%s'"),
featureName, "resizing",
- virTristateSwitchTypeToString(src->features[i]),
- virDomainHPTResizingTypeToString(src->hpt_resizing),
- virTristateSwitchTypeToString(dst->features[i]),
- virDomainHPTResizingTypeToString(dst->hpt_resizing));
+ virDomainHPTResizingTypeToString(src->features[i]),
+ virDomainHPTResizingTypeToString(dst->features[i]));
return false;
}
break;
@@ -26947,10 +26944,11 @@ virDomainDefFormatInternal(virDomainDefPtr def,
break;
case VIR_DOMAIN_FEATURE_HPT:
- if (def->features[i] == VIR_TRISTATE_SWITCH_ON) {
- virBufferAsprintf(buf, "<hpt
resizing='%s'/>\n",
-
virDomainHPTResizingTypeToString(def->hpt_resizing));
- }
+ if (def->features[i] == VIR_DOMAIN_HPT_RESIZING_NONE)
+ break;
+
+ virBufferAsprintf(buf, "<hpt
resizing='%s'/>\n",
+
virDomainHPTResizingTypeToString(def->features[i]));
break;
/* coverity[dead_error_begin] */
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 20f0efc36..4e9044ae6 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1871,7 +1871,8 @@ typedef enum {
VIR_ENUM_DECL(virDomainIOAPIC);
typedef enum {
- VIR_DOMAIN_HPT_RESIZING_ENABLED = 0,
+ VIR_DOMAIN_HPT_RESIZING_NONE = 0,
+ VIR_DOMAIN_HPT_RESIZING_ENABLED,
VIR_DOMAIN_HPT_RESIZING_DISABLED,
VIR_DOMAIN_HPT_RESIZING_REQUIRED,
@@ -2364,7 +2365,6 @@ struct _virDomainDef {
The comment a few lines above here should be updated to include:
* VIR_DOMAIN_FEATURE_HPT is of type virDomainHPTResizing
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
unsigned int hyperv_spinlocks;
virGICVersion gic_version;
char *hyperv_vendor_id;
- virDomainHPTResizing hpt_resizing;
/* These options are of type virTristateSwitch: ON = keep, OFF = drop */
int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
[...]
10:43 a.m.
New subject: [libvirt] [PATCH 11/11] tests: Clean up HPT tests
Give them better names and remove some redundancy.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
...es-hpt-resizing.args => pseries-features-hpt.args} | 1 -
.../pseries-features-hpt.xml} | 0
...chine.xml => pseries-features-invalid-machine.xml} | 2 +-
tests/qemuxml2argvdata/pseries-hpt-resizing.xml | 19 -------------------
tests/qemuxml2argvtest.c | 11 +++--------
tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 +
tests/qemuxml2xmltest.c | 3 +--
7 files changed, 6 insertions(+), 31 deletions(-)
rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args => pseries-features-hpt.args}
(96%)
rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml =>
qemuxml2argvdata/pseries-features-hpt.xml} (100%)
rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml =>
pseries-features-invalid-machine.xml} (86%)
delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml
create mode 120000 tests/qemuxml2xmloutdata/pseries-features-hpt.xml
diff --git a/tests/qemuxml2argvdata/pseries-hpt-resizing.args
b/tests/qemuxml2argvdata/pseries-features-hpt.args
similarity index 96%
rename from tests/qemuxml2argvdata/pseries-hpt-resizing.args
rename to tests/qemuxml2argvdata/pseries-features-hpt.args
index 994789a5e..8cdb32965 100644
--- a/tests/qemuxml2argvdata/pseries-hpt-resizing.args
+++ b/tests/qemuxml2argvdata/pseries-features-hpt.args
@@ -12,7 +12,6 @@ QEMU_AUDIO_DRV=none \
-smp 1,sockets=1,cores=1,threads=1 \
-uuid 1ccfd97d-5eb4-478a-bbe6-88d254c16db7 \
-nographic \
--nodefconfig \
-nodefaults \
-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-guest/monitor.sock,\
server,nowait \
diff --git a/tests/qemuxml2xmloutdata/pseries-hpt-resizing.xml
b/tests/qemuxml2argvdata/pseries-features-hpt.xml
similarity index 100%
rename from tests/qemuxml2xmloutdata/pseries-hpt-resizing.xml
rename to tests/qemuxml2argvdata/pseries-features-hpt.xml
diff --git a/tests/qemuxml2argvdata/pseries-hpt-resizing-invalid-machine.xml
b/tests/qemuxml2argvdata/pseries-features-invalid-machine.xml
similarity index 86%
rename from tests/qemuxml2argvdata/pseries-hpt-resizing-invalid-machine.xml
rename to tests/qemuxml2argvdata/pseries-features-invalid-machine.xml
index 757fcc70e..5a6bb02d5 100644
--- a/tests/qemuxml2argvdata/pseries-hpt-resizing-invalid-machine.xml
+++ b/tests/qemuxml2argvdata/pseries-features-invalid-machine.xml
@@ -7,7 +7,7 @@
<type arch='x86_64' machine='pc'>hvm</type>
</os>
<features>
- <!-- HPT resizing can't be enabled for non-pSeries guests -->
+ <!-- pSeries features can't be enabled for non-pSeries guests -->
<hpt resizing='enabled'/>
</features>
<devices>
diff --git a/tests/qemuxml2argvdata/pseries-hpt-resizing.xml
b/tests/qemuxml2argvdata/pseries-hpt-resizing.xml
deleted file mode 100644
index f9dc9cac9..000000000
--- a/tests/qemuxml2argvdata/pseries-hpt-resizing.xml
+++ /dev/null
@@ -1,19 +0,0 @@
-<domain type='qemu'>
- <name>guest</name>
- <uuid>1ccfd97d-5eb4-478a-bbe6-88d254c16db7</uuid>
- <memory unit='KiB'>524288</memory>
- <vcpu placement='static'>1</vcpu>
- <os>
- <type arch='ppc64' machine='pseries'>hvm</type>
- </os>
- <features>
- <!-- Explicitly enable HPT resizing. The guest will not start
- at all unless HPT resizing can be arranged -->
- <hpt resizing='required'/>
- </features>
- <devices>
- <emulator>/usr/bin/qemu-system-ppc64</emulator>
- <controller type='usb' model='none'/>
- <memballoon model='none'/>
- </devices>
-</domain>
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
index dd64772e2..3aa69fcee 100644
--- a/tests/qemuxml2argvtest.c
+++ b/tests/qemuxml2argvtest.c
@@ -1891,17 +1891,12 @@ mymain(void)
QEMU_CAPS_VIRTIO_SCSI,
QEMU_CAPS_DEVICE_VFIO_PCI);
- DO_TEST("pseries-hpt-resizing",
- QEMU_CAPS_NODEFCONFIG,
+ DO_TEST("pseries-features-hpt",
QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
- DO_TEST_FAILURE("pseries-hpt-resizing",
- QEMU_CAPS_NODEFCONFIG,
+ DO_TEST_FAILURE("pseries-features-hpt",
QEMU_CAPS_MACHINE_OPT);
- DO_TEST_PARSE_ERROR("pseries-hpt-resizing-invalid-machine",
- QEMU_CAPS_NODEFCONFIG,
- QEMU_CAPS_MACHINE_OPT,
- QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
+ DO_TEST_PARSE_ERROR("pseries-features-invalid-machine", NONE);
DO_TEST("pseries-serial-native",
QEMU_CAPS_NODEFCONFIG,
diff --git a/tests/qemuxml2xmloutdata/pseries-features-hpt.xml
b/tests/qemuxml2xmloutdata/pseries-features-hpt.xml
new file mode 120000
index 000000000..bcaf2e6fe
--- /dev/null
+++ b/tests/qemuxml2xmloutdata/pseries-features-hpt.xml
@@ -0,0 +1 @@
+../qemuxml2argvdata/pseries-features-hpt.xml
\ No newline at end of file
diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
index 83809574c..0eb9e6c77 100644
--- a/tests/qemuxml2xmltest.c
+++ b/tests/qemuxml2xmltest.c
@@ -763,8 +763,7 @@ mymain(void)
QEMU_CAPS_VIRTIO_SCSI,
QEMU_CAPS_DEVICE_VFIO_PCI);
- DO_TEST("pseries-hpt-resizing",
- QEMU_CAPS_NODEFCONFIG,
+ DO_TEST("pseries-features-hpt",
QEMU_CAPS_MACHINE_OPT,
QEMU_CAPS_MACHINE_PSERIES_RESIZE_HPT);
--
2.14.3
4:07 p.m.
New subject: [libvirt] [PATCH 11/11] tests: Clean up HPT tests
On 02/06/2018 11:43 AM, Andrea Bolognani wrote:
Give them better names and remove some redundancy.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
...es-hpt-resizing.args => pseries-features-hpt.args} | 1 -
.../pseries-features-hpt.xml} | 0
...chine.xml => pseries-features-invalid-machine.xml} | 2 +-
tests/qemuxml2argvdata/pseries-hpt-resizing.xml | 19 -------------------
tests/qemuxml2argvtest.c | 11 +++--------
tests/qemuxml2xmloutdata/pseries-features-hpt.xml | 1 +
tests/qemuxml2xmltest.c | 3 +--
7 files changed, 6 insertions(+), 31 deletions(-)
rename tests/qemuxml2argvdata/{pseries-hpt-resizing.args =>
pseries-features-hpt.args} (96%)
rename tests/{qemuxml2xmloutdata/pseries-hpt-resizing.xml =>
qemuxml2argvdata/pseries-features-hpt.xml} (100%)
rename tests/qemuxml2argvdata/{pseries-hpt-resizing-invalid-machine.xml =>
pseries-features-invalid-machine.xml} (86%)
delete mode 100644 tests/qemuxml2argvdata/pseries-hpt-resizing.xml
create mode 120000 tests/qemuxml2xmloutdata/pseries-features-hpt.xml
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
2550
days inactive
2557
days old
30 comments
2 participants
participants (2)
-
Andrea Bolognani -
John Ferlan