On Thu, Mar 14, 2013 at 02:54:20PM +0800, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > Currently, -machine option is used only when dump-guest-core is used. > > To use options defined in machine option for new version of QEMU, > it needs to use -machine xxx, and to be compatible with older version > -M, this patch addes QEMU_CAPS_MACH_OPT capability, and assumes > -machine is used for QEMU v1.0 onwards. > > To avoid the collision for creating USB controllers when using USB > option and -device xxxx, it needs to set usb=off in machine option. > QEMU_CAPS_USB_OPT capability is added, and it will be for QEMU > v1.3.0-rc0 onwards which supports USB option. I'd rather see this patch split into two parts. One part to introduce the use of -machine, and a second patch to deal with the usb=off part.

That said I'm not sure why we need to set usb=off at all - we already run qemu with -nodefconfig -nodefaults which should be blocking the default USB controller.

> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > --- > src/qemu/qemu_capabilities.c | 10 ++++++++++ > src/qemu/qemu_capabilities.h | 2 ++ > src/qemu/qemu_command.c | 36 +++++++++++++++++++++++++----------- > tests/qemuxml2argvtest.c | 4 ++-- > 4 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c > index 51fc9dc..79eb83f 100644 > --- a/src/qemu/qemu_capabilities.c > +++ b/src/qemu/qemu_capabilities.c > @@ -205,6 +205,8 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, > "usb-serial", /* 125 */ > "usb-net", > "add-fd", > + "mach-opt", > + "usb-opt", > > ); > > @@ -2416,6 +2418,14 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, > > virQEMUCapsInitQMPBasic(qemuCaps); > > + /* Assuming to use machine option v1.0 onwards*/ > + if (qemuCaps->version >= 1000000) > + virQEMUCapsSet(qemuCaps, QEMU_CAPS_MACH_OPT); > + > + /* USB option is supported v1.3.0-rc0 onwards */ > + if (qemuCaps->version >= 1002090) > + virQEMUCapsSet(qemuCaps, QEMU_CAPS_USB_OPT); > + > if (!(archstr = qemuMonitorGetTargetArch(mon))) > goto cleanup; If we get to this aprt of virQEMUCapsInitQMP() code path, then we already know we have a QEMU >= 1.2.0, so this first version check is not required. Just set the QEMU_CAPS_MACH_OPT in virQEMUCapsInitQMPBasic.

For the USB property just use version 1.3.0 - don't try to do stuff based on development snapshot version numbers.

> diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h > index e69d558..06aaa68 100644 > --- a/src/qemu/qemu_capabilities.h > +++ b/src/qemu/qemu_capabilities.h > @@ -166,6 +166,8 @@ enum virQEMUCapsFlags { > QEMU_CAPS_DEVICE_USB_SERIAL = 125, /* -device usb-serial */ > QEMU_CAPS_DEVICE_USB_NET = 126, /* -device usb-net */ > QEMU_CAPS_ADD_FD = 127, /* -add-fd */ > + QEMU_CAPS_MACH_OPT = 128, /* -machine xxxx*/ We don't need to abbreviate - use QEMU_CAPS_MACHINE_OPT

> + QEMU_CAPS_USB_OPT = 129, /* -machine xxxx,usb=off*/ I'd prefer this second one to be QEMU_CAPS_MACHINE_USB_OPT

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index 6c28123..e7dde21 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -4614,6 +4614,8 @@ qemuBuildMachineArgStr(virCommandPtr cmd, > const virDomainDefPtr def, > virQEMUCapsPtr qemuCaps) > { > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + > /* This should *never* be NULL, since we always provide > * a machine in the capabilities data for QEMU. So this > * check is just here as a safety in case the unexpected > @@ -4621,27 +4623,39 @@ qemuBuildMachineArgStr(virCommandPtr cmd, > if (!def->os.machine) > return 0; > > - if (!def->mem.dump_core) { > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_MACH_OPT)) { > /* if no parameter to the machine type is needed, we still use > * '-M' to keep the most of the compatibility with older versions. > */ > virCommandAddArgList(cmd, "-M", def->os.machine, NULL); > } else { > - if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) { > - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > - "%s", _("dump-guest-core is not available " > - " with this QEMU binary")); > - return -1; > - } > > /* However, in case there is a parameter to be added, we need to > * use the "-machine" parameter because qemu is not parsing the > * "-M" correctly */ > + > virCommandAddArg(cmd, "-machine"); > - virCommandAddArgFormat(cmd, > - "%s,dump-guest-core=%s", > - def->os.machine, > - virDomainMemDumpTypeToString(def->mem.dump_core)); > + virBufferAsprintf(&buf, "%s", def->os.machine); > + > + /* To avoid the collision of creating USB controllers when calling > + * machine->init in QEMU, it needs to set usb=off > + */ > + if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_USB_OPT)) > + virBufferAsprintf(&buf, ",usb=off"); > + > + if (def->mem.dump_core) { > + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DUMP_GUEST_CORE)) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + "%s", _("dump-guest-core is not available " > + " with this QEMU binary")); > + return -1; > + } > + > + virBufferAsprintf(&buf, ",dump-guest-core=%s", > + virDomainMemDumpTypeToString(def->mem.dump_core)); > + } > + > + virCommandAddArg(cmd, virBufferContentAndReset(&buf)); > } > Daniel

From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; This patch adds a CPU feature "powernv" identifying IBM Power processor that supports native hypervisor e.g. KVM. This can be used by virtualization management software to determine the CPU capabilities. PVR doesn't indicate whether it a host or a guest CPU. So, we use /proc/cpuinfo to get the platform information (PowerNV) and use that to set the "powernv" flag. Signed-off-by: Dipankar Sarma <dipankar(a)in.ibm.com&gt; Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; --- src/cpu/cpu_map.xml | 9 ++ src/cpu/cpu_powerpc.c | 349 ++++++++++++++++++++++++++++++++++++++---------- src/cpu/cpu_ppc_data.h | 4 + src/util/sysinfo.c | 2 +- 4 files changed, 294 insertions(+), 70 deletions(-) diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml index eb69a34..6b1f9b9 100644 --- a/src/cpu/cpu_map.xml +++ b/src/cpu/cpu_map.xml @@ -587,14 +587,23 @@ <arch name='ppc64'> <!-- vendor definitions --> <vendor name='IBM' string='PowerPC'/> + <feature name='powernv'> <!-- SYSTEMID_POWERNV --> + <systemid platform='0x00000001'/> + </feature> <!-- IBM-based CPU models --> <model name='POWER7'> + <feature name='powernv'/> + <systemid pvr='0x003f0200'/> <vendor name='IBM'/> </model> <model name='POWER7_v2.1'> + <feature name='powernv'/> + <systemid pvr='0x003f0201'/> <vendor name='IBM'/> </model> <model name='POWER7_v2.3'> + <feature name='powernv'/> + <systemid pvr='0x003f0203'/> <vendor name='IBM'/> </model> </arch> diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c index ac10789..2135944 100644 --- a/src/cpu/cpu_powerpc.c +++ b/src/cpu/cpu_powerpc.c @@ -33,6 +33,7 @@ #include "cpu_map.h" #include "buf.h" +#include "sysinfo.h" #define VIR_FROM_THIS VIR_FROM_CPU @@ -44,16 +45,9 @@ struct cpuPowerPC { uint32_t pvr; }; -static const struct cpuPowerPC cpu_defs[] = { - {"POWER7", "IBM", 0x003f0200}, - {"POWER7_v2.1", "IBM", 0x003f0201}, - {"POWER7_v2.3", "IBM", 0x003f0203}, - {NULL, NULL, 0xffffffff} -}; - - struct ppc_vendor { char *name; + uint32_t pvr; struct ppc_vendor *next; }; @@ -64,64 +58,191 @@ struct ppc_model { struct ppc_model *next; }; +struct ppc_feature { + char *name; + union cpuData *data; + struct ppc_feature *next; +}; + struct ppc_map { struct ppc_vendor *vendors; + struct ppc_feature *features; struct ppc_model *models; }; +static void +ppcDataSubtract(union cpuData *data1, + const union cpuData *data2) +{ + data1->ppc.platform &= ~data2->ppc.platform; +} + +static bool +ppcDataIsSubset(const union cpuData *data, + const union cpuData *subset) +{ + if (subset->ppc.platform && + (data->ppc.platform & subset->ppc.platform) == subset->ppc.platform) + return true; + + return false; +} + +static void +PowerPCDataFree(union cpuData *data) +{ + if (data == NULL) + return; + VIR_FREE(data); +} + + +static union cpuData * +ppcDataCopy(const union cpuData *data) +{ + union cpuData *copy = NULL; + + if (VIR_ALLOC(copy) < 0) { + PowerPCDataFree(copy); + return NULL; + } + copy->ppc = data->ppc; + return copy; +} + + +/* also removes all detected features from data */ static int -ConvertModelVendorFromPVR(char ***model, - char ***vendor, - uint32_t pvr) +ppcDataToCPUFeatures(virCPUDefPtr cpu, + int policy, + union cpuData *data, + const struct ppc_map *map) { - int i; + const struct ppc_feature *feature = map->features; - for (i = 0; cpu_defs[i].name; i++) { - if (cpu_defs[i].pvr == pvr) { - **model = strdup(cpu_defs[i].name); - **vendor = strdup(cpu_defs[i].vendor); - return 0; + while (feature != NULL) { + if (ppcDataIsSubset(data, feature->data)) { + ppcDataSubtract(data, feature->data); + if (virCPUDefAddFeature(cpu, feature->name, policy) < 0) + return -1; } + feature = feature->next; } - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing the definition of this model")); - return -1; + return 0; } -static int -ConvertPVRFromModel(const char *model, - uint32_t *pvr) +static struct ppc_feature * +ppcFeatureNew(void) { - int i; + struct ppc_feature *feature; - for (i = 0; cpu_defs[i].name; i++) { - if (STREQ(cpu_defs[i].name, model)) { - *pvr = cpu_defs[i].pvr; - return 0; - } + if (VIR_ALLOC(feature) < 0) + return NULL; + + if (VIR_ALLOC(feature->data) < 0) { + VIR_FREE(feature); + return NULL; + } + + return feature; +} + + +static void +ppcFeatureFree(struct ppc_feature *feature) +{ + if (feature == NULL) + return; + + VIR_FREE(feature->name); + PowerPCDataFree(feature->data); + VIR_FREE(feature); +} + + +static struct ppc_feature * +ppcFeatureFind(const struct ppc_map *map, + const char *name) +{ + struct ppc_feature *feature; + + feature = map->features; + while (feature != NULL) { + if (STREQ(feature->name, name)) + return feature; + + feature = feature->next; } - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Missing the definition of this model")); - return -1; + return NULL; } static int -cpuMatch(const union cpuData *data, - char **cpu_model, - char **cpu_vendor, - const struct ppc_model *model) +ppcFeatureLoad(xmlXPathContextPtr ctxt, + struct ppc_map *map) { + xmlNodePtr *nodes = NULL; + xmlNodePtr ctxt_node = ctxt->node; + struct ppc_feature *feature; int ret = 0; + unsigned long platform = 0; + unsigned long pvr = 0; + int ret_platform; + int ret_pvr; + int n; + + if (!(feature = ppcFeatureNew())) + goto no_memory; + + feature->name = virXPathString("string(@name)", ctxt); + if (feature->name == NULL) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Missing CPU feature name")); + goto ignore; + } + + if (ppcFeatureFind(map, feature->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("CPU feature %s already defined"), feature->name); + goto ignore; + } + + n = virXPathNodeSet("./systemid", ctxt, &nodes); + if (n < 0) + goto ignore; + + ctxt->node = nodes[0]; + ret_platform = virXPathULongHex("string(@platform)", ctxt, &platform); + ret_pvr = virXPathULongHex("string(@pvr)", ctxt, &pvr); + if (ret_platform < 0 && ret_pvr < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid systemid in %s feature"), feature->name); + goto ignore; + } + feature->data->ppc.platform = platform; + feature->data->ppc.pvr = pvr; - ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr); + if (map->features == NULL) + map->features = feature; + else { + feature->next = map->features; + map->features = feature; + } - if (STREQ(model->name, *cpu_model) && - STREQ(model->vendor->name, *cpu_vendor)) - ret = 1; +out: + ctxt->node = ctxt_node; + VIR_FREE(nodes); return ret; + +no_memory: + virReportOOMError(); + ret = -1; + +ignore: + ppcFeatureFree(feature); + goto out; } @@ -171,6 +292,66 @@ ppcModelFind(const struct ppc_map *map, return NULL; } +/* also removes bits corresponding to vendor string from data */ +static const struct ppc_vendor * +ppcDataToVendor(union cpuData *data, + const struct ppc_map *map) +{ + const struct ppc_vendor *vendor = map->vendors; + + while (vendor) { + if (data->ppc.pvr == vendor->pvr) + return vendor; + vendor = vendor->next; + } + + return NULL; +} + + +static virCPUDefPtr +ppcDataToCPU(const union cpuData *data, + const struct ppc_model *model, + const struct ppc_map *map) +{ + virCPUDefPtr cpu; + union cpuData *copy = NULL; + union cpuData *modelData = NULL; + const struct ppc_vendor *vendor; + + if (VIR_ALLOC(cpu) < 0 || + !(cpu->model = strdup(model->name)) || + !(copy = ppcDataCopy(data)) || + !(modelData = ppcDataCopy(model->data))) + goto no_memory; + + if ((vendor = ppcDataToVendor(copy, map)) && + !(cpu->vendor = strdup(vendor->name))) + goto no_memory; + + ppcDataSubtract(copy, modelData); + ppcDataSubtract(modelData, data); + + /* because feature policy is ignored for host CPU */ + cpu->type = VIR_CPU_TYPE_GUEST; + + if (ppcDataToCPUFeatures(cpu, VIR_CPU_FEATURE_REQUIRE, copy, map) || + ppcDataToCPUFeatures(cpu, VIR_CPU_FEATURE_DISABLE, modelData, map)) + goto error; + +cleanup: + PowerPCDataFree(modelData); + PowerPCDataFree(copy); + return cpu; + +no_memory: + virReportOOMError(); +error: + virCPUDefFree(cpu); + cpu = NULL; + goto cleanup; +} + static struct ppc_vendor * ppcVendorFind(const struct ppc_map *map, const char *name) @@ -256,6 +437,9 @@ ppcModelLoad(xmlXPathContextPtr ctxt, xmlNodePtr *nodes = NULL; struct ppc_model *model; char *vendor = NULL; + unsigned long pvr = 0, platform = 0; + int ret_platform, ret_pvr; + int n; int ret = -1; if (!(model = ppcModelNew())) @@ -268,10 +452,20 @@ ppcModelLoad(xmlXPathContextPtr ctxt, goto ignore; } - ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr); - if (ret < 0) - goto ignore; + n = virXPathNodeSet("./systemid", ctxt, &nodes); + if (n < 0) + goto ignore; + ctxt->node = nodes[0]; + ret_platform = virXPathULongHex("string(@platform)", ctxt, &platform); + ret_pvr = virXPathULongHex("string(@pvr)", ctxt, &pvr); + if (ret_platform < 0 && ret_pvr < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Invalid systemid in %s model"), model->name); + goto ignore; + } + model->data->ppc.platform = platform; + model->data->ppc.pvr = pvr; if (virXPathBoolean("boolean(./vendor)", ctxt)) { vendor = virXPathString("string(./vendor/@name)", ctxt); @@ -324,6 +518,8 @@ ppcMapLoadCallback(enum cpuMapElement element, return ppcVendorLoad(ctxt, map); case CPU_MAP_ELEMENT_MODEL: return ppcModelLoad(ctxt, map); + case CPU_MAP_ELEMENT_FEATURE: + return ppcFeatureLoad(ctxt, map); default: break; } @@ -385,6 +581,7 @@ ppcModelCopy(const struct ppc_model *model) } copy->data->ppc.pvr = model->data->ppc.pvr; + copy->data->ppc.platform = model->data->ppc.platform; copy->vendor = model->vendor; return copy; @@ -437,8 +634,6 @@ PowerPCDecode(virCPUDefPtr cpu, const struct ppc_model *candidate; virCPUDefPtr cpuCandidate; virCPUDefPtr cpuModel = NULL; - char *cpu_vendor = NULL; - char *cpu_model = NULL; unsigned int i; if (data == NULL || (map = ppcLoadMap()) == NULL) @@ -475,13 +670,30 @@ PowerPCDecode(virCPUDefPtr cpu, goto next; } - if (VIR_ALLOC(cpuCandidate) < 0) { - virReportOOMError(); + if (!(cpuCandidate = ppcDataToCPU(data, candidate, map))) goto out; + + if (candidate->vendor && cpuCandidate->vendor && + STRNEQ(candidate->vendor->name, cpuCandidate->vendor)) { + VIR_DEBUG("CPU vendor %s of model %s differs from %s; ignoring", + candidate->vendor->name, candidate->name, + cpuCandidate->vendor); + virCPUDefFree(cpuCandidate); + goto next; } - cpuCandidate->model = strdup(candidate->name); - cpuCandidate->vendor = strdup(candidate->vendor->name); + if (cpu->type == VIR_CPU_TYPE_HOST) { + cpuCandidate->type = VIR_CPU_TYPE_HOST; + for (i = 0; i < cpuCandidate->nfeatures; i++) { + switch (cpuCandidate->features[i].policy) { + case VIR_CPU_FEATURE_DISABLE: + virCPUDefFree(cpuCandidate); + goto next; + default: + cpuCandidate->features[i].policy = -1; + } + } + } if (preferred && STREQ(cpuCandidate->model, preferred)) { virCPUDefFree(cpuModel); @@ -489,19 +701,12 @@ PowerPCDecode(virCPUDefPtr cpu, break; } - ret = cpuMatch(data, &cpu_model, &cpu_vendor, candidate); - if (ret < 0) { - VIR_FREE(cpuCandidate); - goto out; - } else if (ret == 1) { - cpuCandidate->model = cpu_model; - cpuCandidate->vendor = cpu_vendor; + if (cpuModel == NULL + || cpuModel->nfeatures > cpuCandidate->nfeatures) { virCPUDefFree(cpuModel); cpuModel = cpuCandidate; - break; - } - - virCPUDefFree(cpuCandidate); + }else + virCPUDefFree(cpuCandidate); next: candidate = candidate->next; @@ -515,6 +720,8 @@ PowerPCDecode(virCPUDefPtr cpu, cpu->model = cpuModel->model; cpu->vendor = cpuModel->vendor; + cpu->nfeatures = cpuModel->nfeatures; + cpu->features = cpuModel->features; VIR_FREE(cpuModel); ret = 0; @@ -537,19 +744,11 @@ static uint32_t ppc_mfpvr(void) } #endif -static void -PowerPCDataFree(union cpuData *data) -{ - if (data == NULL) - return; - - VIR_FREE(data); -} - static union cpuData * PowerPCNodeData(void) { union cpuData *data; + virSysinfoDefPtr hostinfo; if (VIR_ALLOC(data) < 0) { virReportOOMError(); @@ -561,6 +760,18 @@ PowerPCNodeData(void) data->ppc.pvr = ppc_mfpvr(); #endif + hostinfo = virSysinfoRead(); + if (hostinfo == NULL) { + VIR_FREE(data); + return NULL; + } + + data->ppc.platform &= ~VIR_CPU_PPC64_POWERNV; + + if (STREQ(hostinfo->system_family, "PowerNV")) + data->ppc.platform |= VIR_CPU_PPC64_POWERNV; + virSysinfoDefFree(hostinfo); + return data; } diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h index 685332a..0e691ce 100644 --- a/src/cpu/cpu_ppc_data.h +++ b/src/cpu/cpu_ppc_data.h @@ -28,6 +28,10 @@ struct cpuPPCData { uint32_t pvr; + uint32_t platform; /* Bitflag indicating platform features */ }; +#define VIR_CPU_PPC64_NONE 0x0 +#define VIR_CPU_PPC64_POWERNV 0x1 + #endif /* __VIR_CPU_PPC_DATA_H__ */ diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c index 6a5db80..5f98986 100644 --- a/src/util/sysinfo.c +++ b/src/util/sysinfo.c @@ -234,7 +234,7 @@ virSysinfoRead(void) { if (VIR_ALLOC(ret) < 0) goto no_memory; - if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) { + if (virFileReadAll(CPUINFO, 8192, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); return NULL;

On Thu, Mar 14, 2013 at 02:54:21PM +0800, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > This patch adds a CPU feature "powernv" identifying IBM Power > processor that supports native hypervisor e.g. KVM. This can > be used by virtualization management software to determine > the CPU capabilities. PVR doesn't indicate whether it a > host or a guest CPU. So, we use /proc/cpuinfo to get the > platform information (PowerNV) and use that to set the > "powernv" flag. > > Signed-off-by: Dipankar Sarma <dipankar(a)in.ibm.com&gt; > Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > --- > src/cpu/cpu_map.xml | 9 ++ > src/cpu/cpu_powerpc.c | 349 ++++++++++++++++++++++++++++++++++++++---------- > src/cpu/cpu_ppc_data.h | 4 + > src/util/sysinfo.c | 2 +- > 4 files changed, 294 insertions(+), 70 deletions(-) > > diff --git a/src/cpu/cpu_map.xml b/src/cpu/cpu_map.xml > index eb69a34..6b1f9b9 100644 > --- a/src/cpu/cpu_map.xml > +++ b/src/cpu/cpu_map.xml > @@ -587,14 +587,23 @@ > <arch name='ppc64'> > <!-- vendor definitions --> > <vendor name='IBM' string='PowerPC'/> > + <feature name='powernv'> <!-- SYSTEMID_POWERNV --> > + <systemid platform='0x00000001'/> > + </feature> > <!-- IBM-based CPU models --> > <model name='POWER7'> > + <feature name='powernv'/> > + <systemid pvr='0x003f0200'/> > <vendor name='IBM'/> > </model> > <model name='POWER7_v2.1'> > + <feature name='powernv'/> > + <systemid pvr='0x003f0201'/> > <vendor name='IBM'/> > </model> > <model name='POWER7_v2.3'> > + <feature name='powernv'/> > + <systemid pvr='0x003f0203'/> > <vendor name='IBM'/> > </model> > </arch> > diff --git a/src/cpu/cpu_powerpc.c b/src/cpu/cpu_powerpc.c > index ac10789..2135944 100644 > --- a/src/cpu/cpu_powerpc.c > +++ b/src/cpu/cpu_powerpc.c > @@ -33,6 +33,7 @@ > > #include "cpu_map.h" > #include "buf.h" > +#include "sysinfo.h" > > #define VIR_FROM_THIS VIR_FROM_CPU > > @@ -44,16 +45,9 @@ struct cpuPowerPC { > uint32_t pvr; > }; > > -static const struct cpuPowerPC cpu_defs[] = { > - {"POWER7", "IBM", 0x003f0200}, > - {"POWER7_v2.1", "IBM", 0x003f0201}, > - {"POWER7_v2.3", "IBM", 0x003f0203}, > - {NULL, NULL, 0xffffffff} > -}; > - > - > struct ppc_vendor { > char *name; > + uint32_t pvr; > struct ppc_vendor *next; > }; > > @@ -64,64 +58,191 @@ struct ppc_model { > struct ppc_model *next; > }; > > +struct ppc_feature { > + char *name; > + union cpuData *data; > + struct ppc_feature *next; > +}; > + > struct ppc_map { > struct ppc_vendor *vendors; > + struct ppc_feature *features; > struct ppc_model *models; > }; > > +static void > +ppcDataSubtract(union cpuData *data1, > + const union cpuData *data2) > +{ > + data1->ppc.platform &= ~data2->ppc.platform; > +} > + > +static bool > +ppcDataIsSubset(const union cpuData *data, > + const union cpuData *subset) > +{ > + if (subset->ppc.platform && > + (data->ppc.platform & subset->ppc.platform) == subset->ppc.platform) > + return true; > + > + return false; > +} > + > +static void > +PowerPCDataFree(union cpuData *data) > +{ > + if (data == NULL) > + return; > + VIR_FREE(data); > +} > + > + > +static union cpuData * > +ppcDataCopy(const union cpuData *data) > +{ > + union cpuData *copy = NULL; > + > + if (VIR_ALLOC(copy) < 0) { > + PowerPCDataFree(copy); > + return NULL; > + } > + copy->ppc = data->ppc; > + return copy; > +} > + > + > +/* also removes all detected features from data */ > static int > -ConvertModelVendorFromPVR(char ***model, > - char ***vendor, > - uint32_t pvr) > +ppcDataToCPUFeatures(virCPUDefPtr cpu, > + int policy, > + union cpuData *data, > + const struct ppc_map *map) > { > - int i; > + const struct ppc_feature *feature = map->features; > > - for (i = 0; cpu_defs[i].name; i++) { > - if (cpu_defs[i].pvr == pvr) { > - **model = strdup(cpu_defs[i].name); > - **vendor = strdup(cpu_defs[i].vendor); > - return 0; > + while (feature != NULL) { > + if (ppcDataIsSubset(data, feature->data)) { > + ppcDataSubtract(data, feature->data); > + if (virCPUDefAddFeature(cpu, feature->name, policy) < 0) > + return -1; > } > + feature = feature->next; > } > > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("Missing the definition of this model")); > - return -1; > + return 0; > } > > -static int > -ConvertPVRFromModel(const char *model, > - uint32_t *pvr) > +static struct ppc_feature * > +ppcFeatureNew(void) > { > - int i; > + struct ppc_feature *feature; > > - for (i = 0; cpu_defs[i].name; i++) { > - if (STREQ(cpu_defs[i].name, model)) { > - *pvr = cpu_defs[i].pvr; > - return 0; > - } > + if (VIR_ALLOC(feature) < 0) > + return NULL; > + > + if (VIR_ALLOC(feature->data) < 0) { > + VIR_FREE(feature); > + return NULL; > + } > + > + return feature; > +} > + > + > +static void > +ppcFeatureFree(struct ppc_feature *feature) > +{ > + if (feature == NULL) > + return; > + > + VIR_FREE(feature->name); > + PowerPCDataFree(feature->data); > + VIR_FREE(feature); > +} > + > + > +static struct ppc_feature * > +ppcFeatureFind(const struct ppc_map *map, > + const char *name) > +{ > + struct ppc_feature *feature; > + > + feature = map->features; > + while (feature != NULL) { > + if (STREQ(feature->name, name)) > + return feature; > + > + feature = feature->next; > } > > - virReportError(VIR_ERR_INTERNAL_ERROR, > - "%s", _("Missing the definition of this model")); > - return -1; > + return NULL; > } > > static int > -cpuMatch(const union cpuData *data, > - char **cpu_model, > - char **cpu_vendor, > - const struct ppc_model *model) > +ppcFeatureLoad(xmlXPathContextPtr ctxt, > + struct ppc_map *map) > { > + xmlNodePtr *nodes = NULL; > + xmlNodePtr ctxt_node = ctxt->node; > + struct ppc_feature *feature; > int ret = 0; > + unsigned long platform = 0; > + unsigned long pvr = 0; > + int ret_platform; > + int ret_pvr; > + int n; > + > + if (!(feature = ppcFeatureNew())) > + goto no_memory; > + > + feature->name = virXPathString("string(@name)", ctxt); > + if (feature->name == NULL) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Missing CPU feature name")); > + goto ignore; > + } > + > + if (ppcFeatureFind(map, feature->name)) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("CPU feature %s already defined"), feature->name); > + goto ignore; > + } > + > + n = virXPathNodeSet("./systemid", ctxt, &nodes); > + if (n < 0) > + goto ignore; > + > + ctxt->node = nodes[0]; > + ret_platform = virXPathULongHex("string(@platform)", ctxt, &platform); > + ret_pvr = virXPathULongHex("string(@pvr)", ctxt, &pvr); > + if (ret_platform < 0 && ret_pvr < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Invalid systemid in %s feature"), feature->name); > + goto ignore; > + } > + feature->data->ppc.platform = platform; > + feature->data->ppc.pvr = pvr; > > - ret = ConvertModelVendorFromPVR(&cpu_model, &cpu_vendor, data->ppc.pvr); > + if (map->features == NULL) > + map->features = feature; > + else { > + feature->next = map->features; > + map->features = feature; > + } > > - if (STREQ(model->name, *cpu_model) && > - STREQ(model->vendor->name, *cpu_vendor)) > - ret = 1; > +out: > + ctxt->node = ctxt_node; > + VIR_FREE(nodes); > > return ret; > + > +no_memory: > + virReportOOMError(); > + ret = -1; > + > +ignore: > + ppcFeatureFree(feature); > + goto out; > } > > > @@ -171,6 +292,66 @@ ppcModelFind(const struct ppc_map *map, > return NULL; > } > > +/* also removes bits corresponding to vendor string from data */ > +static const struct ppc_vendor * > +ppcDataToVendor(union cpuData *data, > + const struct ppc_map *map) > +{ > + const struct ppc_vendor *vendor = map->vendors; > + > + while (vendor) { > + if (data->ppc.pvr == vendor->pvr) > + return vendor; > + vendor = vendor->next; > + } > + > + return NULL; > +} > + > + > +static virCPUDefPtr > +ppcDataToCPU(const union cpuData *data, > + const struct ppc_model *model, > + const struct ppc_map *map) > +{ > + virCPUDefPtr cpu; > + union cpuData *copy = NULL; > + union cpuData *modelData = NULL; > + const struct ppc_vendor *vendor; > + > + if (VIR_ALLOC(cpu) < 0 || > + !(cpu->model = strdup(model->name)) || > + !(copy = ppcDataCopy(data)) || > + !(modelData = ppcDataCopy(model->data))) > + goto no_memory; > + > + if ((vendor = ppcDataToVendor(copy, map)) && > + !(cpu->vendor = strdup(vendor->name))) > + goto no_memory; > + > + ppcDataSubtract(copy, modelData); > + ppcDataSubtract(modelData, data); > + > + /* because feature policy is ignored for host CPU */ > + cpu->type = VIR_CPU_TYPE_GUEST; > + > + if (ppcDataToCPUFeatures(cpu, VIR_CPU_FEATURE_REQUIRE, copy, map) || > + ppcDataToCPUFeatures(cpu, VIR_CPU_FEATURE_DISABLE, modelData, map)) > + goto error; > + > +cleanup: > + PowerPCDataFree(modelData); > + PowerPCDataFree(copy); > + return cpu; > + > +no_memory: > + virReportOOMError(); > +error: > + virCPUDefFree(cpu); > + cpu = NULL; > + goto cleanup; > +} > + > static struct ppc_vendor * > ppcVendorFind(const struct ppc_map *map, > const char *name) > @@ -256,6 +437,9 @@ ppcModelLoad(xmlXPathContextPtr ctxt, > xmlNodePtr *nodes = NULL; > struct ppc_model *model; > char *vendor = NULL; > + unsigned long pvr = 0, platform = 0; > + int ret_platform, ret_pvr; > + int n; > int ret = -1; > > if (!(model = ppcModelNew())) > @@ -268,10 +452,20 @@ ppcModelLoad(xmlXPathContextPtr ctxt, > goto ignore; > } > > - ret = ConvertPVRFromModel(model->name, &model->data->ppc.pvr); > - if (ret < 0) > - goto ignore; > + n = virXPathNodeSet("./systemid", ctxt, &nodes); > + if (n < 0) > + goto ignore; > > + ctxt->node = nodes[0]; > + ret_platform = virXPathULongHex("string(@platform)", ctxt, &platform); > + ret_pvr = virXPathULongHex("string(@pvr)", ctxt, &pvr); > + if (ret_platform < 0 && ret_pvr < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Invalid systemid in %s model"), model->name); > + goto ignore; > + } > + model->data->ppc.platform = platform; > + model->data->ppc.pvr = pvr; > > if (virXPathBoolean("boolean(./vendor)", ctxt)) { > vendor = virXPathString("string(./vendor/@name)", ctxt); > @@ -324,6 +518,8 @@ ppcMapLoadCallback(enum cpuMapElement element, > return ppcVendorLoad(ctxt, map); > case CPU_MAP_ELEMENT_MODEL: > return ppcModelLoad(ctxt, map); > + case CPU_MAP_ELEMENT_FEATURE: > + return ppcFeatureLoad(ctxt, map); > default: > break; > } > @@ -385,6 +581,7 @@ ppcModelCopy(const struct ppc_model *model) > } > > copy->data->ppc.pvr = model->data->ppc.pvr; > + copy->data->ppc.platform = model->data->ppc.platform; > copy->vendor = model->vendor; > > return copy; > @@ -437,8 +634,6 @@ PowerPCDecode(virCPUDefPtr cpu, > const struct ppc_model *candidate; > virCPUDefPtr cpuCandidate; > virCPUDefPtr cpuModel = NULL; > - char *cpu_vendor = NULL; > - char *cpu_model = NULL; > unsigned int i; > > if (data == NULL || (map = ppcLoadMap()) == NULL) > @@ -475,13 +670,30 @@ PowerPCDecode(virCPUDefPtr cpu, > goto next; > } > > - if (VIR_ALLOC(cpuCandidate) < 0) { > - virReportOOMError(); > + if (!(cpuCandidate = ppcDataToCPU(data, candidate, map))) > goto out; > + > + if (candidate->vendor && cpuCandidate->vendor && > + STRNEQ(candidate->vendor->name, cpuCandidate->vendor)) { > + VIR_DEBUG("CPU vendor %s of model %s differs from %s; ignoring", > + candidate->vendor->name, candidate->name, > + cpuCandidate->vendor); > + virCPUDefFree(cpuCandidate); > + goto next; > } > > - cpuCandidate->model = strdup(candidate->name); > - cpuCandidate->vendor = strdup(candidate->vendor->name); > + if (cpu->type == VIR_CPU_TYPE_HOST) { > + cpuCandidate->type = VIR_CPU_TYPE_HOST; > + for (i = 0; i < cpuCandidate->nfeatures; i++) { > + switch (cpuCandidate->features[i].policy) { > + case VIR_CPU_FEATURE_DISABLE: > + virCPUDefFree(cpuCandidate); > + goto next; > + default: > + cpuCandidate->features[i].policy = -1; > + } > + } > + } > > if (preferred && STREQ(cpuCandidate->model, preferred)) { > virCPUDefFree(cpuModel); > @@ -489,19 +701,12 @@ PowerPCDecode(virCPUDefPtr cpu, > break; > } > > - ret = cpuMatch(data, &cpu_model, &cpu_vendor, candidate); > - if (ret < 0) { > - VIR_FREE(cpuCandidate); > - goto out; > - } else if (ret == 1) { > - cpuCandidate->model = cpu_model; > - cpuCandidate->vendor = cpu_vendor; > + if (cpuModel == NULL > + || cpuModel->nfeatures > cpuCandidate->nfeatures) { > virCPUDefFree(cpuModel); > cpuModel = cpuCandidate; > - break; > - } > - > - virCPUDefFree(cpuCandidate); > + }else > + virCPUDefFree(cpuCandidate); > > next: > candidate = candidate->next; > @@ -515,6 +720,8 @@ PowerPCDecode(virCPUDefPtr cpu, > > cpu->model = cpuModel->model; > cpu->vendor = cpuModel->vendor; > + cpu->nfeatures = cpuModel->nfeatures; > + cpu->features = cpuModel->features; > VIR_FREE(cpuModel); > > ret = 0; > @@ -537,19 +744,11 @@ static uint32_t ppc_mfpvr(void) > } > #endif > > -static void > -PowerPCDataFree(union cpuData *data) > -{ > - if (data == NULL) > - return; > - > - VIR_FREE(data); > -} > - > static union cpuData * > PowerPCNodeData(void) > { > union cpuData *data; > + virSysinfoDefPtr hostinfo; > > if (VIR_ALLOC(data) < 0) { > virReportOOMError(); > @@ -561,6 +760,18 @@ PowerPCNodeData(void) > data->ppc.pvr = ppc_mfpvr(); > #endif > > + hostinfo = virSysinfoRead(); > + if (hostinfo == NULL) { > + VIR_FREE(data); > + return NULL; > + } > + > + data->ppc.platform &= ~VIR_CPU_PPC64_POWERNV; > + > + if (STREQ(hostinfo->system_family, "PowerNV")) > + data->ppc.platform |= VIR_CPU_PPC64_POWERNV; > + virSysinfoDefFree(hostinfo); > + > return data; > } > > diff --git a/src/cpu/cpu_ppc_data.h b/src/cpu/cpu_ppc_data.h > index 685332a..0e691ce 100644 > --- a/src/cpu/cpu_ppc_data.h > +++ b/src/cpu/cpu_ppc_data.h > @@ -28,6 +28,10 @@ > > struct cpuPPCData { > uint32_t pvr; > + uint32_t platform; /* Bitflag indicating platform features */ > }; > > +#define VIR_CPU_PPC64_NONE 0x0 > +#define VIR_CPU_PPC64_POWERNV 0x1 I'm not sure what to say about this. Superficially it looks ok, but I'm not all that familiar with this code in libvirt, nor PPC. So I think I'd prefer Jiri to look at this at least from the libvirt POV.

> diff --git a/src/util/sysinfo.c b/src/util/sysinfo.c > index 6a5db80..5f98986 100644 > --- a/src/util/sysinfo.c > +++ b/src/util/sysinfo.c > @@ -234,7 +234,7 @@ virSysinfoRead(void) { > if (VIR_ALLOC(ret) < 0) > goto no_memory; > > - if (virFileReadAll(CPUINFO, 2048, &outbuf) < 0) { > + if (virFileReadAll(CPUINFO, 8192, &outbuf) < 0) { > virReportError(VIR_ERR_INTERNAL_ERROR, > _("Failed to open %s"), CPUINFO); > return NULL; This change seems like it probably ought to be separate. Daniel

From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; When getting CPUs' information, it assumes that CPU indexes are not contiguous. But for ppc64 platform, CPU indexes are not contiguous because SMT is needed to be disabled, so CPU information is not right on ppc64 and vpuinfo, vcpupin can't work corretly. This patch is to remove the assumption to be compatible with ppc64. Test: 4 vcpus are assigned to one VM and execute vcpuinfo command. Without patch: There is only one vcpu informaion can be listed. With patch: All vcpus' information can be listed correctly. Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_monitor_json.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 9991a0a..e130f8c 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -1230,13 +1230,6 @@ qemuMonitorJSONExtractCPUInfo(virJSONValuePtr reply, goto cleanup; } - if (cpu != i) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("unexpected cpu index %d expecting %d"), - i, cpu); - goto cleanup; - } -

threads[i] = thread; }

From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; Currently, -device xxx still can't work well for ppc64 platform. It's better use legacy USB option with default for ppc64. This patch is to legacy USB option with default for ppc64. Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; --- src/qemu/qemu_command.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 1c9bfc9..618dfb1 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5783,7 +5783,8 @@ qemuBuildCommandLine(virConnectPtr conn, } } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && cont->model == -1 && - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) { + (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || + def->os.arch == VIR_ARCH_PPC64)) { if (usblegacy) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("Multiple legacy USB controllers are "

On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: > On 2013年03月14日 14:54, Li Zhang wrote: >> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >> >> Currently, -device xxx still can't work well for ppc64 platform. >> It's better use legacy USB option with default for ppc64. >> >> This patch is to legacy USB option with default for ppc64. >> >> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >> --- >> src/qemu/qemu_command.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index 1c9bfc9..618dfb1 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -5783,7 +5783,8 @@ qemuBuildCommandLine(virConnectPtr conn, >> } >> } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && >> cont->model == -1 && >> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) { >> + (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || >> + def->os.arch == VIR_ARCH_PPC64)) { I think you ought to modify qemu_capabilities.c to clear the QEMU_CAPS_PIIX3_USB_UHCI flag when arch == ppc, instead of trying to modify every place which checks that cap.

Daniel

On Thu, Apr 11, 2013 at 05:53:41PM +0800, Li Zhang wrote: > On 2013年04月11日 17:41, Daniel P. Berrange wrote: >> On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: >>> On 2013年03月14日 14:54, Li Zhang wrote: >>>> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >>>> >>>> Currently, -device xxx still can't work well for ppc64 platform. >>>> It's better use legacy USB option with default for ppc64. >>>> >>>> This patch is to legacy USB option with default for ppc64. >>>> >>>> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >>>> --- >>>> src/qemu/qemu_command.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >>>> index 1c9bfc9..618dfb1 100644 >>>> --- a/src/qemu/qemu_command.c >>>> +++ b/src/qemu/qemu_command.c >>>> @@ -5783,7 +5783,8 @@ qemuBuildCommandLine(virConnectPtr conn, >>>> } >>>> } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && >>>> cont->model == -1 && >>>> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) { >>>> + (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || >>>> + def->os.arch == VIR_ARCH_PPC64)) { >> I think you ought to modify qemu_capabilities.c to clear the >> QEMU_CAPS_PIIX3_USB_UHCI flag when arch == ppc, instead of >> trying to modify every place which checks that cap. > Actually, PPC also can support PIIX3_USB_UHCI. > I think this patch is to set the default USB controller as in QEMU, > which can work correctly with USB keyboard and USB mouse. Can you explain what difference you're expecting ? This patch does not change the way the command line args are generated. It merely prevents you from listing multiple <controller> elements in the XML. If you're expecting any kind of functional change, you need more than what you have there.

Daniel

On Mon, Apr 15, 2013 at 10:55:07AM +0800, Li Zhang wrote: > On 2013年04月12日 18:11, Daniel P. Berrange wrote: >> On Thu, Apr 11, 2013 at 05:53:41PM +0800, Li Zhang wrote: >>> On 2013年04月11日 17:41, Daniel P. Berrange wrote: >>>> On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: >>>>> On 2013年03月14日 14:54, Li Zhang wrote: >>>>>> From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >>>>>> >>>>>> Currently, -device xxx still can't work well for ppc64 platform. >>>>>> It's better use legacy USB option with default for ppc64. >>>>>> >>>>>> This patch is to legacy USB option with default for ppc64. >>>>>> >>>>>> Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; >>>>>> --- >>>>>> src/qemu/qemu_command.c | 3 ++- >>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >>>>>> index 1c9bfc9..618dfb1 100644 >>>>>> --- a/src/qemu/qemu_command.c >>>>>> +++ b/src/qemu/qemu_command.c >>>>>> @@ -5783,7 +5783,8 @@ qemuBuildCommandLine(virConnectPtr conn, >>>>>> } >>>>>> } else if (cont->type == VIR_DOMAIN_CONTROLLER_TYPE_USB && >>>>>> cont->model == -1 && >>>>>> - !virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI)) { >>>>>> + (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_PIIX3_USB_UHCI) || >>>>>> + def->os.arch == VIR_ARCH_PPC64)) { >>>> I think you ought to modify qemu_capabilities.c to clear the >>>> QEMU_CAPS_PIIX3_USB_UHCI flag when arch == ppc, instead of >>>> trying to modify every place which checks that cap. >>> Actually, PPC also can support PIIX3_USB_UHCI. >>> I think this patch is to set the default USB controller as in QEMU, >>> which can work correctly with USB keyboard and USB mouse. >> Can you explain what difference you're expecting ? This patch does not >> change the way the command line args are generated. It merely prevents >> you from listing multiple <controller> elements in the XML. If you're >> expecting any kind of functional change, you need more than what you >> have there. > Let me explain this. > > If USB controller is not sepcified, libvirt adds one implicit > controller and model = -1. > This patch is to change command line to '-usb' for this situation for PPC64. > And QEMU will create one USB controller for it on PPC64. Please update your patch to provide a test case to prove that this actually works as you describe.

Daniel

On Thu, Mar 14, 2013 at 02:54:19PM +0800, Li Zhang wrote: > From: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > > For pSeries guest in QEMU, NVRAM is one kind of spapr-vio device. > Users are allowed to specify spapr-vio devices'address. > But NVRAM is not supported in libvirt. So this patch is to > add NVRAM device to allow users to specify its address. > > In QEMU, NVRAM device's address is specified by > "-global spapr-nvram.reg=xxxxx". > > In libvirt, XML file is defined as the following: > > <nvram> > <address type='spapr-vio' reg='0x3000'/> > </nvram> > > Signed-off-by: Li Zhang <zhlcindy(a)linux.vnet.ibm.com&gt; > --- > * v2 -> v1: > Add NVRAM parameters parsing in qemuParseCommandLine. > > src/conf/domain_conf.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++- > src/conf/domain_conf.h | 10 ++++++ > src/qemu/qemu_command.c | 48 +++++++++++++++++++++++++++ > src/qemu/qemu_command.h | 2 ++ > 4 files changed, 142 insertions(+), 1 deletion(-) When adding new XML, you also need to update docs/schemas/domaincommon.rng and docs/formatdomain.html.in In addition for anything which extends the QEMU command line generator you should be adding test case(s) to tests/qemuxml2argvtest.c and also tests/qemuargv2xmltest.c

> @@ -10572,6 +10608,32 @@ virDomainDefParseXML(virCapsPtr caps, > } > } > > + def->nvram = NULL; > + if ((n = virXPathNodeSet("./devices/nvram", ctxt, &nodes)) < 0) { > + goto error; > + } > + > + if (n > 1) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("only a single nvram device is supported")); > + goto error; > + } > + > + if (n > 0) { > + virDomainNVRAMDefPtr nvram = > + virDomainNVRAMDefParseXML(nodes[0], flags); > + if (!nvram) > + goto error; > + def->nvram = nvram; > + VIR_FREE(nodes); > + } else { > + virDomainNVRAMDefPtr nvram; > + if (VIR_ALLOC(nvram) < 0) > + goto no_memory; > + nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE; > + def->nvram = nvram; This adds a <nvram> device to every single guest whether it wants one or not, which is wrong. Just delete this entire 'else' block.

NB if you had run 'make check' you would see this flaw. > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index dee493f..30694d6 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -941,6 +941,13 @@ int qemuDomainAssignSpaprVIOAddresses(virDomainDefPtr def, > goto cleanup; > } > > + if (def->os.arch == VIR_ARCH_PPC64 && > + STREQ(def->os.machine, "pseries")) > + def->nvram->info.type = VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO; > + if (qemuAssignSpaprVIOAddress(def, &def->nvram->info, > + 0x3000ul) < 0) > + goto cleanup; Bad indentation level on the second 'if'

> +char * > +qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev) > +{ > + virBuffer buf = VIR_BUFFER_INITIALIZER; > + if (dev->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_SPAPRVIO && > + dev->info.addr.spaprvio.has_reg) > + virBufferAsprintf(&buf, "spapr-nvram.reg=0x%llx", > + dev->info.addr.spaprvio.reg); > + You should have an 'else' clause here to report an error in that scenario You must also check virBufferError() and raise an OOM error if required.

> + return virBufferContentAndReset(&buf); > +} > > char * > qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, > @@ -7006,6 +7024,19 @@ qemuBuildCommandLine(virConnectPtr conn, > } > } > > + if (def->nvram && > + def->os.arch == VIR_ARCH_PPC64 && > + STREQ(def->os.machine, "pseries")) { > + char *optstr; > + virCommandAddArg(cmd, "-global"); > + optstr = qemuBuildNVRAMDevStr(def->nvram); > + if (!optstr) > + goto error; > + if (optstr) > + virCommandAddArg(cmd, optstr); > + VIR_FREE(optstr); > + } You must have an else clause here and report VIR_ERR_CONFIG_UNSUPPORTED

> + > if (snapshot) > virCommandAddArgList(cmd, "-loadvm", snapshot->def->name, NULL); > > @@ -9139,6 +9170,23 @@ virDomainDefPtr qemuParseCommandLine(virCapsPtr qemuCaps, > goto error; > } > > + } else if (STREQ(arg, "-global") && > + STRPREFIX(progargv[i + 1], "spapr-nvram.reg=")) { > + > + WANT_VALUE(); > + > + if (VIR_ALLOC(def->nvram) < 0) > + goto no_memory; > + > + val += strlen("spapr-nvram.reg="); > + if (virStrToLong_ull(val, NULL, 16, > + &def->nvram->info.addr.spaprvio.reg) < 0) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("invalid value for spapr-nvram.reg :" > + "'%s'"), val); > + goto error; > + } > + > } else if (STREQ(arg, "-S")) { > /* ignore, always added by libvirt */ > } else { Regards, Daniel