[libvirt] Rudimentary (basic) s390x architecture functions for libvirt
Hello there, I don't know whether or not you are only interested in the x86 / x86_64 architecture, but I think you could also be interested in the small bit of code which I have programmed. Since the release of libvirt 0.8.5, there is a small bit of s390x architecture code (one line) within libvirt which has given me the impression that you could be interested in my small bit of code. I study 'computer science' at the University of Tuebingen (Germany) and each student has to do a research study during his studies on his way to receiving his diploma. One of my goals is to implement basic s390x functions into libvirt in order to be able to start / create virtual machines on a s390x architecture (i.e. on our IBM Mainframe z9 at the university) more easily. Our Setup is (as just mentioned) an IBM Mainframe z9 on which I have the root access to a zLinux (which runs native on two LPARs) with the OS "SUSE Enterprise 11 SP1" and a kernel 2.6.34. (Our test virtual machine is a copy of the running zLinux.) I have actually implemented my patch into libvirt release 0.8.6 but I think a git patch would be more practical for you, so I have implemented it into git today (see underneath or attachment). The following XML file creates a virtual machine on my zLinux with my git patch which is listed underneath (or attachment). As you can see, there is not much that needs to be done so that libvirt can create rudimentary virtual machines on IBM Mainframe z9 (under an zLinux). Regards, Patrick Siegl University of Tuebingen (Germany) Here is the necessary XML file: =============================================================== <domain type='kvm'> <name>TestVM</name> <uuid></uuid> <memory>1024000</memory> <currentMemory>1024000</currentMemory> <cpu> <topology sockets='1' cores='1' threads='1'/> </cpu> <os> <type arch="s390x" machine='s390'>hvm</type> <kernel>/boot/image-2.6.34-tue01</kernel> <initrd>/boot/initrd-2.6.34-tue01_GUEST</initrd> <cmdline>"root=/dev/vda1 rw"</cmdline> <boot dev='hd'/> </os> <devices> <graphics type='vnc' port='5903' listen='' keymap='de'/> <console type='vc'> <target type='virtio' name='virtio.serial'/> </console> <disk type='file' device='disk'> <source file='/data/sle11.img'/> <target dev='vda'/> </disk> <interface type='ethernet'> <target dev='tap0'/> <mac address='00:17:31:88:AC:86'/> <ip address='134.2.14.241'/> </interface> </devices> </domain> ... and here is the patch: =============================================================== diff --git a/libvirt-git/src/qemu/qemu_capabilities.c b/libvirt/src/qemu/qemu_capabilities.c index faf7d44..7bbb1d5 100644 --- a/libvirt-git/src/qemu/qemu_capabilities.c +++ b/libvirt/src/qemu/qemu_capabilities.c @@ -83,7 +83,7 @@ static const struct qemu_arch_info const arch_info_hvm[] = { { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 }, { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 }, { "itanium", 64, NULL, "qemu-system-ia64", NULL, NULL, 0 }, - { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, + { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, // Since 0.8.5: doesn't work on it's own! }; static const struct qemu_arch_info const arch_info_xen[] = { diff --git a/libvirt-git/src/nodeinfo.c b/libvirt/src/nodeinfo.c index 22d53e5..c9059d4 100644 --- a/libvirt-git/src/nodeinfo.c +++ b/libvirt/src/nodeinfo.c @@ -164,7 +164,11 @@ cleanup: static int parse_socket(unsigned int cpu) { +# if ( defined __s390x__ || defined __s390__ ) + return 0; /* s390 does not implement physical_package_id by now */ +# else return get_cpu_value(cpu, "topology/physical_package_id", false); +# endif } int linuxNodeInfoCPUPopulate(FILE *cpuinfo, @@ -195,6 +199,30 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, * has no knowledge of NUMA nodes */ /* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ +# if defined __s390x__ || defined __s390__ + while (fgets(line, sizeof(line), cpuinfo) != NULL) { + char *buf = line; + if (STRPREFIX(buf, "# processors")) { /* Number of processors */ + char *p; + unsigned int id; + buf += 12; + while (*buf && c_isspace(*buf)) + buf++; + if (*buf != ':' || !buf[1]) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "parsing cpuinfo cpu count %c", *buf); + return -1; + } + if (virStrToLong_ui(buf+1, &p, 10, &id) == 0 + && (*p == '\0' || c_isspace(*p)) + && id > nodeinfo->cores) { + nodeinfo->cpus = id; + nodeinfo->cores = 1; + nodeinfo->mhz = 1400; /* z9 BC */ + } + } + } +# else /* no s390 */ while (fgets(line, sizeof(line), cpuinfo) != NULL) { char *buf = line; if (STRPREFIX(buf, "processor")) { /* aka a single logical CPU */ @@ -239,6 +267,7 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, nodeinfo->cores = id; } } +# endif /* s390 */ if (!nodeinfo->cpus) { nodeReportError(VIR_ERR_INTERNAL_ERROR, diff --git a/libvirt-git/src/qemu/qemu_command.c b/libvirt/src/qemu/qemu_command.c index c20f031..0419a0c 100644 --- a/libvirt-git/src/qemu/qemu_command.c +++ b/libvirt/src/qemu/qemu_command.c @@ -1267,9 +1267,15 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; } } else { +# if ( defined __s390x__ || defined __s390__ ) + virBufferVSprintf(&opt, "file=%s", disk->src); +# else virBufferVSprintf(&opt, "file=%s,", disk->src); +# endif } } +// makes problems under s390x +# if ! ( defined __s390x__ || defined __s390__ ) if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) virBufferAddLit(&opt, "if=none"); else @@ -1291,6 +1297,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferVSprintf(&opt, ",unit=%d", unitid); } } +# endif if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && disk->bus != VIR_DOMAIN_DISK_BUS_IDE) @@ -1298,10 +1305,12 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (disk->readonly && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_READONLY) virBufferAddLit(&opt, ",readonly=on"); +# if ! ( defined __s390x__ || defined __s390__ ) if (disk->driverType && *disk->driverType != '\0' && disk->type != VIR_DOMAIN_DISK_TYPE_DIR && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT) virBufferVSprintf(&opt, ",format=%s", disk->driverType); +# endif if (disk->serial && (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_SERIAL)) { if (qemuSafeSerialParamValue(disk->serial) < 0) @@ -1370,8 +1379,12 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk, disk->info.addr.drive.unit); break; case VIR_DOMAIN_DISK_BUS_VIRTIO: +# if ( defined __s390x__ || defined __s390__ ) + virBufferAddLit(&opt, "virtio-blk-s390"); +# else virBufferAddLit(&opt, "virtio-blk-pci"); qemuBuildDeviceAddressStr(&opt, &disk->info); +# endif break; case VIR_DOMAIN_DISK_BUS_USB: virBufferAddLit(&opt, "usb-storage"); @@ -1476,6 +1489,9 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) break; case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: +# if ( defined __s390x__ || defined __s390__ ) + virBufferAddLit(&buf, "virtio-serial-s390"); +# else if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virBufferAddLit(&buf, "virtio-serial-pci"); } else { @@ -1491,6 +1507,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",vectors=%d", def->opts.vioserial.vectors); } +# endif break; /* We always get an IDE controller, whether we want it or not. */ @@ -1499,6 +1516,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) goto error; } +# if ! ( defined __s390x__ || defined __s390__ ) if (qemuBuildDeviceAddressStr(&buf, &def->info) < 0) goto error; @@ -1506,6 +1524,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virReportOOMError(); goto error; } +# endif return virBufferContentAndReset(&buf); @@ -1551,7 +1570,11 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, if (!net->model) { nic = "rtl8139"; } else if (STREQ(net->model, "virtio")) { +# if ( defined __s390x__ || defined __s390__ ) + nic = "virtio-net-s390"; +# else nic = "virtio-net-pci"; +# endif } else { nic = net->model; } @@ -2587,7 +2610,11 @@ qemuBuildCommandLine(virConnectPtr conn, break; } - cmd = virCommandNewArgList(emulator, "-S", NULL); +# if ! ( defined __s390x__ || defined __s390__ ) + cmd = virCommandNewArgList(emulator, "-S", NULL); // param '-S' makes problems under s390x +# else + cmd = virCommandNewArgList(emulator, "", NULL); +# endif virCommandAddEnvPassCommon(cmd); @@ -2909,8 +2936,10 @@ qemuBuildCommandLine(virConnectPtr conn, def->onReboot != VIR_DOMAIN_LIFECYCLE_RESTART) virCommandAddArg(cmd, "-no-reboot"); +# if ! ( defined __s390x__ || defined __s390__ ) if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) virCommandAddArg(cmd, "-no-acpi"); +# endif if (!def->os.bootloader) { for (i = 0 ; i < def->os.nBootDevs ; i++) { @@ -2980,6 +3009,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } +# if ! ( defined __s390x__ || defined __s390__ ) if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { virDomainControllerDefPtr cont = def->controllers[i]; @@ -3008,6 +3038,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); } } +# endif /* If QEMU supports -drive param instead of old -hda, -hdb, -cdrom .. */ if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) { @@ -3106,6 +3137,7 @@ qemuBuildCommandLine(virConnectPtr conn, } } +# if ! ( defined __s390x__ || defined __s390__ ) // s390x doesn't need DeviceArg yet (it doesn't even work with ...) if (withDeviceArg) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { virCommandAddArg(cmd, "-global"); @@ -3131,6 +3163,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(optstr); } } +# endif } } else { for (i = 0 ; i < def->ndisks ; i++) { @@ -3277,11 +3310,13 @@ qemuBuildCommandLine(virConnectPtr conn, char vhostfd_name[50] = ""; int vlan; +# if ! ( defined __s390x__ || defined __s390__ ) /* VLANs are not used with -netdev, so don't record them */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) vlan = -1; else +# endif vlan = i; if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || @@ -3338,6 +3373,7 @@ qemuBuildCommandLine(virConnectPtr conn, * * NB, no support for -netdev without use of -device */ +# if ! ( defined __s390x__ || defined __s390__ ) // Old way works on IBM z9 Tuebingen; maybe there are other z9's which also work with "semi-" or "best way" if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { virCommandAddArg(cmd, "-netdev"); @@ -3354,21 +3390,26 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, nic); VIR_FREE(nic); } else { +# endif virCommandAddArg(cmd, "-net"); if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) goto error; virCommandAddArg(cmd, nic); VIR_FREE(nic); +# if ! ( defined __s390x__ || defined __s390__ ) } if (!((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))) { +# endif virCommandAddArg(cmd, "-net"); if (!(host = qemuBuildHostNetStr(net, ',', vlan, tapfd_name, vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); +# if ! ( defined __s390x__ || defined __s390__ ) } +# endif } } @@ -3384,6 +3425,7 @@ qemuBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { +# if ! ( defined __s390x__ || defined __s390__ ) virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&serial->source, serial->info.alias))) @@ -3394,6 +3436,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "isa-serial,chardev=%s", serial->info.alias); +# endif; } else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -3483,6 +3526,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); +# if ( defined __s390x__ || defined __s390__ ) + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, "virtio-serial-s390"); +# endif + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel))) goto error; @@ -3512,6 +3560,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr); +# if ( defined __s390x__ || defined __s390__ ) + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, "virtio-serial-s390"); +# endif + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(console))) goto error; @@ -4011,6 +4064,8 @@ qemuBuildCommandLine(virConnectPtr conn, * NB: Earlier we declared that VirtIO balloon will always be in * slot 0x3 on bus 0x0 */ +// our SUSE Enterprise, running on one LPAR (IBM z9), doesn't have ballon support ... +# if ! ( defined __s390x__ || defined __s390__ ) if ((def->memballoon) && (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) { if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) { @@ -4032,6 +4087,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-balloon", "virtio", NULL); } } +# endif if (current_snapshot && current_snapshot->def->active) virCommandAddArgList(cmd, "-loadvm", current_snapshot->def->name,
On Mon, Jan 24, 2011 at 09:22:04PM +0100, Patrick Siegl wrote:
Hello there,
I don't know whether or not you are only interested in the x86 / x86_64 architecture, but I think you could also be interested in the small bit of code which I have programmed. Since the release of libvirt 0.8.5, there is a small bit of s390x architecture code (one line) within libvirt which has given me the impression that you could be interested in my small bit of code.
Most of the developers only have access to x86, so s390x hasn't been a priority. We're happy to accept patches from anyone who is in a position to actually test s390 functionality.
... and here is the patch: =============================================================== diff --git a/libvirt-git/src/qemu/qemu_capabilities.c b/libvirt/src/qemu/qemu_capabilities.c index faf7d44..7bbb1d5 100644 --- a/libvirt-git/src/qemu/qemu_capabilities.c +++ b/libvirt/src/qemu/qemu_capabilities.c @@ -83,7 +83,7 @@ static const struct qemu_arch_info const arch_info_hvm[] = { { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 }, { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 }, { "itanium", 64, NULL, "qemu-system-ia64", NULL, NULL, 0 }, - { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, + { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, // Since 0.8.5: doesn't work on it's own!
Not sure this comment serves any purpose.
diff --git a/libvirt-git/src/nodeinfo.c b/libvirt/src/nodeinfo.c index 22d53e5..c9059d4 100644 --- a/libvirt-git/src/nodeinfo.c +++ b/libvirt/src/nodeinfo.c @@ -164,7 +164,11 @@ cleanup:
static int parse_socket(unsigned int cpu) { +# if ( defined __s390x__ || defined __s390__ ) + return 0; /* s390 does not implement physical_package_id by now */ +# else return get_cpu_value(cpu, "topology/physical_package_id", false); +# endif }
Looks fine.
int linuxNodeInfoCPUPopulate(FILE *cpuinfo, @@ -195,6 +199,30 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, * has no knowledge of NUMA nodes */
/* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ +# if defined __s390x__ || defined __s390__ + while (fgets(line, sizeof(line), cpuinfo) != NULL) { + char *buf = line; + if (STRPREFIX(buf, "# processors")) { /* Number of processors */ + char *p; + unsigned int id; + buf += 12; + while (*buf && c_isspace(*buf)) + buf++; + if (*buf != ':' || !buf[1]) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "parsing cpuinfo cpu count %c", *buf); + return -1; + } + if (virStrToLong_ui(buf+1, &p, 10, &id) == 0 + && (*p == '\0' || c_isspace(*p)) + && id > nodeinfo->cores) { + nodeinfo->cpus = id; + nodeinfo->cores = 1; + nodeinfo->mhz = 1400; /* z9 BC */ + } + } + } +# else /* no s390 */
Be nice if mhz wasn't hardcoded and could be got from sysfs or procfs, but not super critical since I doubt anyone really cares about this value.
diff --git a/libvirt-git/src/qemu/qemu_command.c b/libvirt/src/qemu/qemu_command.c index c20f031..0419a0c 100644 --- a/libvirt-git/src/qemu/qemu_command.c +++ b/libvirt/src/qemu/qemu_command.c @@ -1267,9 +1267,15 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; } } else { +# if ( defined __s390x__ || defined __s390__ ) + virBufferVSprintf(&opt, "file=%s", disk->src); +# else virBufferVSprintf(&opt, "file=%s,", disk->src); +# endif } } +// makes problems under s390x +# if ! ( defined __s390x__ || defined __s390__ ) if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) virBufferAddLit(&opt, "if=none"); else @@ -1291,6 +1297,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferVSprintf(&opt, ",unit=%d", unitid); } } +# endif if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && disk->bus != VIR_DOMAIN_DISK_BUS_IDE) @@ -1298,10 +1305,12 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (disk->readonly && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_READONLY) virBufferAddLit(&opt, ",readonly=on"); +# if ! ( defined __s390x__ || defined __s390__ ) if (disk->driverType && *disk->driverType != '\0' && disk->type != VIR_DOMAIN_DISK_TYPE_DIR && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT) virBufferVSprintf(&opt, ",format=%s", disk->driverType); +# endif if (disk->serial && (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_SERIAL)) { if (qemuSafeSerialParamValue(disk->serial) < 0)
These changes are seriously dubious. The QEMU command line syntax is architecture independant, so this should not be changed. In addition *any* use of '#ifdef s390' in qemu_command.c is likely wrong. That is changing the behaviour based on what host libvirt is compiled for. We need to change behaviour based on what architecture the guest is emulating. ie, if we run a purely emulated s390 QEMU on an x86 host, we need these changes. If we ran a purely emulated x86 QEMU on a s390 host, we don't need these changes. Thus they need to be conditional based on def->os.arch
@@ -1370,8 +1379,12 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk, disk->info.addr.drive.unit); break; case VIR_DOMAIN_DISK_BUS_VIRTIO: +# if ( defined __s390x__ || defined __s390__ ) + virBufferAddLit(&opt, "virtio-blk-s390"); +# else virBufferAddLit(&opt, "virtio-blk-pci"); qemuBuildDeviceAddressStr(&opt, &disk->info); +# endif
This is expected change, since s390 isn't PCI based, but what addressing scheme is used for s390 devices to uniquely and stablly identify them. eg, we need to make sure that qemu -drive AAA -drive BBB -drive CCC (monitor) drive_del BBB results except same guest visible ABI as qemu -drive AAA -drive CCC Without using qemuBuildDeviceAddressStr, then 'CCC' in the second example would likely appear as 'BBB' from the first example which is bad for the guest. Hence we need to assign stable addresses for s390 somehow, even though they'll be a different scheme than PCI. The same comment appears in all the other places where you add in a '-s390' variant, instead of '-pci', so I won't repeat this in
break; case VIR_DOMAIN_DISK_BUS_USB: virBufferAddLit(&opt, "usb-storage"); @@ -1476,6 +1489,9 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) break;
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: +# if ( defined __s390x__ || defined __s390__ ) + virBufferAddLit(&buf, "virtio-serial-s390"); +# else if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virBufferAddLit(&buf, "virtio-serial-pci"); } else { @@ -1491,6 +1507,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",vectors=%d", def->opts.vioserial.vectors); } +# endif
This is commenting out faaaaar too much code. All we need todo is replace '-pci' with '-s390' and fix the addressing scheme. vectors & max_ports should not be removed.
qemuBuildControllerDevStr(virDomainControllerDefPtr def) goto error; }
+# if ! ( defined __s390x__ || defined __s390__ ) if (qemuBuildDeviceAddressStr(&buf, &def->info) < 0) goto error;
@@ -1506,6 +1524,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virReportOOMError(); goto error; } +# endif
This is also removing too much code - you've disabled the error checking & reporting.
@@ -1551,7 +1570,11 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, if (!net->model) { nic = "rtl8139"; } else if (STREQ(net->model, "virtio")) { +# if ( defined __s390x__ || defined __s390__ ) + nic = "virtio-net-s390"; +# else nic = "virtio-net-pci"; +# endif
Fine, expected change
@@ -2587,7 +2610,11 @@ qemuBuildCommandLine(virConnectPtr conn, break; }
- cmd = virCommandNewArgList(emulator, "-S", NULL); +# if ! ( defined __s390x__ || defined __s390__ ) + cmd = virCommandNewArgList(emulator, "-S", NULL); // param '-S' makes problems under s390x +# else + cmd = virCommandNewArgList(emulator, "", NULL); +# endif
What sort of problems??? We can't simply remove the '-S' arg. that is *mandatory* for libvirt to be able todo initialization of various aspects of QEMU without race conditions & security holes.
@@ -2909,8 +2936,10 @@ qemuBuildCommandLine(virConnectPtr conn, def->onReboot != VIR_DOMAIN_LIFECYCLE_RESTART) virCommandAddArg(cmd, "-no-reboot");
+# if ! ( defined __s390x__ || defined __s390__ ) if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) virCommandAddArg(cmd, "-no-acpi"); +# endif
Fine, acpi isn't relevant for s390.
if (!def->os.bootloader) { for (i = 0 ; i < def->os.nBootDevs ; i++) { @@ -2980,6 +3009,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
+# if ! ( defined __s390x__ || defined __s390__ ) if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { virDomainControllerDefPtr cont = def->controllers[i]; @@ -3008,6 +3038,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); } } +# endif
I can't see that this is correct. What problem are you actually trying to solve
/* If QEMU supports -drive param instead of old -hda, -hdb, -cdrom .. */ if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) { @@ -3106,6 +3137,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
+# if ! ( defined __s390x__ || defined __s390__ ) // s390x doesn't need DeviceArg yet (it doesn't even work with ...) if (withDeviceArg) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { virCommandAddArg(cmd, "-global"); @@ -3131,6 +3163,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(optstr); } } +# endif } } else { for (i = 0 ; i < def->ndisks ; i++) { @@ -3277,11 +3310,13 @@ qemuBuildCommandLine(virConnectPtr conn, char vhostfd_name[50] = ""; int vlan;
+# if ! ( defined __s390x__ || defined __s390__ ) /* VLANs are not used with -netdev, so don't record them */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) vlan = -1; else +# endif
Looks wrong too. If QEMU has declared support for -netdev and -device we should be using it regardless of arch.
vlan = i;
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || @@ -3338,6 +3373,7 @@ qemuBuildCommandLine(virConnectPtr conn, * * NB, no support for -netdev without use of -device */ +# if ! ( defined __s390x__ || defined __s390__ ) // Old way works on IBM z9 Tuebingen; maybe there are other z9's which also work with "semi-" or "best way" if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { virCommandAddArg(cmd, "-netdev"); @@ -3354,21 +3390,26 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, nic); VIR_FREE(nic); } else { +# endif
Same comment as above.
virCommandAddArg(cmd, "-net"); if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) goto error; virCommandAddArg(cmd, nic); VIR_FREE(nic); +# if ! ( defined __s390x__ || defined __s390__ ) } if (!((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))) { +# endif virCommandAddArg(cmd, "-net"); if (!(host = qemuBuildHostNetStr(net, ',', vlan, tapfd_name, vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); +# if ! ( defined __s390x__ || defined __s390__ ) } +# endif
Same comment as above.
} }
@@ -3384,6 +3425,7 @@ qemuBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { +# if ! ( defined __s390x__ || defined __s390__ ) virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&serial->source, serial->info.alias))) @@ -3394,6 +3436,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "isa-serial,chardev=%s", serial->info.alias); +# endif;
This can't be correct.
} else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -3483,6 +3526,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr);
+# if ( defined __s390x__ || defined __s390__ ) + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, "virtio-serial-s390"); +# endif
This looks similarly dubious
virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel))) goto error; @@ -3512,6 +3560,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr);
+# if ( defined __s390x__ || defined __s390__ ) + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, "virtio-serial-s390"); +# endif + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(console))) goto error;
As does this.
@@ -4011,6 +4064,8 @@ qemuBuildCommandLine(virConnectPtr conn, * NB: Earlier we declared that VirtIO balloon will always be in * slot 0x3 on bus 0x0 */ +// our SUSE Enterprise, running on one LPAR (IBM z9), doesn't have ballon support ... +# if ! ( defined __s390x__ || defined __s390__ ) if ((def->memballoon) && (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) { if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) { @@ -4032,6 +4087,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-balloon", "virtio", NULL); } } +# endif
If balloon device isn't required, this should simply be indicated in the XML, rather than hacking the code. The changes to count host CPUs seem fine, and using '-s390' instead of '-pci' for virtio device names is OK, but the rest all looks wrong to me. It is disabling features that we've already detected that QEMU suports. Regards, Daniel
Am 25.01.2011 18:23, schrieb Daniel P. Berrange:
Hello there,
I don't know whether or not you are only interested in the x86 / x86_64 architecture, but I think you could also be interested in the small bit of code which I have programmed. Since the release of libvirt 0.8.5, there is a small bit of s390x architecture code (one line) within libvirt which has given me the impression that you could be interested in my small bit of code. Most of the developers only have access to x86, so s390x hasn't been a priority. We're happy to accept patches from anyone who is in a
On Mon, Jan 24, 2011 at 09:22:04PM +0100, Patrick Siegl wrote: position to actually test s390 functionality.
Good to here :-)
... and here is the patch: =============================================================== diff --git a/libvirt-git/src/qemu/qemu_capabilities.c b/libvirt/src/qemu/qemu_capabilities.c index faf7d44..7bbb1d5 100644 --- a/libvirt-git/src/qemu/qemu_capabilities.c +++ b/libvirt/src/qemu/qemu_capabilities.c @@ -83,7 +83,7 @@ static const struct qemu_arch_info const arch_info_hvm[] = { { "sparc", 32, NULL, "qemu-system-sparc", NULL, NULL, 0 }, { "ppc", 32, NULL, "qemu-system-ppc", NULL, NULL, 0 }, { "itanium", 64, NULL, "qemu-system-ia64", NULL, NULL, 0 }, - { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, + { "s390x", 64, NULL, "qemu-system-s390x", NULL, NULL, 0 }, // Since 0.8.5: doesn't work on it's own! Not sure this comment serves any purpose.
I know that, it was only a comment for myself.
diff --git a/libvirt-git/src/nodeinfo.c b/libvirt/src/nodeinfo.c index 22d53e5..c9059d4 100644 --- a/libvirt-git/src/nodeinfo.c +++ b/libvirt/src/nodeinfo.c @@ -164,7 +164,11 @@ cleanup:
static int parse_socket(unsigned int cpu) { +# if ( defined __s390x__ || defined __s390__ ) + return 0; /* s390 does not implement physical_package_id by now */ +# else return get_cpu_value(cpu, "topology/physical_package_id", false); +# endif } Looks fine.
int linuxNodeInfoCPUPopulate(FILE *cpuinfo, @@ -195,6 +199,30 @@ int linuxNodeInfoCPUPopulate(FILE *cpuinfo, * has no knowledge of NUMA nodes */
/* NOTE: hyperthreads are ignored here; they are parsed out of /sys */ +# if defined __s390x__ || defined __s390__ + while (fgets(line, sizeof(line), cpuinfo) != NULL) { + char *buf = line; + if (STRPREFIX(buf, "# processors")) { /* Number of processors */ + char *p; + unsigned int id; + buf += 12; + while (*buf && c_isspace(*buf)) + buf++; + if (*buf != ':' || !buf[1]) { + nodeReportError(VIR_ERR_INTERNAL_ERROR, + "parsing cpuinfo cpu count %c", *buf); + return -1; + } + if (virStrToLong_ui(buf+1, &p, 10, &id) == 0 + && (*p == '\0' || c_isspace(*p)) + && id > nodeinfo->cores) { + nodeinfo->cpus = id; + nodeinfo->cores = 1; + nodeinfo->mhz = 1400; /* z9 BC */ + } + } + } +# else /* no s390 */
Be nice if mhz wasn't hardcoded and could be got from sysfs or procfs, but not super critical since I doubt anyone really cares about this value.
Will do, but first of all it should be working on s390x.
diff --git a/libvirt-git/src/qemu/qemu_command.c b/libvirt/src/qemu/qemu_command.c index c20f031..0419a0c 100644 --- a/libvirt-git/src/qemu/qemu_command.c +++ b/libvirt/src/qemu/qemu_command.c @@ -1267,9 +1267,15 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; } } else { +# if ( defined __s390x__ || defined __s390__ ) + virBufferVSprintf(&opt, "file=%s", disk->src); +# else virBufferVSprintf(&opt, "file=%s,", disk->src); +# endif } } +// makes problems under s390x +# if ! ( defined __s390x__ || defined __s390__ ) if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) virBufferAddLit(&opt, "if=none"); else @@ -1291,6 +1297,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferVSprintf(&opt, ",unit=%d", unitid); } } +# endif if (bootable && disk->device == VIR_DOMAIN_DISK_DEVICE_DISK && disk->bus != VIR_DOMAIN_DISK_BUS_IDE) @@ -1298,10 +1305,12 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, if (disk->readonly && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_READONLY) virBufferAddLit(&opt, ",readonly=on"); +# if ! ( defined __s390x__ || defined __s390__ ) if (disk->driverType && *disk->driverType != '\0' && disk->type != VIR_DOMAIN_DISK_TYPE_DIR && qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_FORMAT) virBufferVSprintf(&opt, ",format=%s", disk->driverType); +# endif if (disk->serial && (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE_SERIAL)) { if (qemuSafeSerialParamValue(disk->serial) < 0) These changes are seriously dubious. The QEMU command line syntax is architecture independant, so this should not be changed.
In addition *any* use of '#ifdef s390' in qemu_command.c is likely wrong. That is changing the behaviour based on what host libvirt is compiled for. We need to change behaviour based on what architecture the guest is emulating.
ie, if we run a purely emulated s390 QEMU on an x86 host, we need these changes. If we ran a purely emulated x86 QEMU on a s390 host, we don't need these changes.
Thus they need to be conditional based on def->os.arch
@@ -1370,8 +1379,12 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk, disk->info.addr.drive.unit); break; case VIR_DOMAIN_DISK_BUS_VIRTIO: +# if ( defined __s390x__ || defined __s390__ ) + virBufferAddLit(&opt, "virtio-blk-s390"); +# else virBufferAddLit(&opt, "virtio-blk-pci"); qemuBuildDeviceAddressStr(&opt, &disk->info); +# endif This is expected change, since s390 isn't PCI based, but what addressing scheme is used for s390 devices to uniquely and stablly identify them.
eg, we need to make sure that
qemu -drive AAA -drive BBB -drive CCC (monitor) drive_del BBB
results except same guest visible ABI as
qemu -drive AAA -drive CCC
Without using qemuBuildDeviceAddressStr, then 'CCC' in the second example would likely appear as 'BBB' from the first example which is bad for the guest. Hence we need to assign stable addresses for s390 somehow, even though they'll be a different scheme than PCI.
The same comment appears in all the other places where you add in a '-s390' variant, instead of '-pci', so I won't repeat this in
I'm going to try my best to get this running ...
break; case VIR_DOMAIN_DISK_BUS_USB: virBufferAddLit(&opt, "usb-storage"); @@ -1476,6 +1489,9 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) break;
case VIR_DOMAIN_CONTROLLER_TYPE_VIRTIO_SERIAL: +# if ( defined __s390x__ || defined __s390__ ) + virBufferAddLit(&buf, "virtio-serial-s390"); +# else if (def->info.type == VIR_DOMAIN_DEVICE_ADDRESS_TYPE_PCI) { virBufferAddLit(&buf, "virtio-serial-pci"); } else { @@ -1491,6 +1507,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virBufferVSprintf(&buf, ",vectors=%d", def->opts.vioserial.vectors); } +# endif
This is commenting out faaaaar too much code. All we need todo is replace '-pci' with '-s390' and fix the addressing scheme. vectors & max_ports should not be removed.
ok, accepted.
qemuBuildControllerDevStr(virDomainControllerDefPtr def) goto error; }
+# if ! ( defined __s390x__ || defined __s390__ ) if (qemuBuildDeviceAddressStr(&buf, &def->info) < 0) goto error;
@@ -1506,6 +1524,7 @@ qemuBuildControllerDevStr(virDomainControllerDefPtr def) virReportOOMError(); goto error; } +# endif This is also removing too much code - you've disabled the error checking & reporting.
also accepted.
@@ -1551,7 +1570,11 @@ qemuBuildNicDevStr(virDomainNetDefPtr net, if (!net->model) { nic = "rtl8139"; } else if (STREQ(net->model, "virtio")) { +# if ( defined __s390x__ || defined __s390__ ) + nic = "virtio-net-s390"; +# else nic = "virtio-net-pci"; +# endif Fine, expected change
@@ -2587,7 +2610,11 @@ qemuBuildCommandLine(virConnectPtr conn, break; }
- cmd = virCommandNewArgList(emulator, "-S", NULL); +# if ! ( defined __s390x__ || defined __s390__ ) + cmd = virCommandNewArgList(emulator, "-S", NULL); // param '-S' makes problems under s390x +# else + cmd = virCommandNewArgList(emulator, "", NULL); +# endif What sort of problems??? We can't simply remove the '-S' arg. that is *mandatory* for libvirt to be able todo initialization of various aspects of QEMU without race conditions & security holes.
When running a virtual machine under s390x with KVM adding the param '-S' to a commandline the following appears in VNC: small blue box with content 'cons console' in it. Underneath there is a grey cursor (not flashing, only static) and the rest of the image is black. I've got no virtual machine running with param '-S' on s390x. (Look at the screenshot attachment.)
@@ -2909,8 +2936,10 @@ qemuBuildCommandLine(virConnectPtr conn, def->onReboot != VIR_DOMAIN_LIFECYCLE_RESTART) virCommandAddArg(cmd, "-no-reboot");
+# if ! ( defined __s390x__ || defined __s390__ ) if (!(def->features & (1 << VIR_DOMAIN_FEATURE_ACPI))) virCommandAddArg(cmd, "-no-acpi"); +# endif Fine, acpi isn't relevant for s390.
if (!def->os.bootloader) { for (i = 0 ; i < def->os.nBootDevs ; i++) { @@ -2980,6 +3009,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
+# if ! ( defined __s390x__ || defined __s390__ ) if (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE) { for (i = 0 ; i < def->ncontrollers ; i++) { virDomainControllerDefPtr cont = def->controllers[i]; @@ -3008,6 +3038,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(devstr); } } +# endif
I can't see that this is correct. What problem are you actually trying to solve
/* If QEMU supports -drive param instead of old -hda, -hdb, -cdrom .. */ if (qemuCmdFlags & QEMUD_CMD_FLAG_DRIVE) { @@ -3106,6 +3137,7 @@ qemuBuildCommandLine(virConnectPtr conn, } }
+# if ! ( defined __s390x__ || defined __s390__ ) // s390x doesn't need DeviceArg yet (it doesn't even work with ...) if (withDeviceArg) { if (disk->bus == VIR_DOMAIN_DISK_BUS_FDC) { virCommandAddArg(cmd, "-global"); @@ -3131,6 +3163,7 @@ qemuBuildCommandLine(virConnectPtr conn, VIR_FREE(optstr); } } +# endif } } else { for (i = 0 ; i < def->ndisks ; i++) { @@ -3277,11 +3310,13 @@ qemuBuildCommandLine(virConnectPtr conn, char vhostfd_name[50] = ""; int vlan;
+# if ! ( defined __s390x__ || defined __s390__ ) /* VLANs are not used with -netdev, so don't record them */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) vlan = -1; else +# endif
Looks wrong too. If QEMU has declared support for -netdev and -device we should be using it regardless of arch.
vlan = i;
if (net->type == VIR_DOMAIN_NET_TYPE_NETWORK || @@ -3338,6 +3373,7 @@ qemuBuildCommandLine(virConnectPtr conn, * * NB, no support for -netdev without use of -device */ +# if ! ( defined __s390x__ || defined __s390__ ) // Old way works on IBM z9 Tuebingen; maybe there are other z9's which also work with "semi-" or "best way" if ((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { virCommandAddArg(cmd, "-netdev"); @@ -3354,21 +3390,26 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, nic); VIR_FREE(nic); } else { +# endif
Same comment as above.
virCommandAddArg(cmd, "-net"); if (!(nic = qemuBuildNicStr(net, "nic,", vlan))) goto error; virCommandAddArg(cmd, nic); VIR_FREE(nic); +# if ! ( defined __s390x__ || defined __s390__ ) } if (!((qemuCmdFlags & QEMUD_CMD_FLAG_NETDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE))) { +# endif virCommandAddArg(cmd, "-net"); if (!(host = qemuBuildHostNetStr(net, ',', vlan, tapfd_name, vhostfd_name))) goto error; virCommandAddArg(cmd, host); VIR_FREE(host); +# if ! ( defined __s390x__ || defined __s390__ ) } +# endif
Same comment as above.
} }
@@ -3384,6 +3425,7 @@ qemuBuildCommandLine(virConnectPtr conn, /* Use -chardev with -device if they are available */ if ((qemuCmdFlags & QEMUD_CMD_FLAG_CHARDEV) && (qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) { +# if ! ( defined __s390x__ || defined __s390__ ) virCommandAddArg(cmd, "-chardev"); if (!(devstr = qemuBuildChrChardevStr(&serial->source, serial->info.alias))) @@ -3394,6 +3436,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-device"); virCommandAddArgFormat(cmd, "isa-serial,chardev=%s", serial->info.alias); +# endif;
This can't be correct.
} else { virCommandAddArg(cmd, "-serial"); if (!(devstr = qemuBuildChrArgStr(&serial->source, NULL))) @@ -3483,6 +3526,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr);
+# if ( defined __s390x__ || defined __s390__ ) + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, "virtio-serial-s390"); +# endif
This looks similarly dubious
virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(channel))) goto error; @@ -3512,6 +3560,11 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, devstr); VIR_FREE(devstr);
+# if ( defined __s390x__ || defined __s390__ ) + virCommandAddArg(cmd, "-device"); + virCommandAddArg(cmd, "virtio-serial-s390"); +# endif + virCommandAddArg(cmd, "-device"); if (!(devstr = qemuBuildVirtioSerialPortDevStr(console))) goto error;
As does this.
@@ -4011,6 +4064,8 @@ qemuBuildCommandLine(virConnectPtr conn, * NB: Earlier we declared that VirtIO balloon will always be in * slot 0x3 on bus 0x0 */ +// our SUSE Enterprise, running on one LPAR (IBM z9), doesn't have ballon support ... +# if ! ( defined __s390x__ || defined __s390__ ) if ((def->memballoon) && (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_NONE)) { if (def->memballoon->model != VIR_DOMAIN_MEMBALLOON_MODEL_VIRTIO) { @@ -4032,6 +4087,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArgList(cmd, "-balloon", "virtio", NULL); } } +# endif If balloon device isn't required, this should simply be indicated in the XML, rather than hacking the code.
I have now seen, that there is way to disable this :-)
The changes to count host CPUs seem fine, and using '-s390' instead of '-pci' for virtio device names is OK, but the rest all looks wrong to me. It is disabling features that we've already detected that QEMU suports.
Regards, Daniel I will take care of the 'qemu_command.c' file which I have fixed quick and dirty so that you will accept it next time. :-)
Regards, Patrick Siegl
participants (2)
-
Daniel P. Berrange -
Patrick Siegl