[libvirt] [PATCH 1/2] Introduce GIC feature
Michal Privoznik
mprivozn at redhat.com
Thu Apr 30 11:53:02 UTC 2015
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 at 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
More information about the libvir-list
mailing list