[libvirt] [PATCH 09/11] conf: Improve IOAPIC feature handling
John Ferlan
jferlan at redhat.com
Mon Feb 12 21:59:35 UTC 2018
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 at 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 at 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"),
>
More information about the libvir-list
mailing list