[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]