Re: [libvirt] [PATCH 11/41] qemu: Separate guest CPU validation from command line creation

[Jiri Denemark]

On Mon, Aug 29, 2016 at 15:21:43 -0400, John Ferlan wrote:
...
> > -driver.caps->host.cpu->arch = VIR_ARCH_AARCH64;
> > +qemuTestSetHostArch(driver.caps, VIR_ARCH_AARCH64);
> >  DO_TEST("aarch64-kvm-32-on-64",
> >  QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
> >  QEMU_CAPS_KVM, QEMU_CAPS_CPU_AARCH64_OFF);
> >  DO_TEST_FAILURE("aarch64-kvm-32-on-64",
> >  QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
> >  QEMU_CAPS_KVM);
> > -driver.caps->host.cpu->arch = cpuDefault->arch;
> > +qemuTestSetHostArch(driver.caps, VIR_ARCH_NONE);
> 
> Adding this one causes failure... Perhaps due to the:
> 
> +if (cpu)
> +caps->host.arch = cpu->arch;
> 
> in previous patch?  or perhaps some shortly subsequent patch...
> 
> It's not so much an objection to the patch, but by simply adding those
> API's in patch 10 I guess I would have expected them to just work, but
> trial and error proves otherwise. Of course reading future patches
> perhaps sheds even more light (or black magic, voodoo) ;-).

The test suite was heavily relying on bugs in our code, which means both
need to be fixed at the same time, otherwise the tests would just break.

Jirka

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

Re: [libvirt] [PATCH 11/41] qemu: Separate guest CPU validation from command line creation

[John Ferlan]

On 08/12/2016 09:33 AM, Jiri Denemark wrote:
> qemu_command.c should deal with translating our domain definition into a
> QEMU command line and nothing else.
> 
> Signed-off-by: Jiri Denemark 
> ---
>  src/qemu/qemu_command.c  |  71 
>  src/qemu/qemu_process.c  | 105 
> +++
>  tests/qemuxml2argvtest.c |   8 ++--
>  3 files changed, 117 insertions(+), 67 deletions(-)
> 

[...]

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index f9ed6f5..2b94ff0 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -1394,13 +1394,13 @@ mymain(void)
>  DO_TEST_FAILURE("cpu-host-passthrough", NONE);
>  DO_TEST_FAILURE("cpu-qemu-host-passthrough", QEMU_CAPS_KVM);
>  
> -driver.caps->host.cpu = cpuHaswell;
> +qemuTestSetHostCPU(driver.caps, cpuHaswell);
>  DO_TEST("cpu-Haswell", QEMU_CAPS_KVM);
>  DO_TEST("cpu-Haswell2", QEMU_CAPS_KVM);
>  DO_TEST("cpu-Haswell3", QEMU_CAPS_KVM);
>  DO_TEST("cpu-Haswell-noTSX", QEMU_CAPS_KVM);
>  DO_TEST("cpu-host-model-cmt", NONE);
> -driver.caps->host.cpu = cpuDefault;
> +qemuTestSetHostCPU(driver.caps, NULL);

Adding this to the previous patch seems to work, but...

>  
>  DO_TEST("encrypted-disk", NONE);
>  DO_TEST("encrypted-disk-usage", NONE);
> @@ -1947,14 +1947,14 @@ mymain(void)
>  QEMU_CAPS_KVM, QEMU_CAPS_MACHINE_OPT,
>  QEMU_CAPS_MACH_VIRT_GIC_VERSION);
>  
> -driver.caps->host.cpu->arch = VIR_ARCH_AARCH64;
> +qemuTestSetHostArch(driver.caps, VIR_ARCH_AARCH64);
>  DO_TEST("aarch64-kvm-32-on-64",
>  QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
>  QEMU_CAPS_KVM, QEMU_CAPS_CPU_AARCH64_OFF);
>  DO_TEST_FAILURE("aarch64-kvm-32-on-64",
>  QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_DEVICE_VIRTIO_MMIO,
>  QEMU_CAPS_KVM);
> -driver.caps->host.cpu->arch = cpuDefault->arch;
> +qemuTestSetHostArch(driver.caps, VIR_ARCH_NONE);

Adding this one causes failure... Perhaps due to the:

+if (cpu)
+caps->host.arch = cpu->arch;

in previous patch?  or perhaps some shortly subsequent patch...

It's not so much an objection to the patch, but by simply adding those
API's in patch 10 I guess I would have expected them to just work, but
trial and error proves otherwise. Of course reading future patches
perhaps sheds even more light (or black magic, voodoo) ;-).

>  
>  DO_TEST("kvm-pit-device", QEMU_CAPS_KVM_PIT_TICK_POLICY);
>  DO_TEST("kvm-pit-delay", QEMU_CAPS_NO_KVM_PIT);
> 

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

[libvirt] [PATCH 11/41] qemu: Separate guest CPU validation from command line creation

[Jiri Denemark]

qemu_command.c should deal with translating our domain definition into a
QEMU command line and nothing else.

Signed-off-by: Jiri Denemark 
---
 src/qemu/qemu_command.c  |  71 
 src/qemu/qemu_process.c  | 105 +++
 tests/qemuxml2argvtest.c |   8 ++--
 3 files changed, 117 insertions(+), 67 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 3f542bc..823b58d 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -6445,9 +6445,6 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 size_t ncpus = 0;
 char **cpus = NULL;
 virCPUDataPtr data = NULL;
-virCPUDataPtr hostData = NULL;
-char *compare_msg = NULL;
-virCPUCompareResult cmp;
 const char *preferred;
 virCapsPtr caps = NULL;
 bool compareAgainstHost = ((def->virtType == VIR_DOMAIN_VIRT_KVM ||
@@ -6459,15 +6456,6 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 
 host = caps->host.cpu;
 
-if (virQEMUCapsGetCPUDefinitions(qemuCaps, , ) < 0)
-goto cleanup;
-
-if (!host || !host->model || ncpus == 0) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("CPU specification not supported by hypervisor"));
-goto cleanup;
-}
-
 if (!(cpu = virCPUDefCopy(def->cpu)))
 goto cleanup;
 
@@ -6476,53 +6464,9 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 cpuUpdate(cpu, host) < 0)
 goto cleanup;
 
-/* For non-KVM, CPU features are emulated, so host compat doesn't matter */
-if (compareAgainstHost) {
-bool noTSX = false;
-
-cmp = cpuGuestData(host, cpu, , _msg);
-switch (cmp) {
-case VIR_CPU_COMPARE_INCOMPATIBLE:
-if (cpuEncode(host->arch, host, NULL, ,
-  NULL, NULL, NULL, NULL) == 0 &&
-(!cpuHasFeature(hostData, "hle") ||
- !cpuHasFeature(hostData, "rtm")) &&
-(STREQ_NULLABLE(cpu->model, "Haswell") ||
- STREQ_NULLABLE(cpu->model, "Broadwell")))
-noTSX = true;
-
-if (compare_msg) {
-if (noTSX) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("guest and host CPU are not compatible: "
- "%s; try using '%s-noTSX' CPU model"),
-   compare_msg, cpu->model);
-} else {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("guest and host CPU are not compatible: "
- "%s"),
-   compare_msg);
-}
-} else {
-if (noTSX) {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-   _("guest CPU is not compatible with host "
- "CPU; try using '%s-noTSX' CPU model"),
-   cpu->model);
-} else {
-virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
-   _("guest CPU is not compatible with host "
- "CPU"));
-}
-}
-/* fall through */
-case VIR_CPU_COMPARE_ERROR:
-goto cleanup;
-
-default:
-break;
-}
-}
+if (compareAgainstHost &&
+cpuGuestData(host, cpu, , NULL) == VIR_CPU_COMPARE_ERROR)
+goto cleanup;
 
 /* Only 'svm' requires --enable-nesting. The nested
  * 'vmx' patches now simply hook off the CPU features
@@ -6547,7 +6491,7 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 virBufferAddLit(buf, "host");
 
 if (def->os.arch == VIR_ARCH_ARMV7L &&
-host->arch == VIR_ARCH_AARCH64) {
+caps->host.arch == VIR_ARCH_AARCH64) {
 if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_CPU_AARCH64_OFF)) {
 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
_("QEMU binary does not support CPU "
@@ -6566,7 +6510,6 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 } else {
 featCpu = cpu;
 }
-
 } else {
 if (VIR_ALLOC(guest) < 0)
 goto cleanup;
@@ -6582,6 +6525,10 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 
 guest->type = VIR_CPU_TYPE_GUEST;
 guest->fallback = cpu->fallback;
+
+if (virQEMUCapsGetCPUDefinitions(qemuCaps, , ) < 0)
+goto cleanup;
+
 if (cpuDecode(guest, data,
   (const char **)cpus, ncpus, preferred) < 0)
 goto cleanup;
@@ -6611,9 +6558,7 @@ qemuBuildCpuModelArgStr(virQEMUDriverPtr driver,
 ret = 0;
  cleanup:
 virObjectUnref(caps);
-VIR_FREE(compare_msg);