[libvirt] [RFC PATCH v2 0/4] Libvirt for PowerPC
Hi, Recent development in KVM for 64-bit Power ISA Book3S machines, allows users to run multiple KVM guest instances on POWER7 and PPC970 processor based systems. Also qemu-system-ppc64 has been enhanced to support a new machine type "pseries" suitable for Power Book3S machines. This addition effectively brings the KVM+qemu combination to run multiple guest instances on a Power Book3S machine. Libvirt continues to be the key interface to configure and manage the KVM guest instances on x86. This patch set is an effort to enable libvirt to support KVM guest configuration and management on Power Book3S machines. Based on community discussion around the earlier version, this patch series augments the present 'kvm' driver to support PowerPC-KVM based guests. Since some of the supported devices vary between architectures, the code is now reworked to dynamically choose the correct handler function based on guest domain architecture at runtime (def->os.arch). This patch series consists of 4 patches : Patch 1/4 : Use sysfs to gather host topology. Presently libvirt depends on /proc/cpuinfo gather CPU, cores, threads, etc This is highly architecture dependent. A alternative is to use sysfs, which provides a platform-neutral interface to parse CPU topology. Patch 2/4 : Add PowerPC CPU Driver Patch 3/4 : Add support for qemu-system-ppc64 Patch 4/4 : Refactor qemu commmand-line generation to separate out arch- specific options from generic command line. Changelog from v1: * Patches 1,2,3 unchanged ; The hacks in Patch 4 of v1 have been replaced by a new patch to neatly select arch-specific features. Awaiting comments and feedback, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
From 9084802bdf7a2c10d6a913cb7dde001ab6ab3543 Mon Sep 17 00:00:00 2001 From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:45:30 -0700 Subject: [PATCH 1/4] Use sysfs to gather host topology, in place of /proc/cpuinfo
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/nodeinfo.c | 106 +++++++++++++++++++------------------------------------- 1 files changed, 36 insertions(+), 70 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 6448b79..cdd339d 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -30,6 +30,7 @@ #include <errno.h> #include <dirent.h> #include <sys/utsname.h> +#include <sched.h> #if HAVE_NUMACTL # define NUMA_VERSION1_COMPATIBILITY 1 @@ -191,6 +192,11 @@ static int parse_socket(unsigned int cpu) return ret; } +static int parse_core(unsigned int cpu) +{ + return get_cpu_value(cpu, "topology/core_id", false); +} + int linuxNodeInfoCPUPopulate(FILE *cpuinfo, virNodeInfoPtr nodeinfo, bool need_hyperthreads) @@ -199,15 +205,14 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, DIR *cpudir = NULL; struct dirent *cpudirent = NULL; unsigned int cpu; - unsigned long cur_threads; - int socket; - unsigned long long socket_mask = 0; - unsigned int remaining; + unsigned long core, socket, cur_threads; + cpu_set_t core_mask; + cpu_set_t socket_mask; int online; nodeinfo->cpus = 0; nodeinfo->mhz = 0; - nodeinfo->cores = 1; + nodeinfo->cores = 0; nodeinfo->nodes = 1; # if HAVE_NUMACTL @@ -221,20 +226,10 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, /* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ while (fgets(line, sizeof(line), cpuinfo) != NULL) { char *buf = line; - if (STRPREFIX(buf, "processor")) { /* aka a single logical CPU */ - buf += 9; - while (*buf && c_isspace(*buf)) - buf++; - if (*buf != ':') { - nodeReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("parsing cpuinfo processor")); - return -1; - } - nodeinfo->cpus++; # if defined(__x86_64__) || \ defined(__amd64__) || \ defined(__i386__) - } else if (STRPREFIX(buf, "cpu MHz")) { + if (STRPREFIX(buf, "cpu MHz")) { char *p; unsigned int ui; buf += 9; @@ -249,24 +244,9 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, /* Accept trailing fractional part. */ && (*p == '\0' || *p == '.' || c_isspace(*p))) nodeinfo->mhz = ui; - } else if (STRPREFIX(buf, "cpu cores")) { /* aka cores */ - char *p; - unsigned int id; - buf += 9; - while (*buf && c_isspace(*buf)) - buf++; - if (*buf != ':' || !buf[1]) { - nodeReportError(VIR_ERR_INTERNAL_ERROR, - _("parsing cpuinfo cpu cores %c"), *buf); - return -1; - } - if (virStrToLong_ui(buf+1, &p, 10, &id) == 0 - && (*p == '\0' || c_isspace(*p)) - && id > nodeinfo->cores) - nodeinfo->cores = id; # elif defined(__powerpc__) || \ defined(__powerpc64__) - } else if (STRPREFIX(buf, "clock")) { + if (STRPREFIX(buf, "clock")) { char *p; unsigned int ui; buf += 5; @@ -281,53 +261,30 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, /* Accept trailing fractional part. */ && (*p == '\0' || *p == '.' || c_isspace(*p))) nodeinfo->mhz = ui; -# elif defined(__s390__) || \ - defined(__s390x__) - } else if (STRPREFIX(buf, "# processors")) { - char *p; - unsigned int ui; - buf += 12; - while (*buf && c_isspace(*buf)) - buf++; - if (*buf != ':' || !buf[1]) { - nodeReportError(VIR_ERR_INTERNAL_ERROR, - _("parsing number of processors %c"), *buf); - return -1; - } - if (virStrToLong_ui(buf+1, &p, 10, &ui) == 0 - && (*p == '\0' || c_isspace(*p))) - nodeinfo->cpus = ui; /* No other interesting infos are available in /proc/cpuinfo. * However, there is a line identifying processor's version, * identification and machine, but we don't want it to be caught * and parsed in next iteration, because it is not in expected * format and thus lead to error. */ - break; # else # warning Parser for /proc/cpuinfo needs to be adapted for your architecture # endif } } - if (!nodeinfo->cpus) { - nodeReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("no cpus found")); - return -1; - } - - if (!need_hyperthreads) - return 0; - - /* OK, we've parsed what we can out of /proc/cpuinfo. Get the socket - * and thread information from /sys + /* OK, we've parsed clock speed out of /proc/cpuinfo. Get the core, socket + * thread and topology information from /sys */ - remaining = nodeinfo->cpus; cpudir = opendir(CPU_SYS_PATH); if (cpudir == NULL) { virReportSystemError(errno, _("cannot opendir %s"), CPU_SYS_PATH); return -1; } - while ((errno = 0), remaining && (cpudirent = readdir(cpudir))) { + + CPU_ZERO(&core_mask); + CPU_ZERO(&socket_mask); + + while (cpudirent = readdir(cpudir)) { if (sscanf(cpudirent->d_name, "cpu%u", &cpu) != 1) continue; @@ -338,15 +295,19 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, } if (!online) continue; - remaining--; + nodeinfo->cpus++; - socket = parse_socket(cpu); - if (socket < 0) { - closedir(cpudir); - return -1; + /* Parse core */ + core = parse_core(cpu); + if (!CPU_ISSET(core, &core_mask)) { + CPU_SET(core, &core_mask); + nodeinfo->cores++; } - if (!(socket_mask & (1 << socket))) { - socket_mask |= (1 << socket); + + /* Parse socket */ + socket = parse_socket(cpu); + if (!CPU_ISSET(socket, &socket_mask)) { + CPU_SET(socket, &socket_mask); nodeinfo->sockets++; } @@ -367,7 +328,12 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, closedir(cpudir); - /* there should always be at least one socket and one thread */ + /* there should always be at least one cpu, socket and one thread */ + if (nodeinfo->cpus == 0) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("no CPUs found")); + return -1; + } if (nodeinfo->sockets == 0) { nodeReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("no sockets found")); -- 1.7.7 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On 11/14/2011 07:46 AM, Prerna Saxena wrote:
From 9084802bdf7a2c10d6a913cb7dde001ab6ab3543 Mon Sep 17 00:00:00 2001 From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:45:30 -0700 Subject: [PATCH 1/4] Use sysfs to gather host topology, in place of /proc/cpuinfo
Are we sure that sysfs has this information for all kernels as far back as RHEL 5? Or should this patch be doing conditionals; probing sysfs first but falling back to /proc/cpuinfo otherwise? I tried compiling things, but got this warning: nodeinfo.c: In function 'linuxNodeInfoCPUPopulate': nodeinfo.c:287:5: error: suggest parentheses around assignment used as truth value [-Werror=parentheses] nodeinfo.c:202:36: error: unused parameter 'need_hyperthreads' [-Werror=unused-parameter] and seeing other comments in this thread, I didn't review it very closely (this is more a chance to revive the thread after a week, to see if you are planning on a resubmission before we consider a freeze for 0.9.8). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From 918e49581c91c007086a0a7189c9b6cf83ba2bb0 Mon Sep 17 00:00:00 2001 From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:56:20 -0700 Subject: [PATCH 2/4] Add PPC cpu driver.
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> Signed-off-by: Anton Blanchard <anton@au.ibm.com> --- src/Makefile.am | 3 +- src/cpu/cpu.c | 2 + src/cpu/cpu_powerpc.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++ src/cpu/cpu_powerpc.h | 32 +++++++++++++++++++ 4 files changed, 117 insertions(+), 1 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index e931d41..2c4e6ba 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -524,7 +524,8 @@ CPU_SOURCES = \ cpu/cpu.h cpu/cpu.c \ cpu/cpu_generic.h cpu/cpu_generic.c \ cpu/cpu_x86.h cpu/cpu_x86.c cpu/cpu_x86_data.h \ - cpu/cpu_map.h cpu/cpu_map.c + cpu/cpu_map.h cpu/cpu_map.c cpu/cpu_powerpc.h \ + cpu/cpu_powerpc.c VMX_SOURCES = \ vmx/vmx.c vmx/vmx.h diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index b919b6e..e4149e2 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -28,6 +28,7 @@ #include "xml.h" #include "cpu.h" #include "cpu_x86.h" +#include "cpu_powerpc.h" #include "cpu_generic.h" @@ -36,6 +37,7 @@ static struct cpuArchDriver *drivers[] = { &cpuDriverX86, + &cpuDriverPowerPC, /* generic driver must always be the last one */ &cpuDriverGeneric }; diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c new file mode 100644 index 0000000..6ceedc3 --- /dev/null +++ b/src/cpu/cpu_powerpc.c @@ -0,0 +1,81 @@ +/* + * cpu_powerpc.h: CPU driver for PowerPC CPUs + * + * Copyright (C) Copyright (C) IBM Corporation, 2010 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Anton Blanchard <anton@au.ibm.com> + * Prerna Saxena <prerna@linux.vnet.ibm.com> + */ + +#include <config.h> + +#include "memory.h" +#include "cpu.h" + + +#define VIR_FROM_THIS VIR_FROM_CPU + +static const char *archs[] = { "ppc64" }; + +static union cpuData * +PowerPCNodeData(void) +{ + union cpuData *data; + + if (VIR_ALLOC(data) < 0) { + virReportOOMError(); + return NULL; + } + + return data; +} + + +static int +PowerPCDecode(virCPUDefPtr cpu, + const union cpuData *data, + const char **models, + unsigned int nmodels, + const char *preferred) +{ + return 0; +} + +static int +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return 0; + + VIR_FREE(data); +} + +struct cpuArchDriver cpuDriverPowerPC = { + .name = "ppc64", + .arch = archs, + .narch = ARRAY_CARDINALITY(archs), + .compare = NULL, + .decode = PowerPCDecode, + .encode = NULL, + .free = PowerPCDataFree, + .nodeData = PowerPCNodeData, + .guestData = NULL, + .baseline = NULL, + .update = NULL, + .hasFeature = NULL, +}; diff --git a/src/cpu/cpu_powerpc.h b/src/cpu/cpu_powerpc.h new file mode 100644 index 0000000..2e0c1a5 --- /dev/null +++ b/src/cpu/cpu_powerpc.h @@ -0,0 +1,32 @@ +/* + * cpu_powerpc.h: CPU driver for PowerPC CPUs + * + * Copyright (C) Copyright (C) IBM Corporation, 2010 + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + * Authors: + * Anton Blanchard <anton@au.ibm.com> + * Prerna Saxena <prerna@linux.vnet.ibm.com> + */ + +#ifndef __VIR_CPU_POWERPC_H__ +# define __VIR_CPU_POWERPC_H__ + +# include "cpu.h" + +extern struct cpuArchDriver cpuDriverPowerPC; + +#endif /* __VIR_CPU_POWERPC_H__ */ -- 1.7.7 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
From cf5850d807fb6d86a16879a3e4fe120a96b78e27 Mon Sep 17 00:00:00 2001 From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 06:01:33 -0700 Subject: [PATCH 3/4] Add support for ppc64 qemu
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/qemu/qemu_capabilities.c | 64 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 64 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 26a7f11..f9a465d 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -185,6 +185,7 @@ static const struct qemu_arch_info const arch_info_hvm[] = { { "mipsel", 32, NULL, "qemu-system-mipsel", NULL, NULL, 0 }, { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 }, { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 }, + { "ppc64", 64, NULL, "qemu-system-ppc64", NULL, NULL, 0 }, { "itanium", 64, NULL, "qemu-system-ia64", NULL, NULL, 0 }, { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, }; @@ -477,6 +478,67 @@ error: return -1; } +/* ppc64 parser. + * Format : PowerPC <machine> <description> + */ +static int +qemuCapsParsePPCModels(const char *output, + unsigned int *retcount, + const char ***retcpus) +{ + const char *p = output; + const char *next; + unsigned int count = 0; + const char **cpus = NULL; + int i; + do { + const char *t; + + if ((next = strchr(p, '\n'))) + next++; + + if (!STRPREFIX(p, "PowerPC ")) + continue; + + /* Skip the preceding sub-string "PowerPC " */ + p += 8; + + /*Malformed string, does not obey the format 'PowerPC <model> <desc>'*/ + if (!(t = strchr(p, ' ')) || (next && t >= next)) + continue; + + if (*p == '\0' || *p == '\n') + continue; + + if (retcpus) { + unsigned int len; + + if (VIR_REALLOC_N(cpus, count + 1) < 0) + goto error; + + if (t) + len = t - p - 1; + + if (!(cpus[count] = strndup(p, len))) + goto error; + } + count++; + } while ((p = next)); + + if (retcount) + *retcount = count; + if (retcpus) + *retcpus = cpus; + return 0; + +error: + if (cpus) { + for (i = 0; i < count; i++) + VIR_FREE(cpus[i]); + } + VIR_FREE(cpus); + return -1; +} int qemuCapsProbeCPUModels(const char *qemu, @@ -497,6 +559,8 @@ qemuCapsProbeCPUModels(const char *qemu, if (STREQ(arch, "i686") || STREQ(arch, "x86_64")) parse = qemuCapsParseX86Models; + else if (STREQ(arch, "ppc64")) + parse = qemuCapsParsePPCModels; else { VIR_DEBUG("don't know how to parse %s CPU models", arch); return 0; -- 1.7.7 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001 From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 14 Nov 2011 19:43:26 +0530 Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from generic qemu commandline building code. At present, qemuBuildCommandLine emits many x86-specific options while generating the qemu command line. This patch proposes a framework to add arch-specific handler functions for generating different aspects of command line. At present, it is used to prevent x86-specific qemu options from cluttering the qemu command line for other architectures. Going forward, if newer drivers get defined for any arch, the code for handling these might be added by extending the handler function for that architecture.
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 220 ++++++++++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 20 ++++ 2 files changed, 169 insertions(+), 71 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b044050..61dabbf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "local", "handle"); +virQemuCommandLineFunction qemuCmdLineX86 = + { .qemuBuildArchFunction = qemuBuildX86CommandLine, + }; + +virQemuCommandLineFunction qemuCmdLinePPC64 = + { .qemuBuildArchFunction = NULL, + }; static void uname_normalize (struct utsname *ut) @@ -3254,6 +3261,8 @@ qemuBuildCommandLine(virConnectPtr conn, bool emitBootindex = false; int usbcontroller = 0; bool usblegacy = false; + char *command_str; + virQemuCommandLineFunctionPtr qemuCmdFunc; uname_normalize(&ut); virUUIDFormat(def->uuid, uuid); @@ -3409,54 +3418,22 @@ qemuBuildCommandLine(virConnectPtr conn, } } - if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) && - (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) { - virSysinfoDefPtr source = NULL; - bool skip_uuid = false; - - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("the QEMU binary %s does not support smbios settings"), - emulator); - goto error; - } - - /* should we really error out or just warn in those cases ? */ - if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) { - if (driver->hostsysinfo == NULL) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Host SMBIOS information is not available")); - goto error; - } - source = driver->hostsysinfo; - /* Host and guest uuid must differ, by definition of UUID. */ - skip_uuid = true; - } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) { - if (def->sysinfo == NULL) { - qemuReportError(VIR_ERR_XML_ERROR, - _("Domain '%s' sysinfo are not available"), - def->name); - goto error; - } - source = def->sysinfo; - /* domain_conf guaranteed that system_uuid matches guest uuid. */ - } - if (source != NULL) { - char *smbioscmd; + if (!strcmp(def->os.arch, "i686") || + !strcmp(def->os.arch, "x86_64")) + qemuCmdFunc = &qemuCmdLineX86; + else { + if (!strcmp(def->os.arch, "ppc64")) + qemuCmdFunc = &qemuCmdLinePPC64; + } - smbioscmd = qemuBuildSmbiosBiosStr(source); - if (smbioscmd != NULL) { - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); - VIR_FREE(smbioscmd); - } - smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid); - if (smbioscmd != NULL) { - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); - VIR_FREE(smbioscmd); - } - } + if (qemuCmdFunc->qemuBuildArchFunction) { + command_str = (qemuCmdFunc->qemuBuildArchFunction)(conn, driver, def, + qemuCaps); + if (command_str) { + virCommandAddArg(cmd, command_str); + VIR_FREE(command_str); + } } - /* * NB, -nographic *MUST* come before any serial, or monitor * or parallel port flags due to QEMU craziness, where it @@ -3475,25 +3452,6 @@ qemuBuildCommandLine(virConnectPtr conn, "-nodefaults"); /* Disable default guest devices */ } - /* Serial graphics adapter */ - if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) { - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("qemu does not support -device")); - goto error; - } - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SGA)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("qemu does not support SGA")); - goto error; - } - if (!def->nserials) { - qemuReportError(VIR_ERR_XML_ERROR, "%s", - _("need at least one serial port to use SGA")); - goto error; - } - virCommandAddArgList(cmd, "-device", "sga", NULL); - } if (monitor_chr) { char *chrdev; @@ -3664,9 +3622,6 @@ qemuBuildCommandLine(virConnectPtr conn, if (monitor_json && qemuCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) virCommandAddArg(cmd, "-no-shutdown"); - if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) - virCommandAddArg(cmd, "-no-acpi"); - if (!def->os.bootloader) { /* * We prefer using explicit bootindex=N parameters for predictable @@ -4347,8 +4302,10 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); virCommandAddArg(cmd, "-device"); - virCommandAddArgFormat(cmd, "isa-serial,chardev=char%s,id=%s", - serial->info.alias, serial->info.alias); + if (!(devstr = qemuBuildChrDeviceStr(serial, def->os.arch))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -5238,7 +5195,127 @@ qemuBuildCommandLine(virConnectPtr conn, return NULL; } +/* This function separates x86* specific initializations in QEMU command + * line. + */ +char * +qemuBuildX86CommandLine ( virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + virBitmapPtr qemuCaps) +{ + virBuffer opt = VIR_BUFFER_INITIALIZER; + + if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) && + (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) { + virSysinfoDefPtr source = NULL; + bool skip_uuid = false; + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("the QEMU binary %s does not support smbios settings"), + def->emulator); + goto error; + } + + /* should we really error out or just warn in those cases ? */ + if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) { + if (driver->hostsysinfo == NULL) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Host SMBIOS information is not available")); + goto error; + } + source = driver->hostsysinfo; + /* Host and guest uuid must differ, by definition of UUID. */ + skip_uuid = true; + } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) { + if (def->sysinfo == NULL) { + qemuReportError(VIR_ERR_XML_ERROR, + _("Domain '%s' sysinfo are not available"), + def->name); + goto error; + } + source = def->sysinfo; + /* domain_conf guaranteed that system_uuid matches guest uuid. */ + } + if (source != NULL) { + char *smbioscmd; + + smbioscmd = qemuBuildSmbiosBiosStr(source); + if (smbioscmd != NULL) { + virBufferAsprintf(&opt, ", -smbios %s", smbioscmd); + VIR_FREE(smbioscmd); + } + smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid); + if (smbioscmd != NULL) { + virBufferAsprintf(&opt, ", -smbios", smbioscmd, NULL); + VIR_FREE(smbioscmd); + } + } + } + + /* Serial graphics adapter */ + if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) { + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu does not support -device")); + goto error; + } + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SGA)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu does not support SGA")); + goto error; + } + if (!def->nserials) { + qemuReportError(VIR_ERR_XML_ERROR, "%s", + _("need at least one serial port to use SGA")); + goto error; + } + virBufferAddLit(&opt, "-device sga"); + } + + if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) + virBufferAddLit(&opt, "-no-acpi"); + + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; + } + + return virBufferContentAndReset(&opt); + + error: + virBufferFreeAndReset(&opt); + return NULL; +} + +/* + * This function generates the correct '-device' string + * for each arch. + */ +char* +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, char *os_arch) +{ + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + if (STREQ(os_arch, "ppc64")) + virBufferAsprintf(&cmd, " spapr-vty,chardev=char%s ", + serial->info.alias); + else + virBufferAsprintf(&cmd, "isa-serial,chardev=char%s,id=%s", + serial->info.alias, serial->info.alias); + + if (virBufferError(&cmd)) { + virReportOOMError(); + goto error; + } + + return virBufferContentAndReset(&cmd); + + error: + virBufferFreeAndReset(&cmd); + return NULL; +} /* * This method takes a string representing a QEMU command line ARGV set * optionally prefixed by a list of environment variables. It then tries @@ -6444,7 +6521,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->maxvcpus = 1; def->vcpus = 1; def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; - def->features = (1 << VIR_DOMAIN_FEATURE_ACPI) + if (!strcmp(def->os.arch, "i686") || !strcmp(def->os.arch, "x86_64")) + def->features = (1 << VIR_DOMAIN_FEATURE_ACPI) /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/; def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; def->onCrash = VIR_DOMAIN_LIFECYCLE_DESTROY; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bfdaff9..9694962 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -40,6 +40,16 @@ # define QEMU_VNC_PORT_MIN 5900 # define QEMU_VNC_PORT_MAX 65535 +struct _virQemuCommandLineFunction { + char * (*qemuBuildArchFunction) (virConnectPtr, + struct qemud_driver *, + virDomainDefPtr, + virBitmapPtr); + /* Arch-specific features, such as SMBIOS, ACPI */ +}; + +typedef struct _virQemuCommandLineFunction virQemuCommandLineFunction; +typedef struct _virQemuCommandLineFunction* virQemuCommandLineFunctionPtr; virCommandPtr qemuBuildCommandLine(virConnectPtr conn, struct qemud_driver *driver, @@ -53,6 +63,16 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, enum virVMOperationType vmop) ATTRIBUTE_NONNULL(1); +/* Build Additional X86-specific options on command line */ +char * qemuBuildX86CommandLine(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + virBitmapPtr qemuCaps); + +/* Generate string for arch-specific '-device' parameter */ +char* +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, char *os_arch); + /* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, char type_sep, -- 1.7.7 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On Mon, Nov 14, 2011 at 08:26:55PM +0530, Prerna Saxena wrote:
From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001 From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 14 Nov 2011 19:43:26 +0530 Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from generic qemu commandline building code. At present, qemuBuildCommandLine emits many x86-specific options while generating the qemu command line. This patch proposes a framework to add arch-specific handler functions for generating different aspects of command line. At present, it is used to prevent x86-specific qemu options from cluttering the qemu command line for other architectures. Going forward, if newer drivers get defined for any arch, the code for handling these might be added by extending the handler function for that architecture.
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 220 ++++++++++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 20 ++++ 2 files changed, 169 insertions(+), 71 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b044050..61dabbf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "local", "handle");
+virQemuCommandLineFunction qemuCmdLineX86 = + { .qemuBuildArchFunction = qemuBuildX86CommandLine, + }; + +virQemuCommandLineFunction qemuCmdLinePPC64 = + { .qemuBuildArchFunction = NULL, + };
static void uname_normalize (struct utsname *ut) @@ -3254,6 +3261,8 @@ qemuBuildCommandLine(virConnectPtr conn, bool emitBootindex = false; int usbcontroller = 0; bool usblegacy = false; + char *command_str; + virQemuCommandLineFunctionPtr qemuCmdFunc; uname_normalize(&ut);
virUUIDFormat(def->uuid, uuid); @@ -3409,54 +3418,22 @@ qemuBuildCommandLine(virConnectPtr conn, } }
- if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) && - (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) { - virSysinfoDefPtr source = NULL; - bool skip_uuid = false; - - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("the QEMU binary %s does not support smbios settings"), - emulator); - goto error; - } - - /* should we really error out or just warn in those cases ? */ - if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) { - if (driver->hostsysinfo == NULL) { - qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("Host SMBIOS information is not available")); - goto error; - } - source = driver->hostsysinfo; - /* Host and guest uuid must differ, by definition of UUID. */ - skip_uuid = true; - } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) { - if (def->sysinfo == NULL) { - qemuReportError(VIR_ERR_XML_ERROR, - _("Domain '%s' sysinfo are not available"), - def->name); - goto error; - } - source = def->sysinfo; - /* domain_conf guaranteed that system_uuid matches guest uuid. */ - } - if (source != NULL) { - char *smbioscmd; + if (!strcmp(def->os.arch, "i686") || + !strcmp(def->os.arch, "x86_64"))
The coding guidelines require use of STREQ or STRNEQ
+ qemuCmdFunc = &qemuCmdLineX86; + else { + if (!strcmp(def->os.arch, "ppc64")) + qemuCmdFunc = &qemuCmdLinePPC64; + }
If we get an arch that is not PPC or x86, then 'qemuCmdFunc' can end up NULL
- smbioscmd = qemuBuildSmbiosBiosStr(source); - if (smbioscmd != NULL) { - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); - VIR_FREE(smbioscmd); - } - smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid); - if (smbioscmd != NULL) { - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); - VIR_FREE(smbioscmd); - } - } + if (qemuCmdFunc->qemuBuildArchFunction) {
Which will cause a SEGV here.
+ command_str = (qemuCmdFunc->qemuBuildArchFunction)(conn, driver, def, + qemuCaps); + if (command_str) { + virCommandAddArg(cmd, command_str); + VIR_FREE(command_str); + } } - /* * NB, -nographic *MUST* come before any serial, or monitor * or parallel port flags due to QEMU craziness, where it @@ -3475,25 +3452,6 @@ qemuBuildCommandLine(virConnectPtr conn, "-nodefaults"); /* Disable default guest devices */ }
- /* Serial graphics adapter */ - if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) { - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("qemu does not support -device")); - goto error; - } - if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SGA)) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("qemu does not support SGA")); - goto error; - } - if (!def->nserials) { - qemuReportError(VIR_ERR_XML_ERROR, "%s", - _("need at least one serial port to use SGA")); - goto error; - } - virCommandAddArgList(cmd, "-device", "sga", NULL); - }
if (monitor_chr) { char *chrdev; @@ -3664,9 +3622,6 @@ qemuBuildCommandLine(virConnectPtr conn, if (monitor_json && qemuCapsGet(qemuCaps, QEMU_CAPS_NO_SHUTDOWN)) virCommandAddArg(cmd, "-no-shutdown");
- if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) - virCommandAddArg(cmd, "-no-acpi"); - if (!def->os.bootloader) { /* * We prefer using explicit bootindex=N parameters for predictable @@ -4347,8 +4302,10 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr);
virCommandAddArg(cmd, "-device"); - virCommandAddArgFormat(cmd, "isa-serial,chardev=char%s,id=%s", - serial->info.alias, serial->info.alias); + if (!(devstr = qemuBuildChrDeviceStr(serial, def->os.arch))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -5238,7 +5195,127 @@ qemuBuildCommandLine(virConnectPtr conn, return NULL; }
+/* This function separates x86* specific initializations in QEMU command + * line. + */ +char * +qemuBuildX86CommandLine ( virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + virBitmapPtr qemuCaps) +{ + virBuffer opt = VIR_BUFFER_INITIALIZER; + + if ((def->os.smbios_mode != VIR_DOMAIN_SMBIOS_NONE) && + (def->os.smbios_mode != VIR_DOMAIN_SMBIOS_EMULATE)) { + virSysinfoDefPtr source = NULL; + bool skip_uuid = false;
+ if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SMBIOS_TYPE)) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("the QEMU binary %s does not support smbios settings"), + def->emulator); + goto error; + } + + /* should we really error out or just warn in those cases ? */ + if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_HOST) { + if (driver->hostsysinfo == NULL) { + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Host SMBIOS information is not available")); + goto error; + } + source = driver->hostsysinfo; + /* Host and guest uuid must differ, by definition of UUID. */ + skip_uuid = true; + } else if (def->os.smbios_mode == VIR_DOMAIN_SMBIOS_SYSINFO) { + if (def->sysinfo == NULL) { + qemuReportError(VIR_ERR_XML_ERROR, + _("Domain '%s' sysinfo are not available"), + def->name); + goto error; + } + source = def->sysinfo; + /* domain_conf guaranteed that system_uuid matches guest uuid. */ + } + if (source != NULL) { + char *smbioscmd; + + smbioscmd = qemuBuildSmbiosBiosStr(source); + if (smbioscmd != NULL) { + virBufferAsprintf(&opt, ", -smbios %s", smbioscmd); + VIR_FREE(smbioscmd); + } + smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid); + if (smbioscmd != NULL) { + virBufferAsprintf(&opt, ", -smbios", smbioscmd, NULL); + VIR_FREE(smbioscmd); + } + } + } + + /* Serial graphics adapter */ + if (def->os.bios.useserial == VIR_DOMAIN_BIOS_USESERIAL_YES) { + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu does not support -device")); + goto error; + } + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SGA)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("qemu does not support SGA")); + goto error; + } + if (!def->nserials) { + qemuReportError(VIR_ERR_XML_ERROR, "%s", + _("need at least one serial port to use SGA")); + goto error; + } + virBufferAddLit(&opt, "-device sga"); + } + + if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) + virBufferAddLit(&opt, "-no-acpi"); + + if (virBufferError(&opt)) { + virReportOOMError(); + goto error; + } + + return virBufferContentAndReset(&opt); + + error: + virBufferFreeAndReset(&opt); + return NULL; +}
The convention we try to follow is that if there is something in the XML which is not supported by the QEMU we're about to run, then we should raise an error VIR_ERR_CONFIG_UNSUPPORTED. Thus I'd expect to see a PPC method which raises such an error if the user requests no-ACPI, serial BIOS output, or SMBIOS data. Finally, looking at the changes, I'd be somewhat amazed if this refactoring has not broken the test suite since AFAICT, it changes the ordering of some command line arguments. Did 'make check' really pass after applying this ?
+/* + * This function generates the correct '-device' string + * for each arch. + */ +char* +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, char *os_arch) +{ + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + if (STREQ(os_arch, "ppc64")) + virBufferAsprintf(&cmd, " spapr-vty,chardev=char%s ", + serial->info.alias); + else + virBufferAsprintf(&cmd, "isa-serial,chardev=char%s,id=%s", + serial->info.alias, serial->info.alias); + + if (virBufferError(&cmd)) { + virReportOOMError(); + goto error; + } + + return virBufferContentAndReset(&cmd); + + error: + virBufferFreeAndReset(&cmd); + return NULL; +} /* * This method takes a string representing a QEMU command line ARGV set * optionally prefixed by a list of environment variables. It then tries @@ -6444,7 +6521,8 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr caps, def->maxvcpus = 1; def->vcpus = 1; def->clock.offset = VIR_DOMAIN_CLOCK_OFFSET_UTC; - def->features = (1 << VIR_DOMAIN_FEATURE_ACPI) + if (!strcmp(def->os.arch, "i686") || !strcmp(def->os.arch, "x86_64")) + def->features = (1 << VIR_DOMAIN_FEATURE_ACPI) /*| (1 << VIR_DOMAIN_FEATURE_APIC)*/; def->onReboot = VIR_DOMAIN_LIFECYCLE_RESTART; def->onCrash = VIR_DOMAIN_LIFECYCLE_DESTROY; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index bfdaff9..9694962 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -40,6 +40,16 @@ # define QEMU_VNC_PORT_MIN 5900 # define QEMU_VNC_PORT_MAX 65535
+struct _virQemuCommandLineFunction { + char * (*qemuBuildArchFunction) (virConnectPtr, + struct qemud_driver *, + virDomainDefPtr, + virBitmapPtr); + /* Arch-specific features, such as SMBIOS, ACPI */ +}; + +typedef struct _virQemuCommandLineFunction virQemuCommandLineFunction; +typedef struct _virQemuCommandLineFunction* virQemuCommandLineFunctionPtr;
These can both be in the source file too. Also the naming convention for the QEMU driver source files is a prefix 'qemu' ather than 'virQemu'.
@@ -53,6 +63,16 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, enum virVMOperationType vmop) ATTRIBUTE_NONNULL(1);
+/* Build Additional X86-specific options on command line */ +char * qemuBuildX86CommandLine(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + virBitmapPtr qemuCaps);
I think this function can just be static - there's no need to be able to access it outside the source file it lives in.
+ +/* Generate string for arch-specific '-device' parameter */ +char* +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, char *os_arch); + /* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, char type_sep,
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
Hi Daniel, Thanks for taking a look. On 11/15/2011 04:33 PM, Daniel P. Berrange wrote:
On Mon, Nov 14, 2011 at 08:26:55PM +0530, Prerna Saxena wrote:
From ae7764be4454be1900fc3ee0a03bf819d5cd12de Mon Sep 17 00:00:00 2001 From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 14 Nov 2011 19:43:26 +0530 Subject: [PATCH 4/4] Separate out arch-specific qemu initialization from generic qemu commandline building code. At present, qemuBuildCommandLine emits many x86-specific options while generating the qemu command line. This patch proposes a framework to add arch-specific handler functions for generating different aspects of command line. At present, it is used to prevent x86-specific qemu options from cluttering the qemu command line for other architectures. Going forward, if newer drivers get defined for any arch, the code for handling these might be added by extending the handler function for that architecture.
Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 220 ++++++++++++++++++++++++++++++++--------------- src/qemu/qemu_command.h | 20 ++++ 2 files changed, 169 insertions(+), 71 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b044050..61dabbf 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -107,6 +107,13 @@ VIR_ENUM_IMPL(qemuDomainFSDriver, VIR_DOMAIN_FS_DRIVER_TYPE_LAST, "local", ... [snip] ... - if (source != NULL) { - char *smbioscmd; + if (!strcmp(def->os.arch, "i686") || + !strcmp(def->os.arch, "x86_64"))
The coding guidelines require use of STREQ or STRNEQ
I'm sorry, overlooked this. I'll correct this to use STREQ.
+ qemuCmdFunc =&qemuCmdLineX86; + else { + if (!strcmp(def->os.arch, "ppc64")) + qemuCmdFunc =&qemuCmdLinePPC64; + }
If we get an arch that is not PPC or x86, then 'qemuCmdFunc' can end up NULL
- smbioscmd = qemuBuildSmbiosBiosStr(source); - if (smbioscmd != NULL) { - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); - VIR_FREE(smbioscmd); - } - smbioscmd = qemuBuildSmbiosSystemStr(source, skip_uuid); - if (smbioscmd != NULL) { - virCommandAddArgList(cmd, "-smbios", smbioscmd, NULL); - VIR_FREE(smbioscmd); - } - } + if (qemuCmdFunc->qemuBuildArchFunction) {
Which will cause a SEGV here.
My bad, I should have had something like: if (qemuCmdFunc && qemuCmdFunc->qemubuildArchFunction) ... I'll correct this.
... [snip] ....
The convention we try to follow is that if there is something in the XML which is not supported by the QEMU we're about to run, then we should raise an error VIR_ERR_CONFIG_UNSUPPORTED.
Thus I'd expect to see a PPC method which raises such an error if the user requests no-ACPI, serial BIOS output, or SMBIOS data.
Thank you for explaining this. I get your point -- libvirt should be able to flag an error if the user tries to specify in XML, a feature unsupported by qemu. To check if qemu supports a certain feature, libvirt parses the -help string which is generated by running the qemu binary with the '-h' argument. Example: For libvirt to determine if '-no-acpi' is a valid option, it parses the qemu help string to determine if '-no-acpi' is a supported feature. If this option is found in the help string, libvirt assumes that this is an allowed option. However, qemu itself has an inherent limitation. When generating the list of allowed options with the '-h' flags, qemu blindly lists all options instead of doing any arch-specific checking. Example, 'no-acpi' is defined with an architecture mask for only x86. $cat $QEMU_GIT/qemu-options.hx : ..... DEF("no-acpi", 0, QEMU_OPTION_no_acpi, "-no-acpi disable ACPI\n", QEMU_ARCH_I386) STEXI @item -no-acpi @findex -no-acpi Disable ACPI (Advanced Configuration and Power Interface) support. Use it if your guest OS complains about ACPI problems (PC target machine only). ETEXI .... However, the arch-specific masks are not checked while generating the help string. $cat $QEMU_GIT/vl.c: ... const char *options_help = #define DEF(option, opt_arg, opt_enum, opt_help, arch_mask) \ opt_help #define DEFHEADING(text) stringify(text) "\n" #include "qemu-options.def" .... (where qemu-options.def is generated from qemu-options.hx) This essentially means that the 'help' string emitted by qemu is identical, irrespective of the architecture. So, even if a qemu binary meant for a non-x86 architecture may support a subset of qemu's x86 features, the qemu binary for that architecture will always display an identical help string as x86-- never advertising its true features. This will trick libvirt, because launching qemu-system-ppc64 or qemu-system-mips with '-no-acpi' errors out with: "Option no-acpi not supported for this target" There is also a second part of this problem. Libvirt till now has been x86-based only, and so it probes host-architecture qemu to gather supported features, and continues to use these while validating a guest's features. For example, libvirt should make a decision about processing 'acpi' XML feature only if the guest architecture fundamentally supports this. At present, libvirt on x86 assumes acpi to be a mandatory feature in any guest XML. In the absence of the <acpi> feature, it adds 'no-acpi' in the qemu command line. On an x86 host, while this might be fine for a guest running via qemu-system-x86_64 , it will not be right for a guest running via qemu-system-sparc. Considering libvirt is potentially capable of launching pure emulation based cross-architecture guests, it would be an incorrect assumption. The decision to even process the 'acpi' tag should be taken based on whether the guest architecture supports this -- this patch attempts to accomplish this. This patch transfers the decision of processing an XML tag to an arch-specific handler. In the absence of this handler, the option is silently ignored.
Finally, looking at the changes, I'd be somewhat amazed if this refactoring has not broken the test suite since AFAICT, it changes the ordering of some command line arguments. Did 'make check' really pass after applying this ?
I'm sorry, I'd tested this by hand and did not run a 'make check' at the time. Ran it now to see that 59 tests pass and 3 tests fail. I will remember to run 'make check' henceforth.
... [snip] ... +typedef struct _virQemuCommandLineFunction virQemuCommandLineFunction; +typedef struct _virQemuCommandLineFunction* virQemuCommandLineFunctionPtr;
These can both be in the source file too. Also the naming convention for the QEMU driver source files is a prefix 'qemu' ather than 'virQemu'.
Agree, I'll address this.
@@ -53,6 +63,16 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, enum virVMOperationType vmop) ATTRIBUTE_NONNULL(1);
+/* Build Additional X86-specific options on command line */ +char * qemuBuildX86CommandLine(virConnectPtr conn, + struct qemud_driver *driver, + virDomainDefPtr def, + virBitmapPtr qemuCaps);
I think this function can just be static - there's no need to be able to access it outside the source file it lives in.
Will do. ..[snip].. Shared my understanding on the present limitations of qemu / libvirt; and the need for an arch-based cleanup patch. I would love to stand corrected if I have overlooked any aspect. It will also be nice to hear of suggestions on how I can organize this design better. Thanks, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On 11/14/2011 09:43 AM, Prerna Saxena wrote:
Hi, Recent development in KVM for 64-bit Power ISA Book3S machines, allows users to run multiple KVM guest instances on POWER7 and PPC970 processor based systems. Also qemu-system-ppc64 has been enhanced to support a new machine type "pseries" suitable for Power Book3S machines. This addition effectively brings the KVM+qemu combination to run multiple guest instances on a Power Book3S machine.
Libvirt continues to be the key interface to configure and manage the KVM guest instances on x86. This patch set is an effort to enable libvirt to support KVM guest configuration and management on Power Book3S machines.
Based on community discussion around the earlier version, this patch series augments the present 'kvm' driver to support PowerPC-KVM based guests. Since some of the supported devices vary between architectures, the code is now reworked to dynamically choose the correct handler function based on guest domain architecture at runtime (def->os.arch).
This patch series consists of 4 patches : Patch 1/4 : Use sysfs to gather host topology. Presently libvirt depends on /proc/cpuinfo gather CPU, cores, threads, etc This is highly architecture dependent. A alternative is to use sysfs, which provides a platform-neutral interface to parse CPU topology. Patch 2/4 : Add PowerPC CPU Driver Patch 3/4 : Add support for qemu-system-ppc64 Patch 4/4 : Refactor qemu commmand-line generation to separate out arch- specific options from generic command line.
Changelog from v1: * Patches 1,2,3 unchanged ; The hacks in Patch 4 of v1 have been replaced by a new patch to neatly select arch-specific features.
Awaiting comments and feedback, A few quick comments:
Run 'make syntax-check'. Some cleanup needs to be done. Also use ATTRIBUTE_UNUSED on variables that are not use, like here when compiling on x86. cpu/cpu_powerpc.c: In function 'PowerPCDecode': cpu/cpu_powerpc.c:50:28: warning: unused parameter 'cpu' [-Wunused-parameter] cpu/cpu_powerpc.c:51:36: warning: unused parameter 'data' [-Wunused-parameter] cpu/cpu_powerpc.c:52:28: warning: unused parameter 'models' [-Wunused-parameter] cpu/cpu_powerpc.c:53:28: warning: unused parameter 'nmodels' [-Wunused-parameter] cpu/cpu_powerpc.c:54:27: warning: unused parameter 'preferred' [-Wunused-parameter] cpu/cpu_powerpc.c: At top level: cpu/cpu_powerpc.c:75:5: warning: initialization from incompatible pointer type cpu/cpu_powerpc.c: In function 'PowerPCDataFree': cpu/cpu_powerpc.c:66:1: warning: control reaches end of non-void function [-Wreturn-type] nodeinfo.c: In function 'linuxNodeInfoCPUPopulate': nodeinfo.c:287:5: warning: suggest parentheses around assignment used as truth value [-Wparentheses] nodeinfo.c:202:36: warning: unused parameter 'need_hyperthreads' [-Wunused-parameter] qemu/qemu_command.c: In function 'qemuBuildX86CommandLine': qemu/qemu_command.c:5250:17: warning: too many arguments for format [-Wformat-extra-args] qemu/qemu_command.c:5201:41: warning: unused parameter 'conn' [-Wunused-parameter] qemu/qemu_command.c: In function 'qemuBuildCommandLine': qemu/qemu_command.c:3264:35: warning: 'qemuCmdFunc' may be used uninitialized in this function [-Wuninitialized] Some of the code (part 4) when I looked seemed to have indentation problems. Stefan
On 11/15/2011 02:36 AM, Stefan Berger wrote:
On 11/14/2011 09:43 AM, Prerna Saxena wrote:
Hi, Recent development in KVM for 64-bit Power ISA Book3S machines, allows users to run multiple KVM guest instances on POWER7 and PPC970 processor based systems. Also qemu-system-ppc64 has been enhanced to support a new machine type "pseries" suitable for Power Book3S machines. This addition effectively brings the KVM+qemu combination to run multiple guest instances on a Power Book3S machine.
Libvirt continues to be the key interface to configure and manage the KVM guest instances on x86. This patch set is an effort to enable libvirt to support KVM guest configuration and management on Power Book3S machines.
Based on community discussion around the earlier version, this patch series augments the present 'kvm' driver to support PowerPC-KVM based guests. Since some of the supported devices vary between architectures, the code is now reworked to dynamically choose the correct handler function based on guest domain architecture at runtime (def->os.arch).
This patch series consists of 4 patches : Patch 1/4 : Use sysfs to gather host topology. Presently libvirt depends on /proc/cpuinfo gather CPU, cores, threads, etc This is highly architecture dependent. A alternative is to use sysfs, which provides a platform-neutral interface to parse CPU topology. Patch 2/4 : Add PowerPC CPU Driver Patch 3/4 : Add support for qemu-system-ppc64 Patch 4/4 : Refactor qemu commmand-line generation to separate out arch- specific options from generic command line.
Changelog from v1: * Patches 1,2,3 unchanged ; The hacks in Patch 4 of v1 have been replaced by a new patch to neatly select arch-specific features.
Awaiting comments and feedback, A few quick comments:
Run 'make syntax-check'. Some cleanup needs to be done.
Also use ATTRIBUTE_UNUSED on variables that are not use, like here when compiling on x86.
cpu/cpu_powerpc.c: In function 'PowerPCDecode': cpu/cpu_powerpc.c:50:28: warning: unused parameter 'cpu' [-Wunused-parameter] cpu/cpu_powerpc.c:51:36: warning: unused parameter 'data' [-Wunused-parameter] cpu/cpu_powerpc.c:52:28: warning: unused parameter 'models' [-Wunused-parameter] cpu/cpu_powerpc.c:53:28: warning: unused parameter 'nmodels' [-Wunused-parameter] cpu/cpu_powerpc.c:54:27: warning: unused parameter 'preferred' [-Wunused-parameter] cpu/cpu_powerpc.c: At top level: cpu/cpu_powerpc.c:75:5: warning: initialization from incompatible pointer type cpu/cpu_powerpc.c: In function 'PowerPCDataFree': cpu/cpu_powerpc.c:66:1: warning: control reaches end of non-void function [-Wreturn-type]
nodeinfo.c: In function 'linuxNodeInfoCPUPopulate': nodeinfo.c:287:5: warning: suggest parentheses around assignment used as truth value [-Wparentheses] nodeinfo.c:202:36: warning: unused parameter 'need_hyperthreads' [-Wunused-parameter]
qemu/qemu_command.c: In function 'qemuBuildX86CommandLine': qemu/qemu_command.c:5250:17: warning: too many arguments for format [-Wformat-extra-args] qemu/qemu_command.c:5201:41: warning: unused parameter 'conn' [-Wunused-parameter] qemu/qemu_command.c: In function 'qemuBuildCommandLine': qemu/qemu_command.c:3264:35: warning: 'qemuCmdFunc' may be used uninitialized in this function [-Wuninitialized]
Some of the code (part 4) when I looked seemed to have indentation problems.
Stefan
Hi Stefan, Thank you for your comments. I will fix these warnings, and use ATTRIBUTE_UNUSED for variables not in use. Regards, -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Prerna Saxena -
Stefan Berger