Re: [libvirt] [PATCH 1/2] Introduce GIC feature

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(a)redhat.com&gt; > --- > 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.

> 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 @@ > &lt;hidden state='on'/&gt; > &lt;/kvm&gt; > &lt;pvspinlock/&gt; > + &lt;gic version='2'/&gt; > > &lt;/features&gt; > ...</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.

> + } > + 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_

> /* 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; > +