[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 @@
>>        &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.

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]