Re: [PATCH 39/41] cpu: Rework cpuCompare* APIs

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

 




On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> Both cpuCompare* APIs are renamed to virCPUCompare*. And they should now
> work for any guest CPU definition, i.e., even for host-passthrough
> (trivial) and host-model CPUs. The implementation in x86 driver is
> enhanced to provide a hint about -noTSX Broadwell and Haswell models
> when appropriate.
> 
> Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> ---
>  src/cpu/cpu.c            | 37 ++++++++++++---------
>  src/cpu/cpu.h            | 21 ++++++------
>  src/cpu/cpu_arm.c        |  8 ++---
>  src/cpu/cpu_ppc64.c      | 15 +++++++--
>  src/cpu/cpu_x86.c        | 84 ++++++++++++++++++++++++++++++++++++------------
>  src/libvirt_private.syms |  4 +--
>  src/qemu/qemu_driver.c   | 14 ++------
>  tests/cputest.c          |  4 +--
>  8 files changed, 119 insertions(+), 68 deletions(-)
> 
> diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c
> index d6b0372..4ea192c 100644
> --- a/src/cpu/cpu.c
> +++ b/src/cpu/cpu.c
> @@ -91,8 +91,9 @@ cpuGetSubDriverByName(const char *name)
>  
>  
>  /**
> - * cpuCompareXML:
> + * virCPUCompareXML:
>   *
> + * @arch: CPU architecture
>   * @host: host CPU definition
>   * @xml: XML description of either guest or host CPU to be compared with @host

Existing, @failIncompatible description is missing

>   *
> @@ -104,25 +105,26 @@ cpuGetSubDriverByName(const char *name)
>   * the @host CPU.
>   */
>  virCPUCompareResult
> -cpuCompareXML(virCPUDefPtr host,
> -              const char *xml,
> -              bool failIncompatible)
> +virCPUCompareXML(virArch arch,
> +                 virCPUDefPtr host,
> +                 const char *xml,
> +                 bool failIncompatible)
>  {
>      xmlDocPtr doc = NULL;
>      xmlXPathContextPtr ctxt = NULL;
>      virCPUDefPtr cpu = NULL;
>      virCPUCompareResult ret = VIR_CPU_COMPARE_ERROR;
>  
> -    VIR_DEBUG("host=%p, xml=%s", host, NULLSTR(xml));
> +    VIR_DEBUG("arch=%s, host=%p, xml=%s",
> +              virArchToString(arch), host, NULLSTR(xml));

The prototype changed such that 'host' and 'xml' could be passed as NULL
without a compiler complaint (ok a static analysis complaint).  Was that
by design?

>  
>      if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), &ctxt)))

Having xml as NULL probably doesn't work well here.

>          goto cleanup;
>  
> -    cpu = virCPUDefParseXML(ctxt->node, ctxt, VIR_CPU_TYPE_AUTO);
> -    if (cpu == NULL)
> +    if (!(cpu = virCPUDefParseXML(ctxt->node, ctxt, VIR_CPU_TYPE_AUTO)))
>          goto cleanup;
>  
> -    ret = cpuCompare(host, cpu, failIncompatible);
> +    ret = virCPUCompare(arch, host, cpu, failIncompatible);

Allowing host to be NULL will cause failure in PPC and X86 which perhaps
is expected.

>  
>   cleanup:
>      virCPUDefFree(cpu);
> @@ -134,8 +136,9 @@ cpuCompareXML(virCPUDefPtr host,
>  
>  
>  /**
> - * cpuCompare:
> + * virCPUCompare:
>   *
> + * @arch: CPU architecture
>   * @host: host CPU definition
>   * @cpu: either guest or host CPU to be compared with @host
>   *
> @@ -147,21 +150,23 @@ cpuCompareXML(virCPUDefPtr host,
>   * the @host CPU.
>   */
>  virCPUCompareResult
> -cpuCompare(virCPUDefPtr host,
> -           virCPUDefPtr cpu,
> -           bool failIncompatible)
> +virCPUCompare(virArch arch,
> +              virCPUDefPtr host,
> +              virCPUDefPtr cpu,
> +              bool failIncompatible)
>  {
>      struct cpuArchDriver *driver;
>  
> -    VIR_DEBUG("host=%p, cpu=%p", host, cpu);
> +    VIR_DEBUG("arch=%s, host=%p, cpu=%p",
> +              virArchToString(arch), host, cpu);
>  
> -    if ((driver = cpuGetSubDriver(host->arch)) == NULL)
> +    if (!(driver = cpuGetSubDriver(arch)))
>          return VIR_CPU_COMPARE_ERROR;
>  
> -    if (driver->compare == NULL) {
> +    if (!driver->compare) {
>          virReportError(VIR_ERR_NO_SUPPORT,
>                         _("cannot compare CPUs of %s architecture"),
> -                       virArchToString(host->arch));
> +                       virArchToString(arch));
>          return VIR_CPU_COMPARE_ERROR;
>      }
>  
> diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h
> index 77ccb38..0e81e91 100644
> --- a/src/cpu/cpu.h
> +++ b/src/cpu/cpu.h
> @@ -45,7 +45,7 @@ struct _virCPUData {
>  
>  
>  typedef virCPUCompareResult
> -(*cpuArchCompare)   (virCPUDefPtr host,
> +(*virCPUArchCompare)(virCPUDefPtr host,
>                       virCPUDefPtr cpu,
>                       bool failIncompatible);
>  
> @@ -116,7 +116,7 @@ struct cpuArchDriver {
>      const char *name;
>      const virArch *arch;
>      unsigned int narch;
> -    cpuArchCompare      compare;
> +    virCPUArchCompare   compare;
>      cpuArchDecode       decode;
>      cpuArchEncode       encode;
>      cpuArchDataFree     free;
> @@ -134,16 +134,17 @@ struct cpuArchDriver {
>  
>  
>  virCPUCompareResult
> -cpuCompareXML(virCPUDefPtr host,
> -              const char *xml,
> -              bool failIncompatible)
> -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);

Was removing NONNULL by design?  I think it needs to be replaced for xml
at least.

> +virCPUCompareXML(virArch arch,
> +                 virCPUDefPtr host,
> +                 const char *xml,
> +                 bool failIncompatible);
>  
>  virCPUCompareResult
> -cpuCompare  (virCPUDefPtr host,
> -             virCPUDefPtr cpu,
> -             bool failIncompatible)
> -    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +virCPUCompare(virArch arch,
> +              virCPUDefPtr host,
> +              virCPUDefPtr cpu,
> +              bool failIncompatible)
> +    ATTRIBUTE_NONNULL(3);
>  
>  int
>  cpuDecode   (virCPUDefPtr cpu,
> diff --git a/src/cpu/cpu_arm.c b/src/cpu/cpu_arm.c
> index 3b68d83..2cef5bc 100644
> --- a/src/cpu/cpu_arm.c
> +++ b/src/cpu/cpu_arm.c
> @@ -112,9 +112,9 @@ armBaseline(virCPUDefPtr *cpus,
>  }
>  
>  static virCPUCompareResult
> -armCompare(virCPUDefPtr host ATTRIBUTE_UNUSED,
> -           virCPUDefPtr cpu ATTRIBUTE_UNUSED,
> -           bool failMessages ATTRIBUTE_UNUSED)
> +virCPUarmCompare(virCPUDefPtr host ATTRIBUTE_UNUSED,
> +                 virCPUDefPtr cpu ATTRIBUTE_UNUSED,
> +                 bool failMessages ATTRIBUTE_UNUSED)
>  {
>      return VIR_CPU_COMPARE_IDENTICAL;
>  }
> @@ -123,7 +123,7 @@ struct cpuArchDriver cpuDriverArm = {
>      .name = "arm",
>      .arch = archs,
>      .narch = ARRAY_CARDINALITY(archs),
> -    .compare = armCompare,
> +    .compare = virCPUarmCompare,
>      .decode = NULL,
>      .encode = NULL,
>      .free = armDataFree,
> diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c
> index 6f005e5..21ea0e1 100644
> --- a/src/cpu/cpu_ppc64.c
> +++ b/src/cpu/cpu_ppc64.c
> @@ -634,13 +634,24 @@ ppc64Compute(virCPUDefPtr host,
>  }
>  
>  static virCPUCompareResult
> -ppc64DriverCompare(virCPUDefPtr host,
> +virCPUppc64Compare(virCPUDefPtr host,
>                     virCPUDefPtr cpu,
>                     bool failIncompatible)
>  {
>      virCPUCompareResult ret;
>      char *message = NULL;
>  
> +    if (!host || !host->model) {
> +        if (failIncompatible) {
> +            virReportError(VIR_ERR_CPU_INCOMPATIBLE, "%s",
> +                           _("unknown host CPU"));
> +        } else {
> +            VIR_WARN("unknown host CPU");
> +            ret = VIR_CPU_COMPARE_INCOMPATIBLE;
> +        }
> +        return -1;
> +    }
> +
>      ret = ppc64Compute(host, cpu, NULL, &message);
>  
>      if (failIncompatible && ret == VIR_CPU_COMPARE_INCOMPATIBLE) {
> @@ -902,7 +913,7 @@ struct cpuArchDriver cpuDriverPPC64 = {
>      .name       = "ppc64",
>      .arch       = archs,
>      .narch      = ARRAY_CARDINALITY(archs),
> -    .compare    = ppc64DriverCompare,
> +    .compare    = virCPUppc64Compare,
>      .decode     = ppc64DriverDecode,
>      .encode     = NULL,
>      .free       = ppc64DriverFree,
> diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c
> index 782c917..e0987c4 100644
> --- a/src/cpu/cpu_x86.c
> +++ b/src/cpu/cpu_x86.c
> @@ -1057,7 +1057,8 @@ x86ModelFromCPU(const virCPUDef *cpu,
>          policy != -1)
>          return x86ModelNew();
>  
> -    if (policy == VIR_CPU_FEATURE_REQUIRE || policy == -1) {
> +    if (cpu->model &&
> +        (policy == VIR_CPU_FEATURE_REQUIRE || policy == -1)) {

Shall I assume this related to the compare patch? or a fix as a result?
I assume it's related to changes in x86Compute and x86GuestData, but it
wasn't really clear from the commit message.

>          if (!(model = x86ModelFind(map, cpu->model))) {
>              virReportError(VIR_ERR_INTERNAL_ERROR,
>                             _("Unknown CPU model %s"), cpu->model);

[...]

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]