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

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


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

Here's where I'd think that since gic_version is optional we'd have to
have a way to just print "<gic/>"; otherwise, it's not optional.


John
>              /* coverity[dead_error_begin] */
>              case VIR_DOMAIN_FEATURE_LAST:
>                  break;
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 9955052..a79f0b8 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
> @@ -1649,6 +1649,7 @@ typedef enum {
>      VIR_DOMAIN_FEATURE_PVSPINLOCK,
>      VIR_DOMAIN_FEATURE_CAPABILITIES,
>      VIR_DOMAIN_FEATURE_PMU,
> +    VIR_DOMAIN_FEATURE_GIC,
>  
>      VIR_DOMAIN_FEATURE_LAST
>  } virDomainFeature;
> @@ -2171,6 +2172,7 @@ struct _virDomainDef {
>      int hyperv_features[VIR_DOMAIN_HYPERV_LAST];
>      int kvm_features[VIR_DOMAIN_KVM_LAST];
>      unsigned int hyperv_spinlocks;
> +    unsigned int gic_version; /* by default 2 */
>  
>      /* These options are of type virTristateSwitch: ON = keep, OFF = drop */
>      int caps_features[VIR_DOMAIN_CAPS_FEATURE_LAST];
> 


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