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

Re: [libvirt] [PATCH 1/2] Add support for EOI with APIC



On 09/13/2012 04:52 PM, Michal Privoznik wrote:
> On 13.09.2012 16:12, Martin Kletzander wrote:
>> New options is added to support EOI (End of Interrupt) exposure for
>> guests. As it makes sense only when APIC is enabled, I added this into
>> the <apic> element in <features> because this should be tri-state
>> option (cannot be handled as standalone feature).
>> ---
>>  docs/formatdomain.html.in     |  7 +++++++
>>  docs/schemas/domaincommon.rng |  9 ++++++++-
>>  src/conf/domain_conf.c        | 35 ++++++++++++++++++++++++++++++++++-
>>  src/conf/domain_conf.h        | 11 +++++++++++
>>  src/libvirt_private.syms      |  2 ++
>>  5 files changed, 62 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 503685f..66319d0 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -1018,6 +1018,13 @@
>>        <dd>ACPI is useful for power management, for example, with
>>          KVM guests it is required for graceful shutdown to work.
>>        </dd>
>> +      <dt><code>apic</code></dt>
>> +      <dd>APIC allows the use of programmable IRQ
>> +      management. <span class="since">Since 0.10.2 (QEMU only)</span>
>> +      there is an optional attribute <code>eoi</code> with values "on"
>> +      and "off" which toggle the availability of EOI (End of
> 
> s/toggle/toggles/
> 
>> +      Interrupt) for the guest.
>> +      </dd>
>>        <dt><code>hap</code></dt>
>>        <dd>Enable use of Hardware Assisted Paging if available in
>>          the hardware.
> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 292cc9a..89c08da 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -110,6 +110,11 @@ VIR_ENUM_IMPL(virDomainFeature, VIR_DOMAIN_FEATURE_LAST,
>>                "viridian",
>>                "privnet")
>>
>> +VIR_ENUM_IMPL(virDomainApicEoi, VIR_DOMAIN_APIC_EOI_LAST,
>> +              "default",
>> +              "on",
>> +              "off")
>> +
>>  VIR_ENUM_IMPL(virDomainLifecycle, VIR_DOMAIN_LIFECYCLE_LAST,
>>                "destroy",
>>                "restart",
>> @@ -8621,6 +8626,28 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>>                  goto error;
>>              }
>>              def->features |= (1 << val);
>> +            if (val == VIR_DOMAIN_FEATURE_APIC) {
>> +                char *attrstr = NULL;
>> +                if (virAsprintf(&attrstr,
>> +                                "string(./features/%s/@eoi)",
>> +                                nodes[i]->name) < 0)
>> +                    goto no_memory;
>> +
> 
> Why do we want runtime constructed string esp. if it is static one?
> 
>> +                tmp = virXPathString(attrstr, ctxt);
>> +                if (tmp) {
>> +                    int eoi;
>> +                    if ((eoi = virDomainApicEoiTypeFromString(tmp)) < 0) {
>> +                        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> +                                       _("unknown value for attribute eoi: %s"),
>> +                                       nodes[i]->name);
> 
> s/nodes[i]->name/tmp/
> 
>> +                        VIR_FREE(tmp);
>> +                        goto error;
>> +                    }
>> +                    def->apic_eoi = eoi;
>> +                    VIR_FREE(tmp);
>> +                }
>> +                VIR_FREE(attrstr);
>> +            }
>>          }
>>          VIR_FREE(nodes);
>>      }
> 
> ACK with this squashed in:
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index d13f671..16afb15 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8846,26 +8846,19 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
>              }
>              def->features |= (1 << val);
>              if (val == VIR_DOMAIN_FEATURE_APIC) {
> -                char *attrstr = NULL;
> -                if (virAsprintf(&attrstr,
> -                                "string(./features/%s/@eoi)",
> -                                nodes[i]->name) < 0)
> -                    goto no_memory;
> -
> -                tmp = virXPathString(attrstr, ctxt);
> +                tmp = virXPathString("string(./features/apic/@eoi)", ctxt);
>                  if (tmp) {
>                      int eoi;
> -                    if ((eoi = virDomainApicEoiTypeFromString(tmp)) < 0) {
> +                    if ((eoi = virDomainApicEoiTypeFromString(tmp)) <= 0) {
>                          virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>                                         _("unknown value for attribute eoi: %s"),
> -                                       nodes[i]->name);
> +                                       tmp);
>                          VIR_FREE(tmp);
>                          goto error;
>                      }
>                      def->apic_eoi = eoi;
>                      VIR_FREE(tmp);
>                  }
> -                VIR_FREE(attrstr);
>              }
>          }
>          VIR_FREE(nodes);
> 
> 
> Michal
> 

Thanks guys, fushed with the fixes,

Martin


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