[libvirt] [PATCH 0/3] qemu: command: clean up default cpu formatting
Just some minor patches to do with encoding a default CPU on the qemu command line. Patch 1 adds a test case Patch 2 simplifies the code Patch 3 throws an explicit error for a confusing situation Cole Robinson (3): tests: add qemu x86 kvm 32-on-64 test qemu: command: rework adding of default cpu model qemu: command: explicitly error for non-x86 default CPU src/qemu/qemu_command.c | 95 +++++++++------------- .../qemuxml2argvdata/qemuxml2argv-hyperv-off.args | 1 - .../qemuxml2argv-kvm-features-off.args | 1 - .../qemuxml2argv-x86-kvm-32-on-64.args | 21 +++++ .../qemuxml2argv-x86-kvm-32-on-64.xml | 13 +++ tests/qemuxml2argvtest.c | 1 + tests/testutilsqemu.c | 18 +++- 7 files changed, 90 insertions(+), 60 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml -- 2.13.3
There's some specific logic in qemuBuildCpuCommandLine to support auto adding -cpu qemu 32 for arch=i686 with an x86_64 qemu-kvm binary. Add a test case for it Signed-off-by: Cole Robinson <crobinso@redhat.com> --- .../qemuxml2argv-x86-kvm-32-on-64.args | 21 +++++++++++++++++++++ .../qemuxml2argv-x86-kvm-32-on-64.xml | 13 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/testutilsqemu.c | 18 ++++++++++++++++-- 4 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args new file mode 100644 index 000000000..5fdeaf843 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-kvm \ +-name kvm \ +-S \ +-machine pc,accel=kvm \ +-cpu qemu32 \ +-m 4096 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid d091ea82-29e6-2e34-3005-f02617b36e87 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-kvm/monitor.sock,server,\ +nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml new file mode 100644 index 000000000..2939cec15 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml @@ -0,0 +1,13 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <os> + <type arch='i686'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 302c9c892..ef5a9b0dc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -690,6 +690,7 @@ mymain(void) DO_TEST("kvm", QEMU_CAPS_MACHINE_OPT); DO_TEST("default-kvm-host-arch", QEMU_CAPS_MACHINE_OPT); DO_TEST("default-qemu-host-arch", QEMU_CAPS_MACHINE_OPT); + DO_TEST("x86-kvm-32-on-64", QEMU_CAPS_MACHINE_OPT); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); DO_TEST("boot-floppy", NONE); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index ee4853841..d1290fdde 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -111,7 +111,8 @@ typedef enum { TEST_UTILS_QEMU_BIN_ARM, TEST_UTILS_QEMU_BIN_PPC64, TEST_UTILS_QEMU_BIN_PPC, - TEST_UTILS_QEMU_BIN_S390X + TEST_UTILS_QEMU_BIN_S390X, + TEST_UTILS_QEMU_BIN_KVM, } QEMUBinType; static const char *QEMUBinList[] = { @@ -121,7 +122,8 @@ static const char *QEMUBinList[] = { "/usr/bin/qemu-system-arm", "/usr/bin/qemu-system-ppc64", "/usr/bin/qemu-system-ppc", - "/usr/bin/qemu-system-s390x" + "/usr/bin/qemu-system-s390x", + "/usr/bin/qemu-kvm", }; @@ -215,6 +217,18 @@ testQemuAddI686Guest(virCapsPtr caps) machines)) goto error; + machines = NULL; + if (!(machines = testQemuAllocMachines(&nmachines))) + goto error; + + if (!virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_KVM, + QEMUBinList[TEST_UTILS_QEMU_BIN_KVM], + NULL, + nmachines, + machines)) + goto error; + return 0; error: -- 2.13.3
On Fri, Jul 14, 2017 at 07:43:04PM -0400, Cole Robinson wrote:
There's some specific logic in qemuBuildCpuCommandLine to support auto adding -cpu qemu 32 for arch=i686 with an x86_64 qemu-kvm binary. Add a test case for it
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- .../qemuxml2argv-x86-kvm-32-on-64.args | 21 +++++++++++++++++++++ .../qemuxml2argv-x86-kvm-32-on-64.xml | 13 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/testutilsqemu.c | 18 ++++++++++++++++-- 4 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args new file mode 100644 index 000000000..5fdeaf843 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-kvm \ +-name kvm \ +-S \ +-machine pc,accel=kvm \ +-cpu qemu32 \ +-m 4096 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid d091ea82-29e6-2e34-3005-f02617b36e87 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-kvm/monitor.sock,server,\ +nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml new file mode 100644 index 000000000..2939cec15 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml @@ -0,0 +1,13 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <os> + <type arch='i686'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 302c9c892..ef5a9b0dc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -690,6 +690,7 @@ mymain(void) DO_TEST("kvm", QEMU_CAPS_MACHINE_OPT); DO_TEST("default-kvm-host-arch", QEMU_CAPS_MACHINE_OPT); DO_TEST("default-qemu-host-arch", QEMU_CAPS_MACHINE_OPT); + DO_TEST("x86-kvm-32-on-64", QEMU_CAPS_MACHINE_OPT); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); DO_TEST("boot-floppy", NONE); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index ee4853841..d1290fdde 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -111,7 +111,8 @@ typedef enum { TEST_UTILS_QEMU_BIN_ARM, TEST_UTILS_QEMU_BIN_PPC64, TEST_UTILS_QEMU_BIN_PPC, - TEST_UTILS_QEMU_BIN_S390X + TEST_UTILS_QEMU_BIN_S390X, + TEST_UTILS_QEMU_BIN_KVM, } QEMUBinType;
static const char *QEMUBinList[] = { @@ -121,7 +122,8 @@ static const char *QEMUBinList[] = { "/usr/bin/qemu-system-arm", "/usr/bin/qemu-system-ppc64", "/usr/bin/qemu-system-ppc", - "/usr/bin/qemu-system-s390x" + "/usr/bin/qemu-system-s390x", + "/usr/bin/qemu-kvm",
I don't like this patch because it adds a binary that is specific to downstream releases. The whole point of my previous patches was to remove all downstream binaries from our test suite. This test can be easily done with the current x86_64 binary TEST_UTILS_QEMU_BIN_X86_64. Pavel
};
@@ -215,6 +217,18 @@ testQemuAddI686Guest(virCapsPtr caps) machines)) goto error;
+ machines = NULL; + if (!(machines = testQemuAllocMachines(&nmachines))) + goto error; + + if (!virCapabilitiesAddGuestDomain(guest, + VIR_DOMAIN_VIRT_KVM, + QEMUBinList[TEST_UTILS_QEMU_BIN_KVM], + NULL, + nmachines, + machines)) + goto error; + return 0;
error: -- 2.13.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 07/21/2017 07:40 AM, Pavel Hrdina wrote:
On Fri, Jul 14, 2017 at 07:43:04PM -0400, Cole Robinson wrote:
There's some specific logic in qemuBuildCpuCommandLine to support auto adding -cpu qemu 32 for arch=i686 with an x86_64 qemu-kvm binary. Add a test case for it
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- .../qemuxml2argv-x86-kvm-32-on-64.args | 21 +++++++++++++++++++++ .../qemuxml2argv-x86-kvm-32-on-64.xml | 13 +++++++++++++ tests/qemuxml2argvtest.c | 1 + tests/testutilsqemu.c | 18 ++++++++++++++++-- 4 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args new file mode 100644 index 000000000..5fdeaf843 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.args @@ -0,0 +1,21 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-kvm \ +-name kvm \ +-S \ +-machine pc,accel=kvm \ +-cpu qemu32 \ +-m 4096 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid d091ea82-29e6-2e34-3005-f02617b36e87 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-kvm/monitor.sock,server,\ +nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c diff --git a/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml new file mode 100644 index 000000000..2939cec15 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-x86-kvm-32-on-64.xml @@ -0,0 +1,13 @@ +<domain type='kvm'> + <name>kvm</name> + <uuid>d091ea82-29e6-2e34-3005-f02617b36e87</uuid> + <memory unit='KiB'>4194304</memory> + <os> + <type arch='i686'>hvm</type> + </os> + <devices> + <emulator>/usr/bin/qemu-kvm</emulator> + <controller type='usb' model='none'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 302c9c892..ef5a9b0dc 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -690,6 +690,7 @@ mymain(void) DO_TEST("kvm", QEMU_CAPS_MACHINE_OPT); DO_TEST("default-kvm-host-arch", QEMU_CAPS_MACHINE_OPT); DO_TEST("default-qemu-host-arch", QEMU_CAPS_MACHINE_OPT); + DO_TEST("x86-kvm-32-on-64", QEMU_CAPS_MACHINE_OPT); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); DO_TEST("boot-floppy", NONE); diff --git a/tests/testutilsqemu.c b/tests/testutilsqemu.c index ee4853841..d1290fdde 100644 --- a/tests/testutilsqemu.c +++ b/tests/testutilsqemu.c @@ -111,7 +111,8 @@ typedef enum { TEST_UTILS_QEMU_BIN_ARM, TEST_UTILS_QEMU_BIN_PPC64, TEST_UTILS_QEMU_BIN_PPC, - TEST_UTILS_QEMU_BIN_S390X + TEST_UTILS_QEMU_BIN_S390X, + TEST_UTILS_QEMU_BIN_KVM, } QEMUBinType;
static const char *QEMUBinList[] = { @@ -121,7 +122,8 @@ static const char *QEMUBinList[] = { "/usr/bin/qemu-system-arm", "/usr/bin/qemu-system-ppc64", "/usr/bin/qemu-system-ppc", - "/usr/bin/qemu-system-s390x" + "/usr/bin/qemu-system-s390x", + "/usr/bin/qemu-kvm",
I don't like this patch because it adds a binary that is specific to downstream releases. The whole point of my previous patches was to remove all downstream binaries from our test suite.
This test can be easily done with the current x86_64 binary TEST_UTILS_QEMU_BIN_X86_64.
I thought the qemu-kvm patch was required to trigger the logic, but indeed it's not necessary. I'll send a v2 Thanks, Cole
Certain XML features that aren't in the <cpu> block map to -cpu flags on the qemu cli. If one of these is specified but the user didn't explicitly pass an XML <cpu> model, we need to format a default model on the command line. The current code handles this by sprinkling this default cpu handling among all the different flag string formatting. Instead, switch it to do this just once. This alters some test output slightly: the previous code would write the default -cpu in some cases when no flags were actually added, so the output was redundant. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 72 +++++++--------------- .../qemuxml2argvdata/qemuxml2argv-hyperv-off.args | 1 - .../qemuxml2argv-kvm-features-off.args | 1 - 3 files changed, 22 insertions(+), 52 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b83261246..aa12479f7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6714,11 +6714,11 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, virQEMUCapsPtr qemuCaps) { virArch hostarch = virArchFromHost(); - char *cpu = NULL; + char *cpu = NULL, *cpu_flags = NULL; bool hasHwVirt = false; const char *default_model; - bool have_cpu = false; int ret = -1; + virBuffer cpu_buf = VIR_BUFFER_INITIALIZER; virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; @@ -6729,9 +6729,8 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, if (def->cpu && (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { - if (qemuBuildCpuModelArgStr(driver, def, &buf, qemuCaps) < 0) + if (qemuBuildCpuModelArgStr(driver, def, &cpu_buf, qemuCaps) < 0) goto cleanup; - have_cpu = true; /* Only 'svm' requires --enable-nesting. The nested 'vmx' patches now * simply hook off the CPU features. */ @@ -6769,8 +6768,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, ((hostarch == VIR_ARCH_X86_64 && strstr(def->emulator, "kvm")) || strstr(def->emulator, "x86_64"))) { - virBufferAdd(&buf, default_model, -1); - have_cpu = true; + virBufferAdd(&cpu_buf, default_model, -1); } } @@ -6780,21 +6778,14 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, if (timer->name == VIR_DOMAIN_TIMER_NAME_KVMCLOCK && timer->present != -1) { - virBufferAsprintf(&buf, "%s,%ckvmclock", - have_cpu ? "" : default_model, + virBufferAsprintf(&buf, ",%ckvmclock", timer->present ? '+' : '-'); - have_cpu = true; } else if (timer->name == VIR_DOMAIN_TIMER_NAME_HYPERVCLOCK && timer->present == 1) { - virBufferAsprintf(&buf, "%s,hv_time", - have_cpu ? "" : default_model); - have_cpu = true; + virBufferAddLit(&buf, ",hv_time"); } else if (timer->name == VIR_DOMAIN_TIMER_NAME_TSC && timer->frequency > 0) { - virBufferAsprintf(&buf, "%s,tsc-frequency=%lu", - have_cpu ? "" : default_model, - timer->frequency); - have_cpu = true; + virBufferAsprintf(&buf, ",tsc-frequency=%lu", timer->frequency); } } @@ -6805,10 +6796,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, else sign = '-'; - virBufferAsprintf(&buf, "%s,%ckvm_pv_eoi", - have_cpu ? "" : default_model, - sign); - have_cpu = true; + virBufferAsprintf(&buf, ",%ckvm_pv_eoi", sign); } if (def->features[VIR_DOMAIN_FEATURE_PVSPINLOCK]) { @@ -6819,18 +6807,10 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, else sign = '-'; - virBufferAsprintf(&buf, "%s,%ckvm_pv_unhalt", - have_cpu ? "" : default_model, - sign); - have_cpu = true; + virBufferAsprintf(&buf, ",%ckvm_pv_unhalt", sign); } if (def->features[VIR_DOMAIN_FEATURE_HYPERV] == VIR_TRISTATE_SWITCH_ON) { - if (!have_cpu) { - virBufferAdd(&buf, default_model, -1); - have_cpu = true; - } - for (i = 0; i < VIR_DOMAIN_HYPERV_LAST; i++) { switch ((virDomainHyperv) i) { case VIR_DOMAIN_HYPERV_RELAXED: @@ -6866,22 +6846,12 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, for (i = 0; i < def->npanics; i++) { if (def->panics[i]->model == VIR_DOMAIN_PANIC_MODEL_HYPERV) { - if (!have_cpu) { - virBufferAdd(&buf, default_model, -1); - have_cpu = true; - } - virBufferAddLit(&buf, ",hv_crash"); break; } } if (def->features[VIR_DOMAIN_FEATURE_KVM] == VIR_TRISTATE_SWITCH_ON) { - if (!have_cpu) { - virBufferAdd(&buf, default_model, -1); - have_cpu = true; - } - for (i = 0; i < VIR_DOMAIN_KVM_LAST; i++) { switch ((virDomainKVM) i) { case VIR_DOMAIN_KVM_HIDDEN: @@ -6898,12 +6868,8 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, if (def->features[VIR_DOMAIN_FEATURE_PMU]) { virTristateSwitch pmu = def->features[VIR_DOMAIN_FEATURE_PMU]; - if (!have_cpu) - virBufferAdd(&buf, default_model, -1); - virBufferAsprintf(&buf, ",pmu=%s", virTristateSwitchTypeToString(pmu)); - have_cpu = true; } if (def->cpu && def->cpu->cache) { @@ -6911,11 +6877,6 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, bool hostOff = false; bool l3Off = false; - if (!have_cpu) { - virBufferAdd(&buf, default_model, -1); - have_cpu = true; - } - switch (cache->mode) { case VIR_CPU_CACHE_MODE_EMULATE: virBufferAddLit(&buf, ",l3-cache=on"); @@ -6945,13 +6906,22 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, virBufferAddLit(&buf, ",l3-cache=off"); } + if (virBufferCheckError(&cpu_buf) < 0) + goto cleanup; if (virBufferCheckError(&buf) < 0) goto cleanup; - cpu = virBufferContentAndReset(&buf); + cpu = virBufferContentAndReset(&cpu_buf); + cpu_flags = virBufferContentAndReset(&buf); + + if (cpu_flags && !cpu) { + if (VIR_STRDUP(cpu, default_model) < 0) + goto cleanup; + } if (cpu) { - virCommandAddArgList(cmd, "-cpu", cpu, NULL); + virCommandAddArg(cmd, "-cpu"); + virCommandAddArgFormat(cmd, "%s%s", cpu, cpu_flags ? cpu_flags : ""); if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_NESTING) && hasHwVirt) virCommandAddArg(cmd, "-enable-nesting"); @@ -6961,7 +6931,9 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, cleanup: VIR_FREE(cpu); + VIR_FREE(cpu_flags); virBufferFreeAndReset(&buf); + virBufferFreeAndReset(&cpu_buf); return ret; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.args b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.args index e708feece..d1718d1f9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-hyperv-off.args @@ -8,7 +8,6 @@ QEMU_AUDIO_DRV=none \ -name QEMUGuest1 \ -S \ -M pc \ --cpu qemu32 \ -m 214 \ -smp 6,sockets=6,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-kvm-features-off.args b/tests/qemuxml2argvdata/qemuxml2argv-kvm-features-off.args index e708feece..d1718d1f9 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-kvm-features-off.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-kvm-features-off.args @@ -8,7 +8,6 @@ QEMU_AUDIO_DRV=none \ -name QEMUGuest1 \ -S \ -M pc \ --cpu qemu32 \ -m 214 \ -smp 6,sockets=6,cores=1,threads=1 \ -uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ -- 2.13.3
On Fri, Jul 14, 2017 at 07:43:05PM -0400, Cole Robinson wrote:
Certain XML features that aren't in the <cpu> block map to -cpu flags on the qemu cli. If one of these is specified but the user didn't explicitly pass an XML <cpu> model, we need to format a default model on the command line.
The current code handles this by sprinkling this default cpu handling among all the different flag string formatting. Instead, switch it to do this just once.
This alters some test output slightly: the previous code would write the default -cpu in some cases when no flags were actually added, so the output was redundant.
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 72 +++++++--------------- .../qemuxml2argvdata/qemuxml2argv-hyperv-off.args | 1 - .../qemuxml2argv-kvm-features-off.args | 1 - 3 files changed, 22 insertions(+), 52 deletions(-)
ACK Jan
The code only currently handles writing an x86 default -cpu argument, and doesn't know anything about other architectures. Let's make this explicit rather than leaving ex. qemu ppc64 to throw an error about -cpu qemu64 Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index aa12479f7..f727e3d30 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -6716,17 +6716,11 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, virArch hostarch = virArchFromHost(); char *cpu = NULL, *cpu_flags = NULL; bool hasHwVirt = false; - const char *default_model; int ret = -1; virBuffer cpu_buf = VIR_BUFFER_INITIALIZER; virBuffer buf = VIR_BUFFER_INITIALIZER; size_t i; - if (def->os.arch == VIR_ARCH_I686) - default_model = "qemu32"; - else - default_model = "qemu64"; - if (def->cpu && (def->cpu->mode != VIR_CPU_MODE_CUSTOM || def->cpu->model)) { if (qemuBuildCpuModelArgStr(driver, def, &cpu_buf, qemuCaps) < 0) @@ -6768,7 +6762,7 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, ((hostarch == VIR_ARCH_X86_64 && strstr(def->emulator, "kvm")) || strstr(def->emulator, "x86_64"))) { - virBufferAdd(&cpu_buf, default_model, -1); + virBufferAddLit(&cpu_buf, "qemu32"); } } @@ -6915,6 +6909,23 @@ qemuBuildCpuCommandLine(virCommandPtr cmd, cpu_flags = virBufferContentAndReset(&buf); if (cpu_flags && !cpu) { + const char *default_model; + + switch (def->os.arch) { + case VIR_ARCH_I686: + default_model = "qemu32"; + break; + case VIR_ARCH_X86_64: + default_model = "qemu64"; + break; + default: + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("CPU flags requested but can't determine " + "default CPU for arch %s"), + virArchToString(def->os.arch)); + goto cleanup; + } + if (VIR_STRDUP(cpu, default_model) < 0) goto cleanup; } -- 2.13.3
On Fri, 2017-07-14 at 19:43 -0400, Cole Robinson wrote:
The code only currently handles writing an x86 default -cpu argument, and doesn't know anything about other architectures. Let's make this explicit rather than leaving ex. qemu ppc64 to throw an error about -cpu qemu64
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This seems to also fix https://bugzilla.redhat.com/show_bug.cgi?id=1235511 I'll review the series early next week. -- Andrea Bolognani / Red Hat / Virtualization
On 07/15/2017 12:40 PM, Andrea Bolognani wrote:
On Fri, 2017-07-14 at 19:43 -0400, Cole Robinson wrote:
The code only currently handles writing an x86 default -cpu argument, and doesn't know anything about other architectures. Let's make this explicit rather than leaving ex. qemu ppc64 to throw an error about -cpu qemu64
Signed-off-by: Cole Robinson <crobinso@redhat.com>
This seems to also fix
https://bugzilla.redhat.com/show_bug.cgi?id=1235511
I'll review the series early next week.
Thanks, but I don't think it will fix pmu=on for ppc64, just improve the error scenario. We will still need to extend the code to hardcode the default CPU model for PPC64 (would be nice if qemu had a generic 'default' value to use here, or some way to get around specifying a cpu model) - Cole
On Fri, Jul 14, 2017 at 07:43:06PM -0400, Cole Robinson wrote:
The code only currently handles writing an x86 default -cpu argument, and doesn't know anything about other architectures. Let's make this explicit rather than leaving ex. qemu ppc64 to throw an error about -cpu qemu64
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
ACK Jan
On 07/21/2017 10:25 AM, Ján Tomko wrote:
On Fri, Jul 14, 2017 at 07:43:06PM -0400, Cole Robinson wrote:
The code only currently handles writing an x86 default -cpu argument, and doesn't know anything about other architectures. Let's make this explicit rather than leaving ex. qemu ppc64 to throw an error about -cpu qemu64
Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/qemu/qemu_command.c | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-)
ACK
Jan
Thanks, I pushed patch 2 + 3 now which you reviewed, patch 1 was independent and has some comments. - Cole
participants (4)
-
Andrea Bolognani -
Cole Robinson -
Ján Tomko -
Pavel Hrdina