On 29.04.2015 13:31, John Ferlan wrote:
On 04/27/2015 09:07 AM, Michal Privoznik wrote:
Some platforms, like aarch64, don't have APIC but GIC. So there's no reason to have <apic/> feature turned on. However, we are
s/have/have the/ s/turned on/enabled/
still missing <gic/> feature. This commit introduces the feature to XML parser and formatter, adds documentation and updates RNG
s/to/to the/
schema.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- docs/formatdomain.html.in | 9 +++++++++ docs/schemas/domaincommon.rng | 11 ++++++++++- src/conf/domain_conf.c | 34 +++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 2 ++ 4 files changed, 54 insertions(+), 2 deletions(-)
Does aarch64 just ignore APIC? Makes me wonder how things work today... Is this also an architecture capability feature that needs to be added? IOW: How does one determine whether their architecture has GIC instead of APIC.
Correct. This is purely aarch64. Qemu just ignores APIC,
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e921749..27883fe 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -1401,6 +1401,7 @@ <hidden state='on'/> </kvm> <pvspinlock/> + <gic version='2'/>
</features> ...</pre> @@ -1501,6 +1502,14 @@ performance monitoring unit for the guest. <span class="since">Since 1.2.12</span> </dd> + <dt><code>gic</code></dt> + <dd>Some architectures don't have <code>APIC</code> but have + <code>GIC</code> <i>(Generic Interrupt Controller)</i>. For example + aarch64 is one of those architectures. If the guest belongs to the + set, you may want to turn on this feature instead of + <code>APIC</code>. Optional attribute <code>version</code> specifies + version of the controller, however may not be supported by all + hypervisors.</dd>
Enable for architectures using a General Interrupt Controller instead of APIC in order to handle interrupts. For example, the 'aarch64' architecture uses <code>gic</code> instead of <code>apic</code>. The optional attribute <code>version</code> specifies the GIC version; however, it may not be supported by all hypervisors. <span class="since">Since 1.2.16</span> </dd>
NOTE1: I didn't put <code> </code> around the capitalized GIC or APIC since to me that would infer the feature must be capitalized.
NOTE2: That last sentance is awkward - what would happen if it were supplied, but the hypervisor didn't support/recognize it?
NOTE3: I added the <span> and put the </dd> on a separate line like the previous <dd>...</dd> - not that it matters.
</dl>
<h3><a name="elementsTime">Time keeping</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 19461f5..b1d6f6b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3953,7 +3953,7 @@ </element> </define> <!-- - A set of optional features: PAE, APIC, ACPI, + A set of optional features: PAE, APIC, ACPI, GIC, HyperV Enlightenment, KVM features, paravirtual spinlocks and HAP support --> <define name="features"> @@ -4014,6 +4014,15 @@ <optional> <ref name="pmu"/> </optional> + <optional> + <element name="gic"> + <optional> + <attribute name="version"> + <ref name="positiveInteger"/> + </attribute> + </optional> + </element> + </optional> </interleave> </element> </optional> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 03710cb..466163e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -144,7 +144,8 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST, "kvm", "pvspinlock", "capabilities", - "pmu") + "pmu", + "gic")
VIR_ENUM_IMPL(virDomainCapabilitiesPolicy, VIR_DOMAIN_CAPABILITIES_POLICY_LAST, "default", @@ -14361,6 +14362,23 @@ virDomainDefParseXML(xmlDocPtr xml, ctxt->node = node; break;
+ case VIR_DOMAIN_FEATURE_GIC: + node = ctxt->node; + ctxt->node = nodes[i]; + if ((tmp = virXPathString("string(./@version)", ctxt))) { + if (virStrToLong_ui(tmp, NULL, 10, &def->gic_version) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("malformed gic version: %s"), tmp); + goto error; + } + VIR_FREE(tmp);
I think you wan to use virStrToLong_uip... and would probably need to check for an invalid value of zero (that way you can use that...)
+ } else { + def->gic_version = 2; /* By default, GIC version is 2 */
If not provided, then we're setting to the magic number of 2, which means when we format it will be written out. Is that expected/desired? I would think we should be able to handle things when the version is not supplied. Furthermore, since it's possible (from the docs) that a hypervisor doesn't support it, then setting a default could cause an issue.
Correct.
+ } + def->features[val] = VIR_TRISTATE_SWITCH_ON; + ctxt->node = node; + break; + /* coverity[dead_error_begin] */ case VIR_DOMAIN_FEATURE_LAST: break; @@ -16443,6 +16461,14 @@ virDomainDefFeaturesCheckABIStability(virDomainDefPtr src, return false; }
+ /* GIC version */ + if (src->gic_version != dst->gic_version) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Source GIC version '%u' does not match destination '%u'"), + src->gic_version, dst->gic_version); + return false; + } +
Obviously if the gic_version is enabled, so is gic; however, what if gic_
Er, what?
/* hyperv */ if (src->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { @@ -20996,6 +21022,12 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAddLit(buf, "</capabilities>\n"); break;
+ case VIR_DOMAIN_FEATURE_GIC: + if (def->features[i] == VIR_TRISTATE_SWITCH_ON) + virBufferAsprintf(buf, "<gic version='%u'/>\n", + def->gic_version); + break; +
Michal