Am 25.01.2011 18:23, schrieb Daniel P. Berrange:
> 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.
>
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
Attachment:
screenshot_paramS.png
Description: PNG image
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list