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

[Jiri Denemark]

On Tue, Aug 30, 2016 at 17:57:01 -0400, John Ferlan wrote:
...
> > - * 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

I added a separate patch to fix this.

> >   *
> > @@ -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?

host == NULL is definitely allowed by design, since we want to be able
to successfully compare CPUs on ARM where we are not able to detect the
host CPU model.

I'm not exactly sure about the xml argument, but since NULLSTR(xml) was
already here I figured this is a good place to return an error for xml
== NULL rather than forcing all callers to do this check. However I
apparently didn't implement the check :-)

> >  if (!(doc = virXMLParseStringCtxt(xml, _("(CPU_definition)"), )))
> 
> Having xml as NULL probably doesn't work well here.

I added an explicit check for xml == NULL.

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

Yes, although it will partially change in the future.

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

Yup, it was done by design.

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

Yes, it's related to this patch and required to do what commit message
says:

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

Thanks to this x86ModelFromCPU() may now be called for CPU definitions
without a model, e.g., CPUs with host-model mode.

Jirka

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

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

[John Ferlan]

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 
> ---
>  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)"), )))

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