Re: [libvirt] [PATCH 10/41] testutilsqemu: Helpers for changing host CPU and arch

[Jiri Denemark]

On Mon, Aug 29, 2016 at 12:45:03 -0400, John Ferlan wrote:
> 
> 
> On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> > Changing a host architecture or a CPU is not as easy as assigning a new
> > value to the appropriate element in virCaps since there is a relation
> > between the CPU and host architecture (we don't really want to test
> > anything on an AArch64 host with core2duo CPU). This patch introduces
> > qemuTestSetHostArch and qemuTestSetHostCPU helpers which will make sure
> > the host architecture matches the host CPU.
> > 
> > Signed-off-by: Jiri Denemark 
> > ---
> >  tests/testutilsqemu.c | 51 
> > ++-
> >  tests/testutilsqemu.h |  8 ++--
> >  2 files changed, 52 insertions(+), 7 deletions(-)
> > 
> > diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> > index 11dd20e..9b66101 100644
> > --- a/tests/testutilsqemu.c
> > +++ b/tests/testutilsqemu.c
> > @@ -16,6 +16,7 @@
> >  
> >  virCPUDefPtr cpuDefault;
> >  virCPUDefPtr cpuHaswell;
> > +virCPUDefPtr cpuPower8;
> 
> ^^
> Nothing in the commit message that indicates that we're also adding a
> power8 type.
> 
> Thus it would seem that this could be two commits - I would think adding
> the cpuPower8Data

Yes, I separated it to a new patch.

> [1] Also, should each of these should be initialized to NULL?

They are zero-initialized since they are static.

> >  
> >  static virCPUFeatureDef cpuDefaultFeatures[] = {
> >  { (char *) "lahf_lm",   -1 },
> > @@ -92,6 +93,15 @@ static virCPUDef cpuHaswellData = {
> >  cpuHaswellFeatures, /* features */
> >  };
> >  
> > +static virCPUDef cpuPower8Data = {
> > +.type = VIR_CPU_TYPE_HOST,
> > +.arch = VIR_ARCH_PPC64,
> > +.model = (char *) "POWER8",
> > +.sockets = 1,
> > +.cores = 8,
> > +.threads = 8,
> > +};
> > +
> >  static virCapsGuestMachinePtr *testQemuAllocMachines(int *nmachines)
> >  {
> >  virCapsGuestMachinePtr *machines;
> > @@ -331,7 +341,8 @@ virCapsPtr testQemuCapsInit(void)
> >  goto cleanup;
> >  
> >  if (!(cpuDefault = virCPUDefCopy()) ||
> > -!(cpuHaswell = virCPUDefCopy()))
> > +!(cpuHaswell = virCPUDefCopy()) ||
> > +!(cpuPower8 = virCPUDefCopy()))
> >  goto cleanup;
> >  
> >  caps->host.cpu = cpuDefault;
> 
> Should this change to a qemuTestSetHostCPU(caps, NULL) to work properly
> on power8?

It doesn't change much since caps is initialized a few lines earlier as

if (!(caps = virCapabilitiesNew(VIR_ARCH_X86_64, false, false)))
return NULL;

But using qemuTestSetHostCPU() to initialize the CPU is cleaner. Fixed.

...
> ACK 1-10 - just be sure to check/consider thoughts left in 2, 3, and
> here.  Of course we're post freeze now, so I guess there's a bit more
> waiting to do anyway.

You probably haven't noticed it, but this kind of sparse review with
accumulated ACKs scattered in random places of random replies is quite
hard to work with. Either an "ACK series" reply to a cover letter or
separate and standalone replies to each patch with review comments and a
conclusion (ack, nack, conditional ack) makes it a lot easier for the
author to process the review.

Jirka

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

Re: [libvirt] [PATCH 10/41] testutilsqemu: Helpers for changing host CPU and arch

[John Ferlan]

On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> Changing a host architecture or a CPU is not as easy as assigning a new
> value to the appropriate element in virCaps since there is a relation
> between the CPU and host architecture (we don't really want to test
> anything on an AArch64 host with core2duo CPU). This patch introduces
> qemuTestSetHostArch and qemuTestSetHostCPU helpers which will make sure
> the host architecture matches the host CPU.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  tests/testutilsqemu.c | 51 
> ++-
>  tests/testutilsqemu.h |  8 ++--
>  2 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
> index 11dd20e..9b66101 100644
> --- a/tests/testutilsqemu.c
> +++ b/tests/testutilsqemu.c
> @@ -16,6 +16,7 @@
>  
>  virCPUDefPtr cpuDefault;
>  virCPUDefPtr cpuHaswell;
> +virCPUDefPtr cpuPower8;

^^
Nothing in the commit message that indicates that we're also adding a
power8 type.

Thus it would seem that this could be two commits - I would think adding
the cpuPower8Data

[1] Also, should each of these should be initialized to NULL?

>  
>  static virCPUFeatureDef cpuDefaultFeatures[] = {
>  { (char *) "lahf_lm",   -1 },
> @@ -92,6 +93,15 @@ static virCPUDef cpuHaswellData = {
>  cpuHaswellFeatures, /* features */
>  };
>  
> +static virCPUDef cpuPower8Data = {
> +.type = VIR_CPU_TYPE_HOST,
> +.arch = VIR_ARCH_PPC64,
> +.model = (char *) "POWER8",
> +.sockets = 1,
> +.cores = 8,
> +.threads = 8,
> +};
> +
>  static virCapsGuestMachinePtr *testQemuAllocMachines(int *nmachines)
>  {
>  virCapsGuestMachinePtr *machines;
> @@ -331,7 +341,8 @@ virCapsPtr testQemuCapsInit(void)
>  goto cleanup;
>  
>  if (!(cpuDefault = virCPUDefCopy()) ||
> -!(cpuHaswell = virCPUDefCopy()))
> +!(cpuHaswell = virCPUDefCopy()) ||
> +!(cpuPower8 = virCPUDefCopy()))
>  goto cleanup;
>  
>  caps->host.cpu = cpuDefault;

Should this change to a qemuTestSetHostCPU(caps, NULL) to work properly
on power8?

> @@ -440,15 +451,45 @@ virCapsPtr testQemuCapsInit(void)
>  
>   cleanup:
>  virCapabilitiesFreeMachines(machines, nmachines);
> -if (caps->host.cpu != cpuDefault)
> -virCPUDefFree(cpuDefault);
> -if (caps->host.cpu != cpuHaswell)
> -virCPUDefFree(cpuHaswell);
> +caps->host.cpu = NULL;
> +virCPUDefFree(cpuDefault);
> +virCPUDefFree(cpuHaswell);
> +virCPUDefFree(cpuPower8);

[1]
Since we can get to cleanup without ever initializing any of these...


ACK 1-10 - just be sure to check/consider thoughts left in 2, 3, and
here.  Of course we're post freeze now, so I guess there's a bit more
waiting to do anyway.

John

>  virObjectUnref(caps);
>  return NULL;
>  }
>  
>  
> +void
> +qemuTestSetHostArch(virCapsPtr caps,
> +virArch arch)
> +{
> +if (arch == VIR_ARCH_NONE)
> +arch = VIR_ARCH_X86_64;
> +caps->host.arch = arch;
> +qemuTestSetHostCPU(caps, NULL);
> +}
> +
> +
> +void
> +qemuTestSetHostCPU(virCapsPtr caps,
> +   virCPUDefPtr cpu)
> +{
> +virArch arch = caps->host.arch;
> +
> +if (!cpu) {
> +if (ARCH_IS_X86(arch))
> +cpu = cpuDefault;
> +else if (ARCH_IS_PPC64(arch))
> +cpu = cpuPower8;
> +}
> +
> +if (cpu)
> +caps->host.arch = cpu->arch;
> +caps->host.cpu = cpu;
> +}
> +
> +
>  virQEMUCapsPtr
>  qemuTestParseCapabilities(const char *capsFile)
>  {
> diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h
> index f2b71e9..6d35116f 100644
> --- a/tests/testutilsqemu.h
> +++ b/tests/testutilsqemu.h
> @@ -19,8 +19,12 @@ virQEMUCapsPtr qemuTestParseCapabilities(const char 
> *capsFile);
>  
>  extern virCPUDefPtr cpuDefault;
>  extern virCPUDefPtr cpuHaswell;
> -void testQemuCapsSetCPU(virCapsPtr caps,
> -virCPUDefPtr hostCPU);
> +extern virCPUDefPtr cpuPower8;
> +
> +void qemuTestSetHostArch(virCapsPtr caps,
> +virArch arch);
> +void qemuTestSetHostCPU(virCapsPtr caps,
> +virCPUDefPtr cpu);
>  
>  int qemuTestDriverInit(virQEMUDriver *driver);
>  void qemuTestDriverFree(virQEMUDriver *driver);
> 

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

[libvirt] [PATCH 10/41] testutilsqemu: Helpers for changing host CPU and arch

[Jiri Denemark]

Changing a host architecture or a CPU is not as easy as assigning a new
value to the appropriate element in virCaps since there is a relation
between the CPU and host architecture (we don't really want to test
anything on an AArch64 host with core2duo CPU). This patch introduces
qemuTestSetHostArch and qemuTestSetHostCPU helpers which will make sure
the host architecture matches the host CPU.

Signed-off-by: Jiri Denemark 
---
 tests/testutilsqemu.c | 51 ++-
 tests/testutilsqemu.h |  8 ++--
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c
index 11dd20e..9b66101 100644
--- a/tests/testutilsqemu.c
+++ b/tests/testutilsqemu.c
@@ -16,6 +16,7 @@
 
 virCPUDefPtr cpuDefault;
 virCPUDefPtr cpuHaswell;
+virCPUDefPtr cpuPower8;
 
 static virCPUFeatureDef cpuDefaultFeatures[] = {
 { (char *) "lahf_lm",   -1 },
@@ -92,6 +93,15 @@ static virCPUDef cpuHaswellData = {
 cpuHaswellFeatures, /* features */
 };
 
+static virCPUDef cpuPower8Data = {
+.type = VIR_CPU_TYPE_HOST,
+.arch = VIR_ARCH_PPC64,
+.model = (char *) "POWER8",
+.sockets = 1,
+.cores = 8,
+.threads = 8,
+};
+
 static virCapsGuestMachinePtr *testQemuAllocMachines(int *nmachines)
 {
 virCapsGuestMachinePtr *machines;
@@ -331,7 +341,8 @@ virCapsPtr testQemuCapsInit(void)
 goto cleanup;
 
 if (!(cpuDefault = virCPUDefCopy()) ||
-!(cpuHaswell = virCPUDefCopy()))
+!(cpuHaswell = virCPUDefCopy()) ||
+!(cpuPower8 = virCPUDefCopy()))
 goto cleanup;
 
 caps->host.cpu = cpuDefault;
@@ -440,15 +451,45 @@ virCapsPtr testQemuCapsInit(void)
 
  cleanup:
 virCapabilitiesFreeMachines(machines, nmachines);
-if (caps->host.cpu != cpuDefault)
-virCPUDefFree(cpuDefault);
-if (caps->host.cpu != cpuHaswell)
-virCPUDefFree(cpuHaswell);
+caps->host.cpu = NULL;
+virCPUDefFree(cpuDefault);
+virCPUDefFree(cpuHaswell);
+virCPUDefFree(cpuPower8);
 virObjectUnref(caps);
 return NULL;
 }
 
 
+void
+qemuTestSetHostArch(virCapsPtr caps,
+virArch arch)
+{
+if (arch == VIR_ARCH_NONE)
+arch = VIR_ARCH_X86_64;
+caps->host.arch = arch;
+qemuTestSetHostCPU(caps, NULL);
+}
+
+
+void
+qemuTestSetHostCPU(virCapsPtr caps,
+   virCPUDefPtr cpu)
+{
+virArch arch = caps->host.arch;
+
+if (!cpu) {
+if (ARCH_IS_X86(arch))
+cpu = cpuDefault;
+else if (ARCH_IS_PPC64(arch))
+cpu = cpuPower8;
+}
+
+if (cpu)
+caps->host.arch = cpu->arch;
+caps->host.cpu = cpu;
+}
+
+
 virQEMUCapsPtr
 qemuTestParseCapabilities(const char *capsFile)
 {
diff --git a/tests/testutilsqemu.h b/tests/testutilsqemu.h
index f2b71e9..6d35116f 100644
--- a/tests/testutilsqemu.h
+++ b/tests/testutilsqemu.h
@@ -19,8 +19,12 @@ virQEMUCapsPtr qemuTestParseCapabilities(const char 
*capsFile);
 
 extern virCPUDefPtr cpuDefault;
 extern virCPUDefPtr cpuHaswell;
-void testQemuCapsSetCPU(virCapsPtr caps,
-virCPUDefPtr hostCPU);
+extern virCPUDefPtr cpuPower8;
+
+void qemuTestSetHostArch(virCapsPtr caps,
+virArch arch);
+void qemuTestSetHostCPU(virCapsPtr caps,
+virCPUDefPtr cpu);
 
 int qemuTestDriverInit(virQEMUDriver *driver);
 void qemuTestDriverFree(virQEMUDriver *driver);
-- 
2.9.2

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