Re: [libvirt] Rudimentary (basic) s390x architecture functions for libvirt

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