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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list