[libvirt] [RFC v3 PATCH 0/5] PowerPC : Extend libvirt for the PowerPC-KVM platform
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, libvirt must be capable of choosing supported device backends and defaults for each architecture in qemu. To check if qemu supports a certain feature, libvirt at present parses the -help string which is generated by running the qemu binary with the '-h' argument. This approach is gated by QEMU's inherent limitation. When generating the list of allowed options with the '-h' flags, qemu today blindly lists all options defined for any architecture/platform instead of doing any arch-specific checking. This tricks libvirt into assuming a much bigger set of host capabilities than is actually available. Ideally, it would be good to have qemu specify a list of devices for a given architecture and platform which libvirt can parse to understand supported capabilities for that guest. As a part of this patchset, there is an attempt to cleanly bifurcate libvirt code and to remove x86-specific assumptions from generic qemu commandline code. Series Description: ------------------- This patch series consists of 5 patches : Patch 1/5 : Use sysfs to gather host topology in place of /proc/cpuinfo. Patch 2/5 : Add PowerPC CPU Driver Patch 3/5 : Add support for qemu-system-ppc64 Patch 4/5 : Clean up x86-specific assumptions from generic qemu code. Patch 5/5 : Add address family "spapr-vio" Changelog: --------- ** v1->v2 : * Patches 1,2,3 unchanged ; The hacks in Patch 4 of v1 replaced by a new patch to neatly select arch-specific features. ** v2->v3 : * Patches 1,2,3 have minor cleanups ; Patch 4 no longer has an arch-specific handler routine. It is now replaced by a much simpler patch that merely removes x86/pc-specific assumptions from libvirt. ** v3->v4 : * Patches 1,2,3,4 unchanged ; patch 5 is a new addition from Michael Ellerman that adds a new device-tree based addressing mechanism for the 'pseries' guest. -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:45:30 -0700 Subject: [PATCH 1/5] Use sysfs to gather host topology, in place of /proc/cpuinfo Libvirt at present depends on /proc/cpuinfo to gather host details such as CPUs, cores, threads, etc. This is an architecture- dependent approach. An alternative is to use 'Sysfs', which provides a platform-agnostic interface to parse host CPU topology. Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/nodeinfo.c | 114 +++++++++++++++++++------------------------------------- 1 files changed, 39 insertions(+), 75 deletions(-) diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 6448b79..f70654a 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 @@ -67,8 +68,7 @@ /* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - virNodeInfoPtr nodeinfo, - bool need_hyperthreads); + virNodeInfoPtr nodeinfo); static int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, @@ -191,23 +191,26 @@ 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) + virNodeInfoPtr nodeinfo) { char line[1024]; 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 +224,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 +242,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 +259,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 +293,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 +326,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")); @@ -617,7 +581,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { _("cannot open %s"), CPUINFO_PATH); return -1; } - ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo, true); + ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo); VIR_FORCE_FCLOSE(cpuinfo); if (ret < 0) return -1; -- 1.7.7 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On Tue, Nov 29, 2011 at 08:26:57PM +0530, Prerna Saxena wrote:
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:45:30 -0700 Subject: [PATCH 1/5] Use sysfs to gather host topology, in place of /proc/cpuinfo
Libvirt at present depends on /proc/cpuinfo to gather host details such as CPUs, cores, threads, etc. This is an architecture- dependent approach. An alternative is to use 'Sysfs', which provides a platform-agnostic interface to parse host CPU topology.
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/nodeinfo.c | 114 +++++++++++++++++++------------------------------------- 1 files changed, 39 insertions(+), 75 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 6448b79..f70654a 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 @@ -67,8 +68,7 @@
/* NB, this is not static as we need to call it from the testsuite */ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, - virNodeInfoPtr nodeinfo, - bool need_hyperthreads); + virNodeInfoPtr nodeinfo);
static int linuxNodeGetCPUStats(FILE *procstat, int cpuNum, @@ -191,23 +191,26 @@ 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) + virNodeInfoPtr nodeinfo) { char line[1024]; 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 +224,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 +242,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 +259,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 +293,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 +326,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")); @@ -617,7 +581,7 @@ int nodeGetInfo(virConnectPtr conn ATTRIBUTE_UNUSED, virNodeInfoPtr nodeinfo) { _("cannot open %s"), CPUINFO_PATH); return -1; } - ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo, true); + ret = linuxNodeInfoCPUPopulate(cpuinfo, nodeinfo); VIR_FORCE_FCLOSE(cpuinfo); if (ret < 0) return -1;
There's nothing particularly wrong with the idea here, but as Stefan mentions this has broken the test suite, because the nodeinfotest.c now tries to read /sys from the test machine, instead of reading our dummy data files in tests/nodeinfodata/ I think we'd probably need to create some dummy sysfs trees under tests/nodeinfodata/test1/sys/..../ and make sure we can pass in a path to the linuxNodeInfoCPUPopulate API so we cna point the test at our sysfs, instead of the real machine 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/30/2011 06:00 PM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 08:26:57PM +0530, Prerna Saxena wrote:
From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:45:30 -0700 Subject: [PATCH 1/5] Use sysfs to gather host topology, in place of /proc/cpuinfo
Libvirt at present depends on /proc/cpuinfo to gather host details such as CPUs, cores, threads, etc. This is an architecture- dependent approach. An alternative is to use 'Sysfs', which provides a platform-agnostic interface to parse host CPU topology.
Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> --- src/nodeinfo.c | 114 +++++++++++++++++++------------------------------------- 1 files changed, 39 insertions(+), 75 deletions(-)
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 6448b79..f70654a 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c [...snip...] There's nothing particularly wrong with the idea here, but as Stefan mentions this has broken the test suite, because the nodeinfotest.c now tries to read /sys from the test machine, instead of reading our dummy data files in tests/nodeinfodata/
I think we'd probably need to create some dummy sysfs trees under tests/nodeinfodata/test1/sys/..../ and make sure we can pass in a path to the linuxNodeInfoCPUPopulate API so we cna point the test at our sysfs, instead of the real machine
Daniel
Thanks for the pointer. I'll now modify this test to use sysfs. -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:56:20 -0700 Subject: [PATCH 2/5] Add PPC cpu driver. To add support for running libvirt on PowerPC, a CPU driver for the PowerPC platform must be added. Most generic cpu driver routines such as CPU compare, decode, etc are based on CPUID comparison and are not relevant for non-x86 platforms. Here, we introduce stubs for relevant PowerPC routines invoked by libvirt. 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(-) create mode 100644 src/cpu/cpu_powerpc.c create mode 100644 src/cpu/cpu_powerpc.h diff --git a/src/Makefile.am b/src/Makefile.am index 33a32a8..aea3ddd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -533,7 +533,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..ed81694 --- /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 ATTRIBUTE_UNUSED, + const union cpuData *data ATTRIBUTE_UNUSED, + const char **models ATTRIBUTE_UNUSED, + unsigned int nmodels ATTRIBUTE_UNUSED, + const char *preferred ATTRIBUTE_UNUSED) +{ + return 0; +} + +static void +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return; + + 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
On 11/29/2011 09:58 AM, Prerna Saxena wrote:
From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:56:20 -0700 Subject: [PATCH 2/5] Add PPC cpu driver.
To add support for running libvirt on PowerPC, a CPU driver for the PowerPC platform must be added. Most generic cpu driver routines such as CPU compare, decode, etc are based on CPUID comparison and are not relevant for non-x86 platforms. Here, we introduce stubs for relevant PowerPC routines invoked by libvirt.
Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> Signed-off-by: Anton Blanchard<anton@au.ibm.com>
ACK
On Tue, Nov 29, 2011 at 08:28:55PM +0530, Prerna Saxena wrote:
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:56:20 -0700 Subject: [PATCH 2/5] Add PPC cpu driver.
To add support for running libvirt on PowerPC, a CPU driver for the PowerPC platform must be added. Most generic cpu driver routines such as CPU compare, decode, etc are based on CPUID comparison and are not relevant for non-x86 platforms. Here, we introduce stubs for relevant PowerPC routines invoked by libvirt.
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(-) create mode 100644 src/cpu/cpu_powerpc.c create mode 100644 src/cpu/cpu_powerpc.h
diff --git a/src/Makefile.am b/src/Makefile.am index 33a32a8..aea3ddd 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -533,7 +533,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..ed81694 --- /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 ATTRIBUTE_UNUSED, + const union cpuData *data ATTRIBUTE_UNUSED, + const char **models ATTRIBUTE_UNUSED, + unsigned int nmodels ATTRIBUTE_UNUSED, + const char *preferred ATTRIBUTE_UNUSED) +{ + return 0; +} + +static void +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return; + + 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__ */
ACK 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 :|
On 11/30/2011 05:34 AM, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 08:28:55PM +0530, Prerna Saxena wrote:
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 05:56:20 -0700 Subject: [PATCH 2/5] Add PPC cpu driver.
To add support for running libvirt on PowerPC, a CPU driver for the PowerPC platform must be added. Most generic cpu driver routines such as CPU compare, decode, etc are based on CPUID comparison and are not relevant for non-x86 platforms. Here, we introduce stubs for relevant PowerPC routines invoked by libvirt.
ACK
Pushed, as well as adding you to AUTHORS. I'm still waiting for v4 on 1/5 that addresses Daniel's comments. Let me know if I need to adjust any preferred spelling. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 06:01:33 -0700 Subject: [PATCH 3/5] Add support for ppc64 qemu This enables libvirt to select the correct qemu binary (qemu-system-ppc64) for a guest vm based on arch 'ppc64'. Also, libvirt is enabled to correctly parse the list of supported PowerPC CPUs, generated by running 'qemu-system-ppc64 -cpu ?' 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 c5fe41d..c2d3d93 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
On 11/29/2011 09:59 AM, Prerna Saxena wrote:
From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 06:01:33 -0700 Subject: [PATCH 3/5] Add support for ppc64 qemu
This enables libvirt to select the correct qemu binary (qemu-system-ppc64) for a guest vm based on arch 'ppc64'. Also, libvirt is enabled to correctly parse the list of supported PowerPC CPUs, generated by running 'qemu-system-ppc64 -cpu ?'
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 c5fe41d..c2d3d93 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; If what you are parsing here is indeed corrupted, you may end up in an endless loop with p not moving anymore. You have to advance 'p' at least by 1 and have a check before whether *p == '\0' to get out of it. You may also want to put this check before the calculation of 'next'. + + /* 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; *p = '\0' presumably is end-of-string -- shouldn't we 'break' here ? + + if (retcpus) { if retcpus is not provided you may want to return -1 earlier + unsigned int len; + + if (VIR_REALLOC_N(cpus, count + 1)< 0) virReportOOMError(); + goto error; + + if (t) + len = t - p - 1; len would remain uninitialized if t is not set, but t at this point must be set due to the check above. Remove 'if(t)'. + + if (!(cpus[count] = strndup(p, len))) virReportOOMError(); + 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;
Hi Stefan, Thanks for reviewing the patch. On 11/29/2011 10:25 PM, Stefan Berger wrote:
On 11/29/2011 09:59 AM, Prerna Saxena wrote:
From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 06:01:33 -0700 Subject: [PATCH 3/5] Add support for ppc64 qemu
This enables libvirt to select the correct qemu binary (qemu-system-ppc64) for a guest vm based on arch 'ppc64'. Also, libvirt is enabled to correctly parse the list of supported PowerPC CPUs, generated by running 'qemu-system-ppc64 -cpu ?'
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 c5fe41d..c2d3d93 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; If what you are parsing here is indeed corrupted, you may end up in an endless loop with p not moving anymore. You have to advance 'p' at least by 1 and have a check before whether *p == '\0' to get out of it. You may also want to put this check before the calculation of 'next'.
When the execution gets to a 'continue', the do..while loop will resume execution from next iteration after processing the conditional check. In this case, whe condition in 'while' loop is : while ((p =next)) which automatically advances 'p'.
+ + /* 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; *p = '\0' presumably is end-of-string -- shouldn't we 'break' here ?
Agree. The reason it works fine in its present form is 'cos when p becomes empty, 'next' is empty too, and the loop condition fails which causes the do..while loop to terminate.
+ + if (retcpus) { if retcpus is not provided you may want to return -1 earlier + unsigned int len; + + if (VIR_REALLOC_N(cpus, count + 1)< 0) virReportOOMError(); + goto error; + + if (t) + len = t - p - 1; len would remain uninitialized if t is not set, but t at this point must be set due to the check above. Remove 'if(t)'.
Will do.
+ + if (!(cpus[count] = strndup(p, len))) virReportOOMError();
I need to explicitly free cpus[] array, hence hadnt invoked this routine. I'll call this just before return '-1'.
+ 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;
-- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On Tue, Nov 29, 2011 at 08:29:56PM +0530, Prerna Saxena wrote:
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 3 Oct 2011 06:01:33 -0700 Subject: [PATCH 3/5] Add support for ppc64 qemu
This enables libvirt to select the correct qemu binary (qemu-system-ppc64) for a guest vm based on arch 'ppc64'. Also, libvirt is enabled to correctly parse the list of supported PowerPC CPUs, generated by running 'qemu-system-ppc64 -cpu ?'
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 c5fe41d..c2d3d93 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;
Need a virReportOOMError() call here
+ + if (t) + len = t - p - 1; + + if (!(cpus[count] = strndup(p, len))) + goto error;
Need a virReportOOMError() call here too
+ } + 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;
I know we don't have any test case for the x86 CPU parser here either, but it would be desirable to create a test case for the PPC parser while doing this. eg, save the QEMU -cpu? output into a text file in the tests/ directory, and invoke qemuCapsParsePPCModels() on it and validate the result. 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 :|
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 21 Nov 2011 18:20:42 +0530 Subject: [PATCH 4/5] Clean up qemuBuildCommandLine to remove x86-specific assumptions from generic code. This implements the minimal set of changes needed in libvirt to launch a PowerPC-KVM based guest. It removes x86-specific assumptions about choice of serial driver backend from generic qemu guest commandline generation code. It also restricts the ACPI capability to be available for an x86 or x86_64 domain. This is not a complete solution -- it still does not guarantee libvirt the capability to flag non-supported options in guest XML. (Eg, an ACPI specification in a PowerPC guest XML will still get processed, even though qemu-system-ppc64 does not support it while qemu-system-x86_64 does.) This drawback exists because libvirt falls back on qemu to query supported features, and qemu '-h' blindly lists all capabilities -- irrespective of whether they are available while emulating a given architecture or not. The long-term solution would be for qemu to list out capabilities based on architecture and platform -- so that libvirt can cleanly make out what devices are supported on an arch (say 'ppc64') and platform (say, 'mac99'). Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 39 ++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 6 ++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 85b34bb..57b25d6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4550,8 +4550,11 @@ 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, + def->os.machine))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -5438,6 +5441,34 @@ qemuBuildCommandLine(virConnectPtr conn, return NULL; } +/* This function generates the correct '-device' string for character + * devices of each architecture. + */ +char * +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + char *os_arch, + char *machine) +{ + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + if (STREQ(os_arch, "ppc64") && STREQ(machine, "pseries")) + 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 @@ -6649,7 +6680,9 @@ 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 (STREQ(def->os.arch, "i686")||STREQ(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 dbe2fb2..1fe0394 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -53,6 +53,12 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, enum virNetDevVPortProfileOp vmop) ATTRIBUTE_NONNULL(1); +/* Generate string for arch-specific '-device' parameter */ +char * +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + char *os_arch, + char *machine); + /* 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 11/29/2011 10:01 AM, Prerna Saxena wrote:
From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 21 Nov 2011 18:20:42 +0530 Subject: [PATCH 4/5] Clean up qemuBuildCommandLine to remove x86-specific assumptions from generic code.
This implements the minimal set of changes needed in libvirt to launch a PowerPC-KVM based guest. It removes x86-specific assumptions about choice of serial driver backend from generic qemu guest commandline generation code. It also restricts the ACPI capability to be available for an x86 or x86_64 domain. This is not a complete solution -- it still does not guarantee libvirt the capability to flag non-supported options in guest XML. (Eg, an ACPI specification in a PowerPC guest XML will still get processed, even though qemu-system-ppc64 does not support it while qemu-system-x86_64 does.) This drawback exists because libvirt falls back on qemu to query supported features, and qemu '-h' blindly lists all capabilities -- irrespective of whether they are available while emulating a given architecture or not. The long-term solution would be for qemu to list out capabilities based on architecture and platform -- so that libvirt can cleanly make out what devices are supported on an arch (say 'ppc64') and platform (say, 'mac99').
Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 39 ++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 6 ++++++ 2 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 85b34bb..57b25d6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4550,8 +4550,11 @@ 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, + def->os.machine))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -5438,6 +5441,34 @@ qemuBuildCommandLine(virConnectPtr conn, return NULL; }
+/* This function generates the correct '-device' string for character + * devices of each architecture. + */ +char * +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + char *os_arch, + char *machine) +{ + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + if (STREQ(os_arch, "ppc64")&& STREQ(machine, "pseries")) + 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 @@ -6649,7 +6680,9 @@ 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 (STREQ(def->os.arch, "i686")||STREQ(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 dbe2fb2..1fe0394 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -53,6 +53,12 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, enum virNetDevVPortProfileOp vmop) ATTRIBUTE_NONNULL(1);
+/* Generate string for arch-specific '-device' parameter */ +char * +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + char *os_arch, + char *machine); Remove the space before '('. + /* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, char type_sep,
On 11/29/2011 10:27 PM, Stefan Berger wrote:
On 11/29/2011 10:01 AM, Prerna Saxena wrote:
From: Prerna Saxena<prerna@linux.vnet.ibm.com> Date: Mon, 21 Nov 2011 18:20:42 +0530 Subject: [PATCH 4/5] Clean up qemuBuildCommandLine to remove x86-specific assumptions from generic code.
This implements the minimal set of changes needed in libvirt to launch a PowerPC-KVM based guest. It removes x86-specific assumptions about choice of serial driver backend from generic qemu guest commandline generation code. It also restricts the ACPI capability to be available for an x86 or x86_64 domain. This is not a complete solution -- it still does not guarantee libvirt the capability to flag non-supported options in guest XML. (Eg, an ACPI specification in a PowerPC guest XML will still get processed, even though qemu-system-ppc64 does not support it while qemu-system-x86_64 does.) This drawback exists because libvirt falls back on qemu to query supported features, and qemu '-h' blindly lists all capabilities -- irrespective of whether they are available while emulating a given architecture or not. The long-term solution would be for qemu to list out capabilities based on architecture and platform -- so that libvirt can cleanly make out what devices are supported on an arch (say 'ppc64') and platform (say, 'mac99').
Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 39 ++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 6 ++++++ 2 files changed, 42 insertions(+), 3 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 85b34bb..57b25d6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4550,8 +4550,11 @@ 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, + def->os.machine))) + goto error; + virCommandAddArg(cmd, devstr); + VIR_FREE(devstr); } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -5438,6 +5441,34 @@ qemuBuildCommandLine(virConnectPtr conn, return NULL; }
+/* This function generates the correct '-device' string for character + * devices of each architecture. + */ +char * +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + char *os_arch, + char *machine) +{ + virBuffer cmd = VIR_BUFFER_INITIALIZER; + + if (STREQ(os_arch, "ppc64")&& STREQ(machine, "pseries")) + 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 @@ -6649,7 +6680,9 @@ 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 (STREQ(def->os.arch, "i686")||STREQ(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 dbe2fb2..1fe0394 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -53,6 +53,12 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, enum virNetDevVPortProfileOp vmop) ATTRIBUTE_NONNULL(1);
+/* Generate string for arch-specific '-device' parameter */ +char * +qemuBuildChrDeviceStr (virDomainChrDefPtr serial, + char *os_arch, + char *machine); Remove the space before '('.
Thanks for pointing this out.. it had escaped 'make syntax-check' too ! I'll fix this.
+ /* With vlan == -1, use netdev syntax, else old hostnet */ char * qemuBuildHostNetStr(virDomainNetDefPtr net, char type_sep,
-- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On Tue, Nov 29, 2011 at 08:31:05PM +0530, Prerna Saxena wrote:
From: Prerna Saxena <prerna@linux.vnet.ibm.com> Date: Mon, 21 Nov 2011 18:20:42 +0530 Subject: [PATCH 4/5] Clean up qemuBuildCommandLine to remove x86-specific assumptions from generic code.
This implements the minimal set of changes needed in libvirt to launch a PowerPC-KVM based guest. It removes x86-specific assumptions about choice of serial driver backend from generic qemu guest commandline generation code. It also restricts the ACPI capability to be available for an x86 or x86_64 domain. This is not a complete solution -- it still does not guarantee libvirt the capability to flag non-supported options in guest XML. (Eg, an ACPI specification in a PowerPC guest XML will still get processed, even though qemu-system-ppc64 does not support it while qemu-system-x86_64 does.) This drawback exists because libvirt falls back on qemu to query supported features, and qemu '-h' blindly lists all capabilities -- irrespective of whether they are available while emulating a given architecture or not. The long-term solution would be for qemu to list out capabilities based on architecture and platform -- so that libvirt can cleanly make out what devices are supported on an arch (say 'ppc64') and platform (say, 'mac99').
Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/qemu/qemu_command.c | 39 ++++++++++++++++++++++++++++++++++++--- src/qemu/qemu_command.h | 6 ++++++ 2 files changed, 42 insertions(+), 3 deletions(-)
/* * This method takes a string representing a QEMU command line ARGV set @@ -6649,7 +6680,9 @@ 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 (STREQ(def->os.arch, "i686")||STREQ(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;
This is the cause of one of the tes case failures Stefan mentions. At this point in the function, we have not set def->os.arch to any value. You'll need to move this code right down to the end of this function instead, and add a check for def->os.arch being NULL too for safety. 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 :|
From: Michael Ellerman <michael@ellerman.id.au> Date: Tue, 29 Nov 2011 13:48:09 +0530 Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on the same bus. For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices. This patch adds a new address type "spapr-vio", which achieves this. The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device. Additionally, this patch forces an "spapr-vscsi" controller to be selected for a guest running on architecture 'ppc64' and platform 'pseries'. Signed-off-by: Michael Ellerman <michael@ellerman.id.au> Signed-off-by: Prerna Saxena <prerna@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 18 ++++++++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 2 +- 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ea99f7..5c3809e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "drive", "virtio-serial", "ccid", - "usb") + "usb", + "spapr-vio") VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti, VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST, @@ -1844,6 +1845,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.usb.port); break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg); + break; + default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown address type '%d'"), info->type); @@ -2103,6 +2109,34 @@ cleanup: } static int +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node, + virDomainDeviceSpaprVioAddressPtr addr) +{ + char *reg; + int ret; + + memset(addr, 0, sizeof(*addr)); + addr->has_reg = false; + + reg = virXMLPropString(node, "reg"); + if (reg) { + if (virStrToLong_ull(reg, NULL, 16, &addr->reg) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse <address> 'reg' attribute")); + ret = -1; + goto cleanup; + } + + addr->has_reg = true; + } + + ret = 0; +cleanup: + VIR_FREE(reg); + return ret; +} + +static int virDomainDeviceUSBMasterParseXML(xmlNodePtr node, virDomainDeviceUSBMasterPtr master) { @@ -2215,6 +2249,11 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break; + case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (virDomainDeviceSpaprVioAddressParseXML(address, &info->addr.spaprvio) < 0) + goto cleanup; + break; + default: /* Should not happen */ virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -3048,6 +3087,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, } if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Controllers must use the 'pci' address type")); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4439f55..b1f9260 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -69,6 +69,7 @@ enum virDomainDeviceAddressType { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST }; @@ -121,6 +122,13 @@ struct _virDomainDeviceUSBAddress { char *port; }; +typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress; +typedef virDomainDeviceSpaprVioAddress *virDomainDeviceSpaprVioAddressPtr; +struct _virDomainDeviceSpaprVioAddress { + unsigned long long reg; + bool has_reg; +}; + enum virDomainControllerMaster { VIR_DOMAIN_CONTROLLER_MASTER_NONE, VIR_DOMAIN_CONTROLLER_MASTER_USB, @@ -145,6 +153,7 @@ struct _virDomainDeviceInfo { virDomainDeviceVirtioSerialAddress vioserial; virDomainDeviceCcidAddress ccid; virDomainDeviceUSBAddress usb; + virDomainDeviceSpaprVioAddress spaprvio; } addr; int mastertype; union { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 57b25d6..0d03fbc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1253,6 +1253,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) def->controllers[i]->idx == 0) continue; + if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + continue; if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; if (qemuDomainPCIAddressSetNextAddr(addrs, &def->controllers[i]->info) < 0) @@ -1403,6 +1405,9 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virBufferAsprintf(buf, ",bus="); qemuUsbId(buf, info->addr.usb.bus); virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port); + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, ",reg=%#llx", info->addr.spaprvio.reg); } return 0; @@ -2087,7 +2092,8 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, } char * -qemuBuildControllerDevStr(virDomainControllerDefPtr def, +qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller) { @@ -2095,7 +2101,11 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def, switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - virBufferAddLit(&buf, "lsi"); + if (STREQ(domainDef->os.arch, "ppc64") && STREQ(domainDef->os.machine, "pseries")) { + virBufferAddLit(&buf, "spapr-vscsi"); + } else { + virBufferAddLit(&buf, "lsi"); + } virBufferAsprintf(&buf, ",id=scsi%d", def->idx); break; @@ -4009,7 +4019,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *devstr; virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(cont, qemuCaps, NULL))) + if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, NULL))) goto error; virCommandAddArg(cmd, devstr); @@ -4028,7 +4038,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); char *devstr; - if (!(devstr = qemuBuildControllerDevStr(def->controllers[i], qemuCaps, + if (!(devstr = qemuBuildControllerDevStr(def, def->controllers[i], qemuCaps, &usbcontroller))) goto error; diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1fe0394..3978b2b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -95,7 +95,8 @@ char * qemuBuildDriveDevStr(virDomainDiskDefPtr disk, char * qemuBuildFSDevStr(virDomainFSDefPtr fs, virBitmapPtr qemuCaps); /* Current, best practice */ -char * qemuBuildControllerDevStr(virDomainControllerDefPtr def, +char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 96c0070..eabfeaa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -329,7 +329,7 @@ int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, goto cleanup; } - if (!(devstr = qemuBuildControllerDevStr(controller, priv->qemuCaps, NULL))) { + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) { goto cleanup; } } -- 1.7.7 -- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On 11/29/2011 10:03 AM, Prerna Saxena wrote:
From: Michael Ellerman<michael@ellerman.id.au> Date: Tue, 29 Nov 2011 13:48:09 +0530 Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on the same bus.
For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices.
This patch adds a new address type "spapr-vio", which achieves this.
The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device.
Additionally, this patch forces an "spapr-vscsi" controller to be selected for a guest running on architecture 'ppc64' and platform 'pseries'.
Signed-off-by: Michael Ellerman<michael@ellerman.id.au> Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 18 ++++++++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 2 +- 5 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ea99f7..5c3809e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "drive", "virtio-serial", "ccid", - "usb") + "usb", + "spapr-vio")
VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti, VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST, @@ -1844,6 +1845,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.usb.port); break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg); + break; + default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown address type '%d'"), info->type); @@ -2103,6 +2109,34 @@ cleanup: }
static int +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node, + virDomainDeviceSpaprVioAddressPtr addr) +{ + char *reg; + int ret; + + memset(addr, 0, sizeof(*addr)); + addr->has_reg = false; + + reg = virXMLPropString(node, "reg"); + if (reg) { + if (virStrToLong_ull(reg, NULL, 16,&addr->reg)< 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse<address> 'reg' attribute")); + ret = -1; + goto cleanup; + } + + addr->has_reg = true; + } + + ret = 0; +cleanup: + VIR_FREE(reg); + return ret; +} + +static int virDomainDeviceUSBMasterParseXML(xmlNodePtr node, virDomainDeviceUSBMasterPtr master) { @@ -2215,6 +2249,11 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (virDomainDeviceSpaprVioAddressParseXML(address,&info->addr.spaprvio)< 0) + goto cleanup; + break; + default: /* Should not happen */ virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -3048,6 +3087,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, }
if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE&& + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO&& def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Controllers must use the 'pci' address type")); Is this still the proper error message for this type of bus? You may want to look at def->os.arch / def->os.machine to see what is expected here in terms of bus and then have a more specific error message. diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4439f55..b1f9260 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -69,6 +69,7 @@ enum virDomainDeviceAddressType { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST }; @@ -121,6 +122,13 @@ struct _virDomainDeviceUSBAddress { char *port; };
+typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress; +typedef virDomainDeviceSpaprVioAddress *virDomainDeviceSpaprVioAddressPtr; +struct _virDomainDeviceSpaprVioAddress { + unsigned long long reg; + bool has_reg; +}; + enum virDomainControllerMaster { VIR_DOMAIN_CONTROLLER_MASTER_NONE, VIR_DOMAIN_CONTROLLER_MASTER_USB, @@ -145,6 +153,7 @@ struct _virDomainDeviceInfo { virDomainDeviceVirtioSerialAddress vioserial; virDomainDeviceCcidAddress ccid; virDomainDeviceUSBAddress usb; + virDomainDeviceSpaprVioAddress spaprvio; } addr; int mastertype; union { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 57b25d6..0d03fbc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1253,6 +1253,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) def->controllers[i]->idx == 0) continue;
+ if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + continue; if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; if (qemuDomainPCIAddressSetNextAddr(addrs,&def->controllers[i]->info)< 0) @@ -1403,6 +1405,9 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virBufferAsprintf(buf, ",bus="); qemuUsbId(buf, info->addr.usb.bus); virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port); + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, ",reg=%#llx", info->addr.spaprvio.reg); }
return 0; @@ -2087,7 +2092,8 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, }
char * -qemuBuildControllerDevStr(virDomainControllerDefPtr def, +qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller) { @@ -2095,7 +2101,11 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def,
switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - virBufferAddLit(&buf, "lsi"); + if (STREQ(domainDef->os.arch, "ppc64")&& STREQ(domainDef->os.machine, "pseries")) { + virBufferAddLit(&buf, "spapr-vscsi"); + } else { + virBufferAddLit(&buf, "lsi"); + } virBufferAsprintf(&buf, ",id=scsi%d", def->idx); break;
@@ -4009,7 +4019,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *devstr;
virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(cont, qemuCaps, NULL))) + if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, NULL))) goto error;
virCommandAddArg(cmd, devstr); @@ -4028,7 +4038,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device");
char *devstr; - if (!(devstr = qemuBuildControllerDevStr(def->controllers[i], qemuCaps, + if (!(devstr = qemuBuildControllerDevStr(def, def->controllers[i], qemuCaps, &usbcontroller))) goto error;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1fe0394..3978b2b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -95,7 +95,8 @@ char * qemuBuildDriveDevStr(virDomainDiskDefPtr disk, char * qemuBuildFSDevStr(virDomainFSDefPtr fs, virBitmapPtr qemuCaps); /* Current, best practice */ -char * qemuBuildControllerDevStr(virDomainControllerDefPtr def, +char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 96c0070..eabfeaa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -329,7 +329,7 @@ int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, goto cleanup; }
- if (!(devstr = qemuBuildControllerDevStr(controller, priv->qemuCaps, NULL))) { + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) { goto cleanup; } } Otherwise looks good to me.
On 11/29/2011 10:46 PM, Stefan Berger wrote:
On 11/29/2011 10:03 AM, Prerna Saxena wrote:
From: Michael Ellerman<michael@ellerman.id.au> Date: Tue, 29 Nov 2011 13:48:09 +0530 Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on the same bus.
For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices.
This patch adds a new address type "spapr-vio", which achieves this.
The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device.
Additionally, this patch forces an "spapr-vscsi" controller to be selected for a guest running on architecture 'ppc64' and platform 'pseries'.
Signed-off-by: Michael Ellerman<michael@ellerman.id.au> Signed-off-by: Prerna Saxena<prerna@linux.vnet.ibm.com> --- src/conf/domain_conf.c | 42 +++++++++++++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 9 +++++++++ src/qemu/qemu_command.c | 18 ++++++++++++++---- src/qemu/qemu_command.h | 3 ++- src/qemu/qemu_hotplug.c | 2 +- 5 files changed, 67 insertions(+), 7 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3ea99f7..5c3809e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -139,7 +139,8 @@ VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "drive", "virtio-serial", "ccid", - "usb") + "usb", + "spapr-vio")
VIR_ENUM_IMPL(virDomainDeviceAddressPciMulti, VIR_DOMAIN_DEVICE_ADDRESS_PCI_MULTI_LAST, @@ -1844,6 +1845,11 @@ virDomainDeviceInfoFormat(virBufferPtr buf, info->addr.usb.port); break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, " reg='%#llx'", info->addr.spaprvio.reg); + break; + default: virDomainReportError(VIR_ERR_INTERNAL_ERROR, _("unknown address type '%d'"), info->type); @@ -2103,6 +2109,34 @@ cleanup: }
static int +virDomainDeviceSpaprVioAddressParseXML(xmlNodePtr node, + virDomainDeviceSpaprVioAddressPtr addr) +{ + char *reg; + int ret; + + memset(addr, 0, sizeof(*addr)); + addr->has_reg = false; + + reg = virXMLPropString(node, "reg"); + if (reg) { + if (virStrToLong_ull(reg, NULL, 16,&addr->reg)< 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Cannot parse<address> 'reg' attribute")); + ret = -1; + goto cleanup; + } + + addr->has_reg = true; + } + + ret = 0; +cleanup: + VIR_FREE(reg); + return ret; +} + +static int virDomainDeviceUSBMasterParseXML(xmlNodePtr node, virDomainDeviceUSBMasterPtr master) { @@ -2215,6 +2249,11 @@ virDomainDeviceInfoParseXML(xmlNodePtr node, goto cleanup; break;
+ case VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO: + if (virDomainDeviceSpaprVioAddressParseXML(address,&info->addr.spaprvio)< 0) + goto cleanup; + break; + default: /* Should not happen */ virDomainReportError(VIR_ERR_INTERNAL_ERROR, @@ -3048,6 +3087,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, }
if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE&& + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO&& def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Controllers must use the 'pci' address type")); Is this still the proper error message for this type of bus? You may want to look at def->os.arch / def->os.machine to see what is expected here in terms of bus and then have a more specific error message.
Will do.
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4439f55..b1f9260 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -69,6 +69,7 @@ enum virDomainDeviceAddressType { VIR_DOMAIN_DEVICE_ADDRESS_TYPE_VIRTIO_SERIAL, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_CCID, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_USB, + VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO,
VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST }; @@ -121,6 +122,13 @@ struct _virDomainDeviceUSBAddress { char *port; };
+typedef struct _virDomainDeviceSpaprVioAddress virDomainDeviceSpaprVioAddress; +typedef virDomainDeviceSpaprVioAddress *virDomainDeviceSpaprVioAddressPtr; +struct _virDomainDeviceSpaprVioAddress { + unsigned long long reg; + bool has_reg; +}; + enum virDomainControllerMaster { VIR_DOMAIN_CONTROLLER_MASTER_NONE, VIR_DOMAIN_CONTROLLER_MASTER_USB, @@ -145,6 +153,7 @@ struct _virDomainDeviceInfo { virDomainDeviceVirtioSerialAddress vioserial; virDomainDeviceCcidAddress ccid; virDomainDeviceUSBAddress usb; + virDomainDeviceSpaprVioAddress spaprvio; } addr; int mastertype; union { diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 57b25d6..0d03fbc 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1253,6 +1253,8 @@ qemuAssignDevicePCISlots(virDomainDefPtr def, qemuDomainPCIAddressSetPtr addrs) def->controllers[i]->idx == 0) continue;
+ if (def->controllers[i]->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + continue; if (def->controllers[i]->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE) continue; if (qemuDomainPCIAddressSetNextAddr(addrs,&def->controllers[i]->info)< 0) @@ -1403,6 +1405,9 @@ qemuBuildDeviceAddressStr(virBufferPtr buf, virBufferAsprintf(buf, ",bus="); qemuUsbId(buf, info->addr.usb.bus); virBufferAsprintf(buf, ".0,port=%s", info->addr.usb.port); + } else if (info->type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) { + if (info->addr.spaprvio.has_reg) + virBufferAsprintf(buf, ",reg=%#llx", info->addr.spaprvio.reg); }
return 0; @@ -2087,7 +2092,8 @@ qemuBuildUSBControllerDevStr(virDomainControllerDefPtr def, }
char * -qemuBuildControllerDevStr(virDomainControllerDefPtr def, +qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller) { @@ -2095,7 +2101,11 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def,
switch (def->type) { case VIR_DOMAIN_CONTROLLER_TYPE_SCSI: - virBufferAddLit(&buf, "lsi"); + if (STREQ(domainDef->os.arch, "ppc64")&& STREQ(domainDef->os.machine, "pseries")) { + virBufferAddLit(&buf, "spapr-vscsi"); + } else { + virBufferAddLit(&buf, "lsi"); + } virBufferAsprintf(&buf, ",id=scsi%d", def->idx); break;
@@ -4009,7 +4019,7 @@ qemuBuildCommandLine(virConnectPtr conn, char *devstr;
virCommandAddArg(cmd, "-device"); - if (!(devstr = qemuBuildControllerDevStr(cont, qemuCaps, NULL))) + if (!(devstr = qemuBuildControllerDevStr(def, cont, qemuCaps, NULL))) goto error;
virCommandAddArg(cmd, devstr); @@ -4028,7 +4038,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device");
char *devstr; - if (!(devstr = qemuBuildControllerDevStr(def->controllers[i], qemuCaps, + if (!(devstr = qemuBuildControllerDevStr(def, def->controllers[i], qemuCaps, &usbcontroller))) goto error;
diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1fe0394..3978b2b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -95,7 +95,8 @@ char * qemuBuildDriveDevStr(virDomainDiskDefPtr disk, char * qemuBuildFSDevStr(virDomainFSDefPtr fs, virBitmapPtr qemuCaps); /* Current, best practice */ -char * qemuBuildControllerDevStr(virDomainControllerDefPtr def, +char * qemuBuildControllerDevStr(virDomainDefPtr domainDef, + virDomainControllerDefPtr def, virBitmapPtr qemuCaps, int *nusbcontroller);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 96c0070..eabfeaa 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -329,7 +329,7 @@ int qemuDomainAttachPciControllerDevice(struct qemud_driver *driver, goto cleanup; }
- if (!(devstr = qemuBuildControllerDevStr(controller, priv->qemuCaps, NULL))) { + if (!(devstr = qemuBuildControllerDevStr(vm->def, controller, priv->qemuCaps, NULL))) { goto cleanup; } } Otherwise looks good to me.
-- Prerna Saxena Linux Technology Centre, IBM Systems and Technology Lab, Bangalore, India
On Tue, 2011-11-29 at 12:16 -0500, Stefan Berger wrote:
On 11/29/2011 10:03 AM, Prerna Saxena wrote:
@@ -3048,6 +3087,7 @@ virDomainControllerDefParseXML(xmlNodePtr node, }
if (def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE && + def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && def->info.type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Controllers must use the 'pci' address type")); Is this still the proper error message for this type of bus? You may want to look at def->os.arch / def->os.machine to see what is expected here in terms of bus and then have a more specific error message.
No it's not the right message, which is why we're adding that check to skip the message if the controller has an spapr-vio address. So I don't think we need to anything else with arch/machine. cheers
On Tue, Nov 29, 2011 at 08:33:09PM +0530, Prerna Saxena wrote:
From: Michael Ellerman <michael@ellerman.id.au> Date: Tue, 29 Nov 2011 13:48:09 +0530 Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on the same bus.
For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices.
This patch adds a new address type "spapr-vio", which achieves this.
The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device.
In libvirt, we need to guarantee that addressing is stable across QEMU releases. So if the 'reg' property is not set in the XML, libvirt must auto-assign one itself before starting QEMU. You'll notice we have a qemuDomainAssignPCIAddresses() API. We will need to have some kind of equivalent for sparpr-vio "reg" addresses. Ideally libvirt will auto-assign using the same algorithm as QEMU would have, just for sake of sanity. 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 :|
On Wed, 2011-11-30 at 12:44 +0000, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 08:33:09PM +0530, Prerna Saxena wrote:
From: Michael Ellerman <michael@ellerman.id.au> Date: Tue, 29 Nov 2011 13:48:09 +0530 Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on the same bus.
For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices.
This patch adds a new address type "spapr-vio", which achieves this.
The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device.
In libvirt, we need to guarantee that addressing is stable across QEMU releases. So if the 'reg' property is not set in the XML, libvirt must auto-assign one itself before starting QEMU.
Must we really? IMHO if the user doesn't specify anything then that is essentially asking for "the default", whatever that is. My reasoning for putting the address assignment code in QEMU in the first place was that it has the most knowledge and so can make the best decision.
You'll notice we have a qemuDomainAssignPCIAddresses() API. We will need to have some kind of equivalent for sparpr-vio "reg" addresses. Ideally libvirt will auto-assign using the same algorithm as QEMU would have, just for sake of sanity.
Yeah I've seen it. I'd rather not do anything similar for spapr-vio, precisely because we will end up with identical duplicate code in libvirt & QEMU. But if it's a hard requirement in your view then I'll do it. cheers
On Thu, Dec 01, 2011 at 05:38:31PM +1100, Michael Ellerman wrote:
On Wed, 2011-11-30 at 12:44 +0000, Daniel P. Berrange wrote:
On Tue, Nov 29, 2011 at 08:33:09PM +0530, Prerna Saxena wrote:
From: Michael Ellerman <michael@ellerman.id.au> Date: Tue, 29 Nov 2011 13:48:09 +0530 Subject: [PATCH 5/5] Add address-type "spapr-vio" for devices based on the same bus.
For QEMU PPC64 we have a machine type ("pseries") which has a virtual bus called "spapr-vio". We need to be able to create devices on this bus, and as such need a way to specify the address for those devices.
This patch adds a new address type "spapr-vio", which achieves this.
The addressing is specified with a "reg" property in the address definition. The reg is optional, if it is not specified QEMU will auto-assign an address for the device.
In libvirt, we need to guarantee that addressing is stable across QEMU releases. So if the 'reg' property is not set in the XML, libvirt must auto-assign one itself before starting QEMU.
Must we really? IMHO if the user doesn't specify anything then that is essentially asking for "the default", whatever that is.
My reasoning for putting the address assignment code in QEMU in the first place was that it has the most knowledge and so can make the best decision.
You'll notice we have a qemuDomainAssignPCIAddresses() API. We will need to have some kind of equivalent for sparpr-vio "reg" addresses. Ideally libvirt will auto-assign using the same algorithm as QEMU would have, just for sake of sanity.
Yeah I've seen it. I'd rather not do anything similar for spapr-vio, precisely because we will end up with identical duplicate code in libvirt & QEMU.
But if it's a hard requirement in your view then I'll do it.
It is the only way to ensure stable device addresses every time the guest is started. eg, consider what happens if you don't assign addresses qemu -drive foo -drive bar QEMU assigns foo reg=1, bar reg=2. Now delete hotunplug a device (qemu) drive_del foo Now migrate QEMU to a new host, which means it'll be started with qemu -drive bar So QEMU will assign bar reg=1, whereas the guest OS currently thinks it has reg=2. libvirt *must* define addresses for every single device it passes to QEMU at all times. Even if we ignore hotplug, and just consider that we cold unpluged 'foo', we still want 'bar' to have the same address, in case the guest OS had configured something based on address, or worse yet done an OS license/activation check based on hardware config. 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 :|
On Thu, 2011-12-01 at 09:52 +0000, Daniel P. Berrange wrote:
On Thu, Dec 01, 2011 at 05:38:31PM +1100, Michael Ellerman wrote:
But if it's a hard requirement in your view then I'll do it.
It is the only way to ensure stable device addresses every time the guest is started.
eg, consider what happens if you don't assign addresses
qemu -drive foo -drive bar
QEMU assigns foo reg=1, bar reg=2. Now delete hotunplug a device
(qemu) drive_del foo
Now migrate QEMU to a new host, which means it'll be started with
qemu -drive bar
So QEMU will assign bar reg=1, whereas the guest OS currently thinks it has reg=2.
libvirt *must* define addresses for every single device it passes to QEMU at all times. Even if we ignore hotplug, and just consider that we cold unpluged 'foo', we still want 'bar' to have the same address, in case the guest OS had configured something based on address, or worse yet done an OS license/activation check based on hardware config.
OK. Currently I don't think either of those examples actually apply to us, but I can see that it is liable to bite us in the future if we don't assign addresses. Below is a first cut at a patch to do this. It seems to work but it has a few problems. Firstly there are calls in qemuDomainUpdateDeviceConfig() to qemuDomainAssignPCIAddresses(), I'm not sure if I need to also add calls to qemuAssignSpaprVIOAddresses() in there. Can you explain to me when qemuDomainUpdateDeviceConfig() is called and what it is doing? Secondly, and this is a real problem, if the user specifies a serial with no address type, we don't see it, because it doesn't have a spapr-vio address, and so we don't take it into account in the address allocation. But then because it ends becoming a spapr-vty in qemuBuildChrDeviceStr() (which Prerna added), it clashes with the other serial we created. Example config is: <serial type="pty"/> <serial type="pty"> <address type="spapr-vio"/> </serial> So we need some way of knowing that the first serial will end up on the spapr-vio bus, so we can take it into account in the address allocation. I haven't thought of a good way to fix that yet. We could just say that it's unsupported for pseries, ie. you MUST specify the address type, but that would be a pity. cheers diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index eceae35..dfd92ad 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -684,6 +684,63 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virBitmapPtr qemuCaps) return -1; } +static int qemuSpaprVIOFindByReg(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDeviceInfoPtr info, void *opaque) +{ + virDomainDeviceInfoPtr target = (virDomainDeviceInfoPtr)opaque; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return 0; + + /* Match a dev that has a reg, is not us, and has a matching reg */ + if (info->addr.spaprvio.has_reg && info != target && + info->addr.spaprvio.reg == target->addr.spaprvio.reg) + /* Has to be < 0 so virDomainDeviceInfoIterate() will exit */ + return -1; + + return 0; +} + +static void qemuAssignSpaprVIOAddress(virDomainDefPtr def, + virDomainDeviceInfoPtr info, + unsigned long long default_reg) +{ + int rc; + + if (info->type != VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO) + return; + + if (info->addr.spaprvio.has_reg) + return; + + info->addr.spaprvio.has_reg = true; + info->addr.spaprvio.reg = default_reg; + + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + while (rc != 0) { + /* We hit another device, move along by 4K and search again */ + info->addr.spaprvio.reg += 0x1000; + rc = virDomainDeviceInfoIterate(def, qemuSpaprVIOFindByReg, info); + } +} + +int qemuAssignSpaprVIOAddresses(virDomainDefPtr def) +{ + int i; + + for (i = 0 ; i < def->nnets; i++) + qemuAssignSpaprVIOAddress(def, &def->nets[i]->info, 0x1000ul); + + for (i = 0 ; i < def->ncontrollers; i++) + qemuAssignSpaprVIOAddress(def, &def->controllers[i]->info, 0x2000ul); + + for (i = 0 ; i < def->nserials; i++) + qemuAssignSpaprVIOAddress(def, &def->serials[i]->info, 0x30000000ul); + + /* No other devices are currently supported on spapr-vio */ + + return 0; +} #define QEMU_PCI_ADDRESS_LAST_SLOT 31 #define QEMU_PCI_ADDRESS_LAST_FUNCTION 8 diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 8b51ef3..f748c2b 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -203,6 +203,8 @@ int qemuAssignDeviceHostdevAlias(virDomainDefPtr def, virDomainHostdevDefPtr hos int qemuAssignDeviceControllerAlias(virDomainControllerDefPtr controller); int qemuAssignDeviceRedirdevAlias(virDomainDefPtr def, virDomainRedirdevDefPtr redirdev, int idx); +int qemuAssignSpaprVIOAddresses(virDomainDefPtr def); + int qemuParseKeywords(const char *str, char ***retkeywords, diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 105bdde..6c47516 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1317,6 +1317,8 @@ static virDomainPtr qemudDomainCreate(virConnectPtr conn, const char *xml, if (qemuDomainAssignPCIAddresses(def) < 0) goto cleanup; + qemuAssignSpaprVIOAddresses(def); + if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, def, false))) @@ -4903,6 +4905,8 @@ static virDomainPtr qemudDomainDefine(virConnectPtr conn, const char *xml) { if (qemuDomainAssignPCIAddresses(def) < 0) goto cleanup; + qemuAssignSpaprVIOAddresses(def); + if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, def, false))) { @@ -10748,6 +10752,8 @@ static virDomainPtr qemuDomainAttach(virConnectPtr conn, if (qemuDomainAssignPCIAddresses(def) < 0) goto cleanup; + qemuAssignSpaprVIOAddresses(def); + if (!(vm = virDomainAssignDef(driver->caps, &driver->domains, def, false))) -- 1.7.7.3
On 11/29/2011 09:55 AM, Prerna Saxena wrote:
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.
I applied this patch series on an x86 machine and ran 'make check'. I now have two tests failing due to it not making use of the proc filesystem entries anymore... TEST: nodeinfotest Expect 39 'CPUs: 2, MHz: 2800, Nodes: 1, Cores: 2 ' Actual 39 'CPUs: 8, MHz: 2800, Nodes: 1, Cores: 4 ' !Expect 39 'CPUs: 2, MHz: 2211, Nodes: 1, Cores: 2 ' Actual 39 'CPUs: 8, MHz: 2211, Nodes: 1, Cores: 4 ' !Expect 39 'CPUs: 4, MHz: 1595, Nodes: 1, Cores: 2 ' Actual 39 'CPUs: 8, MHz: 1595, Nodes: 1, Cores: 4 ' !Expect 39 'CPUs: 4, MHz: 1000, Nodes: 1, Cores: 4 ' Actual 39 'CPUs: 8, MHz: 1000, Nodes: 1, Cores: 4 ' !Expect 39 'CPUs: 4, MHz: 2814, Nodes: 1, Cores: 2 ' Actual 39 'CPUs: 8, MHz: 2814, Nodes: 1, Cores: 4 ' !Expect 39 'CPUs: 4, MHz: 1000, Nodes: 1, Cores: 2 ' Actual 39 'CPUs: 8, MHz: 1000, Nodes: 1, Cores: 4 ' ! 6 FAIL FAIL: nodeinfotest [...] TEST: qemuargv2xmltest /bin/sh: line 5: 875 Segmentation fault (core dumped) abs_top_builddir=`cd '..'; pwd` abs_top_srcdir=`cd '..'; pwd` abs_builddir=`pwd` abs_srcdir=`cd '.'; pwd` CONFIG_HEADER="`cd '..'; pwd`/config.h" PATH="`cd '..'; pwd`/daemon:`cd '..'; pwd`/tools:`cd '..'; pwd`/tests:$PATH" SHELL="/bin/sh" LIBVIRT_DRIVER_DIR="/root/tmp/libvirt-acl/src/.libs" LC_ALL=C ${dir}$tst FAIL: qemuargv2xmltest I had 'export DEBUG_TESTS=1' set to see the debugging info on the nodeinfo test. The qemuargv2xmltest fails due to the following: 0x00000038ec32e334 in __strcmp_ssse3 () from /lib64/libc.so.6 Missing separate debuginfos, use: debuginfo-install augeas-libs-0.9.0-1.fc14.x86_64 avahi-libs-0.6.27-8.fc14.x86_64 cyrus-sasl-lib-2.1.23-12.fc14.x86_64 dbus-libs-1.4.0-3.fc14.x86_64 device-mapper-libs-1.02.63-2.fc14.x86_64 glibc-2.13-2.x86_64 gnutls-2.8.6-2.fc14.x86_64 keyutils-libs-1.2-6.fc12.x86_64 krb5-libs-1.8.4-2.fc14.x86_64 libcap-ng-0.6.5-1.fc14.x86_64 libcom_err-1.41.12-6.fc14.x86_64 libcurl-7.21.0-10.fc14.x86_64 libgcc-4.5.1-4.fc14.x86_64 libgcrypt-1.4.5-4.fc13.x86_64 libgpg-error-1.9-1.fc14.x86_64 libidn-1.18-1.fc14.x86_64 libpcap-1.1.1-3.fc14.x86_64 libselinux-2.0.96-6.fc14.1.x86_64 libsepol-2.0.41-3.fc14.x86_64 libssh2-1.2.4-1.fc14.x86_64 libtasn1-2.7-1.fc14.x86_64 libudev-161-10.fc14.x86_64 libxml2-2.7.7-3.fc14.x86_64 libxslt-1.1.26-3.fc14.x86_64 netcf-libs-0.1.9-1.fc14.x86_64 nspr-4.8.8-1.fc14.x86_64 nss-3.12.10-4.fc14.x86_64 nss-softokn-freebl-3.12.10-1.fc14.x86_64 nss-util-3.12.10-1.fc14.x86_64 numactl-2.0.3-8.fc13.x86_64 openldap-2.4.23-10.fc14.x86_64 openssl-1.0.0e-1.fc14.x86_64 xen-libs-4.0.2-1.fc14.x86_64 yajl-1.0.7-3.fc13.x86_64 zlib-1.2.5-2.fc14.x86_64 (gdb) up #1 0x000000000042d56c in qemuParseCommandLine (caps=0x9b94a0, progenv=0x9badc0, progargv=0x9bae00, pidfile=0x0, monConfig=0x0, monJSON=0x0) at qemu/qemu_command.c:6694 6694 if (STREQ(def->os.arch, "i686")||STREQ(def->os.arch, "x86_64")) (gdb) print def->os $1 = {type = 0x0, arch = 0x0, machine = 0x0, nBootDevs = 0, bootDevs = {0, 0, 0, 0}, bootmenu = 0, init = 0x0, kernel = 0x0, initrd = 0x0, cmdline = 0x0, root = 0x0, loader = 0x0, bootloader = 0x0, bootloaderArgs = 0x0, smbios_mode = 0, bios = {useserial = 0}} I looked on a RHEL 5 machine (i686). The sysfs at least is there. Stefan
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, libvirt must be capable of choosing supported device backends and defaults for each architecture in qemu.
To check if qemu supports a certain feature, libvirt at present parses the -help string which is generated by running the qemu binary with the '-h' argument. This approach is gated by QEMU's inherent limitation. When generating the list of allowed options with the '-h' flags, qemu today blindly lists all options defined for any architecture/platform instead of doing any arch-specific checking. This tricks libvirt into assuming a much bigger set of host capabilities than is actually available. Ideally, it would be good to have qemu specify a list of devices for a given architecture and platform which libvirt can parse to understand supported capabilities for that guest.
As a part of this patchset, there is an attempt to cleanly bifurcate libvirt code and to remove x86-specific assumptions from generic qemu commandline code.
Series Description: ------------------- This patch series consists of 5 patches : Patch 1/5 : Use sysfs to gather host topology in place of /proc/cpuinfo. Patch 2/5 : Add PowerPC CPU Driver Patch 3/5 : Add support for qemu-system-ppc64 Patch 4/5 : Clean up x86-specific assumptions from generic qemu code. Patch 5/5 : Add address family "spapr-vio"
Changelog: --------- ** v1->v2 : * Patches 1,2,3 unchanged ; The hacks in Patch 4 of v1 replaced by a new patch to neatly select arch-specific features.
** v2->v3 : * Patches 1,2,3 have minor cleanups ; Patch 4 no longer has an arch-specific handler routine. It is now replaced by a much simpler patch that merely removes x86/pc-specific assumptions from libvirt.
** v3->v4 : * Patches 1,2,3,4 unchanged ; patch 5 is a new addition from Michael Ellerman that adds a new device-tree based addressing mechanism for the 'pseries' guest.
participants (5)
-
Daniel P. Berrange -
Eric Blake -
Michael Ellerman -
Prerna Saxena -
Stefan Berger