[libvirt] [PATCH v2 1/1][RESEND] Add NVRAM device
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> --- * 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(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10f361c..8c1e8ae 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -175,7 +175,8 @@ VIR_ENUM_IMPL(virDomainDevice, VIR_DOMAIN_DEVICE_LAST, "redirdev", "smartcard", "chr", - "memballoon") + "memballoon", + "nvram") VIR_ENUM_IMPL(virDomainDeviceAddress, VIR_DOMAIN_DEVICE_ADDRESS_TYPE_LAST, "none", @@ -1442,6 +1443,16 @@ void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def) VIR_FREE(def); } +void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def) +{ + if (!def) + return; + + virDomainDeviceInfoClear(&def->info); + + VIR_FREE(def); +} + void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def) { if (!def) @@ -1602,6 +1613,7 @@ void virDomainDeviceDefFree(virDomainDeviceDefPtr def) case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST: break; } @@ -1791,6 +1803,7 @@ void virDomainDefFree(virDomainDefPtr def) virDomainWatchdogDefFree(def->watchdog); virDomainMemballoonDefFree(def->memballoon); + virDomainNVRAMDefFree(def->nvram); for (i = 0; i < def->nseclabels; i++) virSecurityLabelDefFree(def->seclabels[i]); @@ -2342,6 +2355,12 @@ int virDomainDeviceInfoIterate(virDomainDefPtr def, if (cb(def, &device, &def->memballoon->info, opaque) < 0) return -1; } + if (def->nvram) { + device.type = VIR_DOMAIN_DEVICE_NVRAM; + device.data.nvram = def->nvram; + if (cb(def, &device, &def->nvram->info, opaque) < 0) + return -1; + } device.type = VIR_DOMAIN_DEVICE_HUB; for (i = 0; i < def->nhubs ; i++) { device.data.hub = def->hubs[i]; @@ -7461,6 +7480,23 @@ error: goto cleanup; } +static virDomainNVRAMDefPtr +virDomainNVRAMDefParseXML(const xmlNodePtr node, + unsigned int flags) +{ + virDomainNVRAMDefPtr def; + + if (VIR_ALLOC(def) < 0) { + virReportOOMError(); + return NULL; + } + + if ( virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0 ) + return NULL; + + return def; +} + static virSysinfoDefPtr virSysinfoParseXML(const xmlNodePtr node, xmlXPathContextPtr ctxt) @@ -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; + } + /* analysis of the hub devices */ if ((n = virXPathNodeSet("./devices/hub", ctxt, &nodes)) < 0) { goto error; @@ -13547,6 +13609,21 @@ virDomainMemballoonDefFormat(virBufferPtr buf, } static int +virDomainNVRAMDefFormat(virBufferPtr buf, + virDomainNVRAMDefPtr def, + unsigned int flags) +{ + virBufferAsprintf(buf, " <nvram>\n"); + if (virDomainDeviceInfoIsSet(&def->info, flags)) + if (virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) + return -1; + + virBufferAddLit(buf, " </nvram>\n"); + + return 0; +} + +static int virDomainSysinfoDefFormat(virBufferPtr buf, virSysinfoDefPtr def) { @@ -14787,6 +14864,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, if (def->memballoon) virDomainMemballoonDefFormat(buf, def->memballoon, flags); + if (def->nvram) + virDomainNVRAMDefFormat(buf, def->nvram, flags); + virBufferAddLit(buf, " </devices>\n"); virBufferAdjustIndent(buf, 2); @@ -16061,6 +16141,7 @@ virDomainDeviceDefCopy(virCapsPtr caps, case VIR_DOMAIN_DEVICE_SMARTCARD: case VIR_DOMAIN_DEVICE_CHR: case VIR_DOMAIN_DEVICE_MEMBALLOON: + case VIR_DOMAIN_DEVICE_NVRAM: case VIR_DOMAIN_DEVICE_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("Copying definition of '%d' type " diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4ffa4aa..8e5f1d8 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -108,6 +108,9 @@ typedef virDomainChrDef *virDomainChrDefPtr; typedef struct _virDomainMemballoonDef virDomainMemballoonDef; typedef virDomainMemballoonDef *virDomainMemballoonDefPtr; +typedef struct _virDomainNVRAMDef virDomainNVRAMDef; +typedef virDomainNVRAMDef *virDomainNVRAMDefPtr; + typedef struct _virDomainSnapshotObj virDomainSnapshotObj; typedef virDomainSnapshotObj *virDomainSnapshotObjPtr; @@ -133,6 +136,7 @@ typedef enum { VIR_DOMAIN_DEVICE_SMARTCARD, VIR_DOMAIN_DEVICE_CHR, VIR_DOMAIN_DEVICE_MEMBALLOON, + VIR_DOMAIN_DEVICE_NVRAM, VIR_DOMAIN_DEVICE_LAST } virDomainDeviceType; @@ -158,6 +162,7 @@ struct _virDomainDeviceDef { virDomainSmartcardDefPtr smartcard; virDomainChrDefPtr chr; virDomainMemballoonDefPtr memballoon; + virDomainNVRAMDefPtr nvram; } data; }; @@ -1418,6 +1423,9 @@ struct _virDomainMemballoonDef { virDomainDeviceInfo info; }; +struct _virDomainNVRAMDef { + virDomainDeviceInfo info; +}; enum virDomainSmbiosMode { VIR_DOMAIN_SMBIOS_NONE, @@ -1849,6 +1857,7 @@ struct _virDomainDef { /* Only 1 */ virDomainWatchdogDefPtr watchdog; virDomainMemballoonDefPtr memballoon; + virDomainNVRAMDefPtr nvram; virCPUDefPtr cpu; virSysinfoDefPtr sysinfo; virDomainRedirFilterDefPtr redirfilter; @@ -1951,6 +1960,7 @@ int virDomainChrSourceDefCopy(virDomainChrSourceDefPtr src, void virDomainSoundCodecDefFree(virDomainSoundCodecDefPtr def); void virDomainSoundDefFree(virDomainSoundDefPtr def); void virDomainMemballoonDefFree(virDomainMemballoonDefPtr def); +void virDomainNVRAMDefFree(virDomainNVRAMDefPtr def); void virDomainWatchdogDefFree(virDomainWatchdogDefPtr def); void virDomainVideoDefFree(virDomainVideoDefPtr def); virDomainHostdevDefPtr virDomainHostdevDefAlloc(void); 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; + /* No other devices are currently supported on spapr-vio */ ret = 0; @@ -3406,6 +3413,17 @@ error: return NULL; } +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); + + 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); + } + 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 { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index e4db000..7a0fe17 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -118,6 +118,8 @@ char * qemuBuildWatchdogDevStr(virDomainWatchdogDefPtr dev, char * qemuBuildMemballoonDevStr(virDomainMemballoonDefPtr dev, virQEMUCapsPtr qemuCaps); +char * qemuBuildNVRAMDevStr(virDomainNVRAMDefPtr dev); + char * qemuBuildUSBInputDevStr(virDomainInputDefPtr dev, virQEMUCapsPtr qemuCaps); -- 1.7.10.1
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> 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. Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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; 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*/ + QEMU_CAPS_USB_OPT = 129, /* -machine xxxx,usb=off*/ QEMU_CAPS_LAST, /* this must always be the last item */ }; 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)); } return 0; diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 4357068..296503b 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -361,8 +361,8 @@ mymain(void) DO_TEST("minimal-s390", QEMU_CAPS_NAME); DO_TEST("machine-aliases1", NONE); DO_TEST("machine-aliases2", QEMU_CAPS_KVM); - DO_TEST("machine-core-on", QEMU_CAPS_DUMP_GUEST_CORE); - DO_TEST("machine-core-off", QEMU_CAPS_DUMP_GUEST_CORE); + DO_TEST("machine-core-on", QEMU_CAPS_MACH_OPT, QEMU_CAPS_DUMP_GUEST_CORE); + DO_TEST("machine-core-off", QEMU_CAPS_MACH_OPT, QEMU_CAPS_DUMP_GUEST_CORE); DO_TEST_FAILURE("machine-core-on", NONE); DO_TEST("boot-cdrom", NONE); DO_TEST("boot-network", NONE); -- 1.7.10.1
On Thu, Mar 14, 2013 at 02:54:20PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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 -- |: 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 2013年03月14日 19:11, Daniel P. Berrange wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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
On Thu, Mar 14, 2013 at 02:54:20PM +0800, Li Zhang wrote: the use of -machine, and a second patch to deal with the usb=off part. OK, I will split it.
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. Here is the reason.
In QEMU, vl.c defines one usb_enable() which is to set USB option. Actually, this USB option is one of global machine options. -nodefaults doesn't set this option. bool usb_enabled(bool default_usb) { QemuOpts *mach_opts; mach_opts = qemu_opts_find(qemu_find_opts("machine"), 0); if (mach_opts) { return qemu_opt_get_bool(mach_opts, "usb", default_usb); } return default_usb; } In machine->init(), For sPAPR, it will create USB controller according to this option. if (usb_enabled(spapr->has_graphics)) { pci_create_simple(phb->bus, -1, "pci-ohci"); if (spapr->has_graphics) { usbdevice_create("keyboard"); usbdevice_create("mouse"); } } This code says that, it will create one USB controller if VGA is enabled. With this code, libvirt also create another USB controller by -device xxxx. Then QEMU will report one error. Actually, if with legacy USB (-usb), this error won't occur. Because -usb is just to set USB option as on(usb=on). As another patch of mine, I also want to use legacy USB as default which will be created int machine->init() when there is no specific controller is defined in XML.
Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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.
Oh, maybe I should set this version as 1.3.0. The official release of 1.2.0 doesn't support this option AFAIK.
For the USB property just use version 1.3.0 - don't try to do stuff based on development snapshot version numbers.
OK, I will correct it. Thanks. :)
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 OK, I will correct it.
+ QEMU_CAPS_USB_OPT = 129, /* -machine xxxx,usb=off*/ I'd prefer this second one to be QEMU_CAPS_MACHINE_USB_OPT OK, I will correct.
Thanks a lot for your review. :)
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@linux.vnet.ibm.com> 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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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; -- 1.7.10.1
Hi, Could anyone give more comment? Thanks. :) On 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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 -- |: 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 2013年04月11日 17:50, Daniel P. Berrange wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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
On Thu, Mar 14, 2013 at 02:54:21PM +0800, Li Zhang wrote: prefer Jiri to look at this at least from the libvirt POV.
Sure, thanks. Jiri, would you please help review this patch?
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
On Thu, Mar 14, 2013 at 14:54:21 +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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(-)
Looks like this patch was not even rebased since it was written back in time 1.0.1 was released. Anyway, I realized I did not push my changes to powerpc driver so I did that. And I also started rewriting this patch on top of it since this patch is rather huge and seems to mix lots of things. Also PowerPC CPUs seem to be quite different from x86 CPUs so enhancing powerpc driver by copy&pasting code from x86 driver is not the best way :-) It just makes powerpc driver unnecessarily complicated.
<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>
So what is this "powernv" feature used for? It won't show up in capabilities XML as it is included in all powerpc CPU models. That also means, users don't need to explicitly enable it when configuring guest CPU. Thus it could only make sense to allow users to disable this feature for a given guest. However, I don't see "powernv" string anywhere in QEMU source code and thus it cannot really be used in any way in guest CPU definition. Jirka
On 2013年04月19日 22:04, Jiri Denemark wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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(-) Looks like this patch was not even rebased since it was written back in time 1.0.1 was released. Anyway, I realized I did not push my changes to
On Thu, Mar 14, 2013 at 14:54:21 +0800, Li Zhang wrote: powerpc driver so I did that. And I also started rewriting this patch on top of it since this patch is rather huge and seems to mix lots of things. Also PowerPC CPUs seem to be quite different from x86 CPUs so enhancing powerpc driver by copy&pasting code from x86 driver is not the best way :-) It just makes powerpc driver unnecessarily complicated.
We hope it matches x86 driver, so most code are from x86. There is no CPUID on powerpc, but we also need feature such as "powernv".
<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>
So what is this "powernv" feature used for? It won't show up in capabilities XML as it is included in all powerpc CPU models. That also
This feature means that powerpc can support KVM. Other platform, for example, pseries doesn't support KVM. So we need this feature. Currently, we haven't introduced this feature in capabilities XML yet.
means, users don't need to explicitly enable it when configuring guest CPU. Thus it could only make sense to allow users to disable this feature for a given guest. However, I don't see "powernv" string anywhere in QEMU source code and thus it cannot really be used in any way in guest CPU definition.
This feature is from sysinfo on host, not from QEMU. QEMU haven't included this feature yet. I am not sure whether it is necessary to introduce this to QEMU.
Jirka
On Mon, Apr 22, 2013 at 10:39:46 +0800, Li Zhang wrote:
On 2013年04月19日 22:04, Jiri Denemark wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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(-) Looks like this patch was not even rebased since it was written back in time 1.0.1 was released. Anyway, I realized I did not push my changes to
On Thu, Mar 14, 2013 at 14:54:21 +0800, Li Zhang wrote: powerpc driver so I did that. And I also started rewriting this patch on top of it since this patch is rather huge and seems to mix lots of things. Also PowerPC CPUs seem to be quite different from x86 CPUs so enhancing powerpc driver by copy&pasting code from x86 driver is not the best way :-) It just makes powerpc driver unnecessarily complicated.
We hope it matches x86 driver, so most code are from x86. There is no CPUID on powerpc, but we also need feature such as "powernv".
And CPUID is what makes x86 driver so complicated. That's why I think it makes more sense to implement the right functionality from scratch rather than copy&pasting x86 driver :-)
So what is this "powernv" feature used for? It won't show up in capabilities XML as it is included in all powerpc CPU models. That also
This feature means that powerpc can support KVM. Other platform, for example, pseries doesn't support KVM. So we need this feature.
Currently, we haven't introduced this feature in capabilities XML yet.
means, users don't need to explicitly enable it when configuring guest CPU. Thus it could only make sense to allow users to disable this feature for a given guest. However, I don't see "powernv" string anywhere in QEMU source code and thus it cannot really be used in any way in guest CPU definition.
This feature is from sysinfo on host, not from QEMU. QEMU haven't included this feature yet. I am not sure whether it is necessary to introduce this to QEMU.
I think I understand what powernv feature is but I don't get what it is (planned to be) used for in libvirt. And I'd like to understand that :-) The CPU modeling stuff in libvirt is used for *guest* CPU configuration, i.e., users can configure what CPU model and features the guest should see. So unless you want it to enable/disable something like nested virt, I don't see a lot of sense in introducing such feature. (I may be wrong of course, that's why I'm asking.) Capabilities XML provides some details about host CPU but that's mostly to give an idea about what guest CPU configuration the host could be able to provide. If you want to add powernv feature just because you need to distinguish if the host supports KVM or not, there's much better way... In guest section of capabilities XML, KVM support is indicated by <domain type='kvm'> element. And that's what existing apps already use to detect KVM presence/absence. Jirka
On 2013年04月22日 17:10, Jiri Denemark wrote:
On Mon, Apr 22, 2013 at 10:39:46 +0800, Li Zhang wrote:
On 2013年04月19日 22:04, Jiri Denemark wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@in.ibm.com> Signed-off-by: Li Zhang <zhlcindy@linux.vnet.ibm.com> --- 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(-) Looks like this patch was not even rebased since it was written back in time 1.0.1 was released. Anyway, I realized I did not push my changes to
On Thu, Mar 14, 2013 at 14:54:21 +0800, Li Zhang wrote: powerpc driver so I did that. And I also started rewriting this patch on top of it since this patch is rather huge and seems to mix lots of things. Also PowerPC CPUs seem to be quite different from x86 CPUs so enhancing powerpc driver by copy&pasting code from x86 driver is not the best way :-) It just makes powerpc driver unnecessarily complicated. We hope it matches x86 driver, so most code are from x86. There is no CPUID on powerpc, but we also need feature such as "powernv". And CPUID is what makes x86 driver so complicated. That's why I think it makes more sense to implement the right functionality from scratch rather than copy&pasting x86 driver :-)
OK, you are right.
So what is this "powernv" feature used for? It won't show up in capabilities XML as it is included in all powerpc CPU models. That also This feature means that powerpc can support KVM. Other platform, for example, pseries doesn't support KVM. So we need this feature.
Currently, we haven't introduced this feature in capabilities XML yet.
means, users don't need to explicitly enable it when configuring guest CPU. Thus it could only make sense to allow users to disable this feature for a given guest. However, I don't see "powernv" string anywhere in QEMU source code and thus it cannot really be used in any way in guest CPU definition. This feature is from sysinfo on host, not from QEMU. QEMU haven't included this feature yet. I am not sure whether it is necessary to introduce this to QEMU. I think I understand what powernv feature is but I don't get what it is (planned to be) used for in libvirt. And I'd like to understand that :-) The CPU modeling stuff in libvirt is used for *guest* CPU configuration, i.e., users can configure what CPU model and features the guest should see. So unless you want it to enable/disable something like nested virt, I don't see a lot of sense in introducing such feature. (I may be wrong of course, that's why I'm asking.) Capabilities XML provides some details about host CPU but that's mostly to give an idea about what guest CPU configuration the host could be able to provide.
I see. This feature is read-only. Nested virt isn't considered yet. Actually, I haven't understood this completely. I think PowerPC doesn't support nest virt. I just thought that migration is only allowed with 'powernv'. Currently, only powernv platform can work with KVM. In the future, there may be more features of CPU.
If you want to add powernv feature just because you need to distinguish if the host supports KVM or not, there's much better way... In guest section of capabilities XML, KVM support is indicated by <domain type='kvm'> element. And that's what existing apps already use to detect KVM presence/absence.
As my understanding, there is still some difference from 'kvm' capability. 'powernv' is only considered as one CPU feature of PPC64. If other PPC platforms support KVM in the future, this feature can be used to identify whether migration can be executed . :)
Jirka
-- Li Zhang IBM China Linux Technology Centre
On Mon, Apr 22, 2013 at 17:38:23 +0800, Li Zhang wrote:
On 2013年04月22日 17:10, Jiri Denemark wrote:
If you want to add powernv feature just because you need to distinguish if the host supports KVM or not, there's much better way... In guest section of capabilities XML, KVM support is indicated by <domain type='kvm'> element. And that's what existing apps already use to detect KVM presence/absence.
As my understanding, there is still some difference from 'kvm' capability. 'powernv' is only considered as one CPU feature of PPC64. If other PPC platforms support KVM in the future, this feature can be used to identify whether migration can be executed . :)
Well, as I already said, unless there is a way to explicitly enable/disable powernv feature in guest CPU, I don't see any reason for exposing the feature to users/apps. We definitely don't want apps/users to detect support for migration (or anything else) by checking host CPU features. If migration is not supported, then any migration API can fail with VIR_ERR_OPERATION_UNSUPPORTED. That said, powernv feature can be used internally by libvirt to check if some configurations/operations are supported but it doesn't have to be exposed to users/apps. The CPU driver stuff is there for configuring *guest* CPU. Jirka
On 2013年04月29日 22:26, Jiri Denemark wrote:
On Mon, Apr 22, 2013 at 17:38:23 +0800, Li Zhang wrote:
On 2013年04月22日 17:10, Jiri Denemark wrote:
If you want to add powernv feature just because you need to distinguish if the host supports KVM or not, there's much better way... In guest section of capabilities XML, KVM support is indicated by <domain type='kvm'> element. And that's what existing apps already use to detect KVM presence/absence. As my understanding, there is still some difference from 'kvm' capability. 'powernv' is only considered as one CPU feature of PPC64. If other PPC platforms support KVM in the future, this feature can be used to identify whether migration can be executed . :) Well, as I already said, unless there is a way to explicitly enable/disable powernv feature in guest CPU, I don't see any reason for exposing the feature to users/apps. We definitely don't want apps/users to detect support for migration (or anything else) by checking host CPU features. If migration is not supported, then any migration API can fail with VIR_ERR_OPERATION_UNSUPPORTED. That said, powernv feature can be used internally by libvirt to check if some configurations/operations are supported but it doesn't have to be exposed to users/apps. The CPU driver stuff is there for configuring *guest* CPU.
I see, got it. This feature can be removed. So it is not necessary to add <feature> in CPU driver for ppc64. Thanks.
Jirka
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> --- 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; } -- 1.7.10.1
On Thu, Mar 14, 2013 at 02:54:22PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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; - } -
Since you removed this, the 'cpu' variable is completely unused, so you should also remove earlier logic which populates it
threads[i] = thread; }
There is similar code to this in qemu_monitor_text.c which needs to be removed too if (vcpu != (lastVcpu + 1)) goto error; 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 2013年03月14日 19:14, Daniel P. Berrange wrote:
On Thu, Mar 14, 2013 at 02:54:22PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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; - } - Since you removed this, the 'cpu' variable is completely unused, so you should also remove earlier logic which populates it OK, I will remove that. threads[i] = thread; }
There is similar code to this in qemu_monitor_text.c which needs to be removed too
if (vcpu != (lastVcpu + 1)) goto error;
OK, I will remove this in next version. Thanks a lot. :)
Daniel
From: Li Zhang <zhlcindy@linux.vnet.ibm.com> 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@linux.vnet.ibm.com> --- 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 " -- 1.7.10.1
Hi all, Any comment about this patch? Thanks.:) On 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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 " Hi Daniel,
What about this patch? I think it's better to use legacy USB on PPC64 platform as default. Otherwise, it still needs to add default USB mouse and keyboard when graphic is enabled. Thanks. :) --Li
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@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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 -- |: 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 2013年04月11日 17:41, Daniel P. Berrange wrote:
On 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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
On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: 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.
In libvirt, there is no USB keyboard and mouse when no USB controller is specified.
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 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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
On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: 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 -- |: 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 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 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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
On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: 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. For XML file, multiple USB controllers still can be added as long as the model is specified. PIIX3_USB_UHCI also can be added on PPC64 because QEMU has such a capability. Thanks. :) --Li
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 2013年03月14日 14:54, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- 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
On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: 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 -- |: 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 2013年04月15日 18:16, Daniel P. Berrange 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 2013年03月14日 14:54, Li Zhang wrote: > From: Li Zhang <zhlcindy@linux.vnet.ibm.com> > > 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@linux.vnet.ibm.com> > --- > 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
On Sun, Apr 07, 2013 at 05:00:06PM +0800, Li Zhang wrote: 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
On Mon, Apr 15, 2013 at 10:55:07AM +0800, Li Zhang wrote: this actually works as you describe.
Sure, I will send out later. Thanks. :)
Daniel
On Thu, Mar 14, 2013 at 02:54:19PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- * 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 -- |: 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 2013年03月14日 19:02, Daniel P. Berrange wrote:
On Thu, Mar 14, 2013 at 02:54:19PM +0800, Li Zhang wrote:
From: Li Zhang <zhlcindy@linux.vnet.ibm.com>
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@linux.vnet.ibm.com> --- * 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 Sure, I will do that in my next version.
@@ -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.
In QEMU, NVRAM device is always enabled. So I would like to add this device in libvirt with default.
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'
I will correct it.
+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.
OK. I will add that.
+ 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
OK, I will add that in next version.
+ 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
participants (4)
-
Daniel P. Berrange -
Jiri Denemark -
Li Zhang -
Li Zhang