[Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Thomas Huth]

The consoles ("sclpconsole" and "sclplmconsole") can only be configured
with "-device" and "-chardev" so far. Other machines use the convenience
option "-serial" to configure the default consoles, too, even for virtual
consoles like spapr-vty on the pseries machine. So let's support this
option on s390x, too, so we can easily enable the serial console here
again with "-nodefaults", for example. Also map the second -serial
option to the "sclplmconsole", so that there is now an easy way to
configure this second console on s390x, too.
Additionally, the new code is also smaller than the old one and we have
less s390x-specific code in vl.c :-)

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/event-facility.c         | 14 +++++++++++
 hw/s390x/s390-virtio-ccw.c        | 19 +++++++++++++--
 include/hw/boards.h               |  1 -
 include/hw/s390x/event-facility.h |  2 ++
 vl.c                              | 50 ---------------------------------------
 5 files changed, 33 insertions(+), 53 deletions(-)

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 9c24bc6..e6940a2 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -511,3 +511,17 @@ static void register_types(void)
 }
 
 type_init(register_types)
+
+BusState *sclp_get_event_facility_bus(void)
+{
+    Object *busobj;
+    SCLPEventsBus *sbus;
+
+    busobj = object_resolve_path_type("", TYPE_SCLP_EVENTS_BUS, NULL);
+    sbus = OBJECT_CHECK(SCLPEventsBus, busobj, TYPE_SCLP_EVENTS_BUS);
+    if (!sbus) {
+        return NULL;
+    }
+
+    return &sbus->qbus;
+}
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 435f7c9..fe28514 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -288,6 +288,15 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
     }
 }
 
+static void s390_create_sclpconsole(const char *type, Chardev *chardev)
+{
+    DeviceState *dev;
+
+    dev = qdev_create(sclp_get_event_facility_bus(), type);
+    qdev_prop_set_chr(dev, "chardev", chardev);
+    qdev_init_nofail(dev);
+}
+
 static void ccw_init(MachineState *machine)
 {
     int ret;
@@ -311,6 +320,14 @@ static void ccw_init(MachineState *machine)
                       machine->initrd_filename, "s390-ccw.img",
                       "s390-netboot.img", true);
 
+    /* init consoles */
+    if (serial_hds[0]) {
+        s390_create_sclpconsole("sclpconsole", serial_hds[0]);
+    }
+    if (serial_hds[1]) {
+        s390_create_sclpconsole("sclplmconsole", serial_hds[1]);
+    }
+
     /*
      * We cannot easily make the pci host bridge conditional as older QEMUs
      * always created it. Doing so would break migration across QEMU versions.
@@ -470,10 +487,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->no_floppy = 1;
-    mc->no_serial = 1;
     mc->no_parallel = 1;
     mc->no_sdcard = 1;
-    mc->use_sclp = 1;
     mc->max_cpus = S390_MAX_CPUS;
     mc->has_hotpluggable_cpus = true;
     mc->get_hotplug_handler = s390_get_hotplug_handler;
diff --git a/include/hw/boards.h b/include/hw/boards.h
index a609239..5c5eee5 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -180,7 +180,6 @@ struct MachineClass {
     unsigned int no_serial:1,
         no_parallel:1,
         use_virtcon:1,
-        use_sclp:1,
         no_floppy:1,
         no_cdrom:1,
         no_sdcard:1,
diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
index 5698e5e..5cc16f6 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -210,4 +210,6 @@ typedef struct SCLPEventFacilityClass {
     bool (*event_pending)(SCLPEventFacility *ef);
 } SCLPEventFacilityClass;
 
+BusState *sclp_get_event_facility_bus(void);
+
 #endif
diff --git a/vl.c b/vl.c
index fce1fd1..b32340c 100644
--- a/vl.c
+++ b/vl.c
@@ -133,7 +133,6 @@ int main(int argc, char **argv)
 #include "sysemu/iothread.h"
 
 #define MAX_VIRTIO_CONSOLES 1
-#define MAX_SCLP_CONSOLES 1
 
 static const char *data_dir[16];
 static int data_dir_idx;
@@ -157,7 +156,6 @@ int no_frame;
 Chardev *serial_hds[MAX_SERIAL_PORTS];
 Chardev *parallel_hds[MAX_PARALLEL_PORTS];
 Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES];
-Chardev *sclp_hds[MAX_SCLP_CONSOLES];
 int win2k_install_hack = 0;
 int singlestep = 0;
 int smp_cpus;
@@ -209,7 +207,6 @@ static int has_defaults = 1;
 static int default_serial = 1;
 static int default_parallel = 1;
 static int default_virtcon = 1;
-static int default_sclp = 1;
 static int default_monitor = 1;
 static int default_floppy = 1;
 static int default_cdrom = 1;
@@ -2571,39 +2568,6 @@ static int virtcon_parse(const char *devname)
     return 0;
 }
 
-static int sclp_parse(const char *devname)
-{
-    QemuOptsList *device = qemu_find_opts("device");
-    static int index = 0;
-    char label[32];
-    QemuOpts *dev_opts;
-
-    if (strcmp(devname, "none") == 0) {
-        return 0;
-    }
-    if (index == MAX_SCLP_CONSOLES) {
-        error_report("too many sclp consoles");
-        exit(1);
-    }
-
-    assert(arch_type == QEMU_ARCH_S390X);
-
-    dev_opts = qemu_opts_create(device, NULL, 0, NULL);
-    qemu_opt_set(dev_opts, "driver", "sclpconsole", &error_abort);
-
-    snprintf(label, sizeof(label), "sclpcon%d", index);
-    sclp_hds[index] = qemu_chr_new(label, devname);
-    if (!sclp_hds[index]) {
-        error_report("could not connect sclp console"
-                     " to character backend '%s'", devname);
-        return -1;
-    }
-    qemu_opt_set(dev_opts, "chardev", label, &error_abort);
-
-    index++;
-    return 0;
-}
-
 static int debugcon_parse(const char *devname)
 {
     QemuOpts *opts;
@@ -4237,9 +4201,6 @@ int main(int argc, char **argv, char **envp)
     if (!has_defaults || !machine_class->use_virtcon) {
         default_virtcon = 0;
     }
-    if (!has_defaults || !machine_class->use_sclp) {
-        default_sclp = 0;
-    }
     if (!has_defaults || machine_class->no_floppy) {
         default_floppy = 0;
     }
@@ -4286,16 +4247,11 @@ int main(int argc, char **argv, char **envp)
             add_device_config(DEV_SERIAL, "mon:stdio");
         } else if (default_virtcon && default_monitor) {
             add_device_config(DEV_VIRTCON, "mon:stdio");
-        } else if (default_sclp && default_monitor) {
-            add_device_config(DEV_SCLP, "mon:stdio");
         } else {
             if (default_serial)
                 add_device_config(DEV_SERIAL, "stdio");
             if (default_virtcon)
                 add_device_config(DEV_VIRTCON, "stdio");
-            if (default_sclp) {
-                add_device_config(DEV_SCLP, "stdio");
-            }
             if (default_monitor)
                 monitor_parse("stdio", "readline", false);
         }
@@ -4308,9 +4264,6 @@ int main(int argc, char **argv, char **envp)
             monitor_parse("vc:80Cx24C", "readline", false);
         if (default_virtcon)
             add_device_config(DEV_VIRTCON, "vc:80Cx24C");
-        if (default_sclp) {
-            add_device_config(DEV_SCLP, "vc:80Cx24C");
-        }
     }
 
 #if defined(CONFIG_VNC)
@@ -4560,9 +4513,6 @@ int main(int argc, char **argv, char **envp)
         exit(1);
     if (foreach_device_config(DEV_VIRTCON, virtcon_parse) < 0)
         exit(1);
-    if (foreach_device_config(DEV_SCLP, sclp_parse) < 0) {
-        exit(1);
-    }
     if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
         exit(1);
 
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Christian Borntraeger]

On 04/24/2018 01:44 PM, Thomas Huth wrote:
> The consoles ("sclpconsole" and "sclplmconsole") can only be configured
> with "-device" and "-chardev" so far. Other machines use the convenience
> option "-serial" to configure the default consoles, too, even for virtual
> consoles like spapr-vty on the pseries machine. So let's support this
> option on s390x, too, so we can easily enable the serial console here
> again with "-nodefaults", for example. Also map the second -serial
> option to the "sclplmconsole", so that there is now an easy way to
> configure this second console on s390x, too.
> Additionally, the new code is also smaller than the old one and we have
> less s390x-specific code in vl.c :-)

Can you show some new example command lines?

I read that as "the current command lines continue to work". Correct?
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/event-facility.c         | 14 +++++++++++
>  hw/s390x/s390-virtio-ccw.c        | 19 +++++++++++++--
>  include/hw/boards.h               |  1 -
>  include/hw/s390x/event-facility.h |  2 ++
>  vl.c                              | 50 ---------------------------------------
>  5 files changed, 33 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 9c24bc6..e6940a2 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -511,3 +511,17 @@ static void register_types(void)
>  }
>  
>  type_init(register_types)
> +
> +BusState *sclp_get_event_facility_bus(void)
> +{
> +    Object *busobj;
> +    SCLPEventsBus *sbus;
> +
> +    busobj = object_resolve_path_type("", TYPE_SCLP_EVENTS_BUS, NULL);
> +    sbus = OBJECT_CHECK(SCLPEventsBus, busobj, TYPE_SCLP_EVENTS_BUS);
> +    if (!sbus) {
> +        return NULL;
> +    }
> +
> +    return &sbus->qbus;
> +}
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 435f7c9..fe28514 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -288,6 +288,15 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>      }
>  }
>  
> +static void s390_create_sclpconsole(const char *type, Chardev *chardev)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(sclp_get_event_facility_bus(), type);
> +    qdev_prop_set_chr(dev, "chardev", chardev);
> +    qdev_init_nofail(dev);
> +}
> +
>  static void ccw_init(MachineState *machine)
>  {
>      int ret;
> @@ -311,6 +320,14 @@ static void ccw_init(MachineState *machine)
>                        machine->initrd_filename, "s390-ccw.img",
>                        "s390-netboot.img", true);
>  
> +    /* init consoles */
> +    if (serial_hds[0]) {
> +        s390_create_sclpconsole("sclpconsole", serial_hds[0]);
> +    }
> +    if (serial_hds[1]) {
> +        s390_create_sclpconsole("sclplmconsole", serial_hds[1]);
> +    }
> +
>      /*
>       * We cannot easily make the pci host bridge conditional as older QEMUs
>       * always created it. Doing so would break migration across QEMU versions.
> @@ -470,10 +487,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
>      mc->no_floppy = 1;
> -    mc->no_serial = 1;
>      mc->no_parallel = 1;
>      mc->no_sdcard = 1;
> -    mc->use_sclp = 1;
>      mc->max_cpus = S390_MAX_CPUS;
>      mc->has_hotpluggable_cpus = true;
>      mc->get_hotplug_handler = s390_get_hotplug_handler;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a609239..5c5eee5 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -180,7 +180,6 @@ struct MachineClass {
>      unsigned int no_serial:1,
>          no_parallel:1,
>          use_virtcon:1,
> -        use_sclp:1,
>          no_floppy:1,
>          no_cdrom:1,
>          no_sdcard:1,
> diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> index 5698e5e..5cc16f6 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -210,4 +210,6 @@ typedef struct SCLPEventFacilityClass {
>      bool (*event_pending)(SCLPEventFacility *ef);
>  } SCLPEventFacilityClass;
>  
> +BusState *sclp_get_event_facility_bus(void);
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index fce1fd1..b32340c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -133,7 +133,6 @@ int main(int argc, char **argv)
>  #include "sysemu/iothread.h"
>  
>  #define MAX_VIRTIO_CONSOLES 1
> -#define MAX_SCLP_CONSOLES 1
>  
>  static const char *data_dir[16];
>  static int data_dir_idx;
> @@ -157,7 +156,6 @@ int no_frame;
>  Chardev *serial_hds[MAX_SERIAL_PORTS];
>  Chardev *parallel_hds[MAX_PARALLEL_PORTS];
>  Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES];
> -Chardev *sclp_hds[MAX_SCLP_CONSOLES];
>  int win2k_install_hack = 0;
>  int singlestep = 0;
>  int smp_cpus;
> @@ -209,7 +207,6 @@ static int has_defaults = 1;
>  static int default_serial = 1;
>  static int default_parallel = 1;
>  static int default_virtcon = 1;
> -static int default_sclp = 1;
>  static int default_monitor = 1;
>  static int default_floppy = 1;
>  static int default_cdrom = 1;
> @@ -2571,39 +2568,6 @@ static int virtcon_parse(const char *devname)
>      return 0;
>  }
>  
> -static int sclp_parse(const char *devname)
> -{
> -    QemuOptsList *device = qemu_find_opts("device");
> -    static int index = 0;
> -    char label[32];
> -    QemuOpts *dev_opts;
> -
> -    if (strcmp(devname, "none") == 0) {
> -        return 0;
> -    }
> -    if (index == MAX_SCLP_CONSOLES) {
> -        error_report("too many sclp consoles");
> -        exit(1);
> -    }
> -
> -    assert(arch_type == QEMU_ARCH_S390X);
> -
> -    dev_opts = qemu_opts_create(device, NULL, 0, NULL);
> -    qemu_opt_set(dev_opts, "driver", "sclpconsole", &error_abort);
> -
> -    snprintf(label, sizeof(label), "sclpcon%d", index);
> -    sclp_hds[index] = qemu_chr_new(label, devname);
> -    if (!sclp_hds[index]) {
> -        error_report("could not connect sclp console"
> -                     " to character backend '%s'", devname);
> -        return -1;
> -    }
> -    qemu_opt_set(dev_opts, "chardev", label, &error_abort);
> -
> -    index++;
> -    return 0;
> -}
> -
>  static int debugcon_parse(const char *devname)
>  {
>      QemuOpts *opts;
> @@ -4237,9 +4201,6 @@ int main(int argc, char **argv, char **envp)
>      if (!has_defaults || !machine_class->use_virtcon) {
>          default_virtcon = 0;
>      }
> -    if (!has_defaults || !machine_class->use_sclp) {
> -        default_sclp = 0;
> -    }
>      if (!has_defaults || machine_class->no_floppy) {
>          default_floppy = 0;
>      }
> @@ -4286,16 +4247,11 @@ int main(int argc, char **argv, char **envp)
>              add_device_config(DEV_SERIAL, "mon:stdio");
>          } else if (default_virtcon && default_monitor) {
>              add_device_config(DEV_VIRTCON, "mon:stdio");
> -        } else if (default_sclp && default_monitor) {
> -            add_device_config(DEV_SCLP, "mon:stdio");
>          } else {
>              if (default_serial)
>                  add_device_config(DEV_SERIAL, "stdio");
>              if (default_virtcon)
>                  add_device_config(DEV_VIRTCON, "stdio");
> -            if (default_sclp) {
> -                add_device_config(DEV_SCLP, "stdio");
> -            }
>              if (default_monitor)
>                  monitor_parse("stdio", "readline", false);
>          }
> @@ -4308,9 +4264,6 @@ int main(int argc, char **argv, char **envp)
>              monitor_parse("vc:80Cx24C", "readline", false);
>          if (default_virtcon)
>              add_device_config(DEV_VIRTCON, "vc:80Cx24C");
> -        if (default_sclp) {
> -            add_device_config(DEV_SCLP, "vc:80Cx24C");
> -        }
>      }
>  
>  #if defined(CONFIG_VNC)
> @@ -4560,9 +4513,6 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      if (foreach_device_config(DEV_VIRTCON, virtcon_parse) < 0)
>          exit(1);
> -    if (foreach_device_config(DEV_SCLP, sclp_parse) < 0) {
> -        exit(1);
> -    }
>      if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
>          exit(1);
>  
> 

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Thomas Huth]

On 24.04.2018 13:49, Christian Borntraeger wrote:
> 
> 
> On 04/24/2018 01:44 PM, Thomas Huth wrote:
>> The consoles ("sclpconsole" and "sclplmconsole") can only be configured
>> with "-device" and "-chardev" so far. Other machines use the convenience
>> option "-serial" to configure the default consoles, too, even for virtual
>> consoles like spapr-vty on the pseries machine. So let's support this
>> option on s390x, too, so we can easily enable the serial console here
>> again with "-nodefaults", for example. Also map the second -serial
>> option to the "sclplmconsole", so that there is now an easy way to
>> configure this second console on s390x, too.
>> Additionally, the new code is also smaller than the old one and we have
>> less s390x-specific code in vl.c :-)
> 
> Can you show some new example command lines?

Sure. I'm mainly using this together with nodefaults:

qemu-system-s390x -no-shutdown -nographic -nodefaults -serial mon:stdio

That's way easier than typing:

qemu-system-s390x -no-shutdown -nographic -nodefaults \
  -chardev stdio,id=c1,mux=on -device sclpconsole,chardev=c1 \
  -mon chardev=c1

Another example: You only want to see the QEMU monitor on stdio, but not
the serial output, without using -nodefaults (i.e. you still want to
have the other default devices). AFAIK that's pretty impossible with the
current code. But once you've got this patch applied, you can do:

qemu-system-s390x -no-shutdown -nographic -serial none

And to view the sclplm console, you can now simply do:

qemu-system-s390x -no-shutdown -nographic -serial null -serial mon:stdio

> I read that as "the current command lines continue to work". Correct?

Right. I was a little bit afraid that this might break migration, but I
gave it a quick check and it still seems to work fine here.
"info qom-tree" and "info qtree" at the HMP monitor show slightly
different output, though ... not sure whether that's critical or not?

 Thomas

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Cornelia Huck]

On Tue, 24 Apr 2018 16:02:59 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 24.04.2018 13:49, Christian Borntraeger wrote:
> > 
> > 
> > On 04/24/2018 01:44 PM, Thomas Huth wrote:  
> >> The consoles ("sclpconsole" and "sclplmconsole") can only be configured
> >> with "-device" and "-chardev" so far. Other machines use the convenience
> >> option "-serial" to configure the default consoles, too, even for virtual
> >> consoles like spapr-vty on the pseries machine. So let's support this
> >> option on s390x, too, so we can easily enable the serial console here
> >> again with "-nodefaults", for example. Also map the second -serial
> >> option to the "sclplmconsole", so that there is now an easy way to
> >> configure this second console on s390x, too.
> >> Additionally, the new code is also smaller than the old one and we have
> >> less s390x-specific code in vl.c :-)  
> > 
> > Can you show some new example command lines?  
> 
> Sure. I'm mainly using this together with nodefaults:
> 
> qemu-system-s390x -no-shutdown -nographic -nodefaults -serial mon:stdio
> 
> That's way easier than typing:
> 
> qemu-system-s390x -no-shutdown -nographic -nodefaults \
>   -chardev stdio,id=c1,mux=on -device sclpconsole,chardev=c1 \
>   -mon chardev=c1

What, you don't like typing complicated command lines? :)

> 
> Another example: You only want to see the QEMU monitor on stdio, but not
> the serial output, without using -nodefaults (i.e. you still want to
> have the other default devices). AFAIK that's pretty impossible with the
> current code. But once you've got this patch applied, you can do:
> 
> qemu-system-s390x -no-shutdown -nographic -serial none
> 
> And to view the sclplm console, you can now simply do:
> 
> qemu-system-s390x -no-shutdown -nographic -serial null -serial mon:stdio

Looks cool. Maybe add some of the examples to the patch description?

> 
> > I read that as "the current command lines continue to work". Correct?  
> 
> Right. I was a little bit afraid that this might break migration, but I
> gave it a quick check and it still seems to work fine here.
> "info qom-tree" and "info qtree" at the HMP monitor show slightly
> different output, though ... not sure whether that's critical or not?

What looks different? If we still send/expect the same kind of
information, it should not have any impact, I guess.

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Thomas Huth]

On 24.04.2018 16:09, Cornelia Huck wrote:
> On Tue, 24 Apr 2018 16:02:59 +0200
> Thomas Huth <thuth@redhat.com> wrote:
> 
>> On 24.04.2018 13:49, Christian Borntraeger wrote:
>>>
>>>
>>> On 04/24/2018 01:44 PM, Thomas Huth wrote:  
>>>> The consoles ("sclpconsole" and "sclplmconsole") can only be configured
>>>> with "-device" and "-chardev" so far. Other machines use the convenience
>>>> option "-serial" to configure the default consoles, too, even for virtual
>>>> consoles like spapr-vty on the pseries machine. So let's support this
>>>> option on s390x, too, so we can easily enable the serial console here
>>>> again with "-nodefaults", for example. Also map the second -serial
>>>> option to the "sclplmconsole", so that there is now an easy way to
>>>> configure this second console on s390x, too.
>>>> Additionally, the new code is also smaller than the old one and we have
>>>> less s390x-specific code in vl.c :-)  
[...]
>>> I read that as "the current command lines continue to work". Correct?  
>>
>> Right. I was a little bit afraid that this might break migration, but I
>> gave it a quick check and it still seems to work fine here.
>> "info qom-tree" and "info qtree" at the HMP monitor show slightly
>> different output, though ... not sure whether that's critical or not?
> 
> What looks different? If we still send/expect the same kind of
> information, it should not have any impact, I guess.

$ diff -u sclp-qtree-before.txt sclp-qtree-after.txt
--- sclp-qtree-before.txt	2018-04-24 16:17:14.527921927 +0200
+++ sclp-qtree-after.txt	2018-04-24 16:17:35.582877042 +0200
@@ -62,6 +62,6 @@
     bus: s390-sclp-events-bus.0
       type s390-sclp-events-bus
       dev: sclpconsole, id ""
-        chardev = "sclpcon0"
+        chardev = "serial0"
       dev: sclp-cpu-hotplug, id ""
       dev: sclpquiesce, id ""

... i.e. just the label of the chardev changed. I think that's ok, and
migration likely does not care about that.

$ diff -u sclp-qom-tree-before.txt sclp-qom-tree-after.txt
--- sclp-qom-tree-before.txt	2018-04-24 16:17:48.462849585 +0200
+++ sclp-qom-tree-after.txt	2018-04-24 16:18:02.222820256 +0200
@@ -1,13 +1,14 @@
-(qemu) info qom-tree
+(qemu) info qom-tree
 /machine (s390-ccw-virtio-2.12-machine)
   /unattached (container)
-    /system[0] (qemu:memory-region)
     /sysbus (System)
-    /device[1] (virtio-net-ccw)
+    /device[0] (qemu-s390x-cpu)
+    /s390.ram[0] (qemu:memory-region)
+    /device[1] (sclpconsole)
+    /device[2] (virtio-net-ccw)
       /virtio-backend (virtio-net-device)
       /virtio-bus (virtio-ccw-bus)
-    /s390.ram[0] (qemu:memory-region)
-    /device[0] (qemu-s390x-cpu)
+    /system[0] (qemu:memory-region)
     /io[0] (qemu:memory-region)
   /sclp (sclp)
     /s390-sclp-event-facility (s390-sclp-event-facility)
@@ -17,7 +18,6 @@
   /s390-flic-qemu (s390-flic-qemu)
   /s390-skeys (s390-skeys-qemu)
   /peripheral-anon (container)
-    /device[0] (sclpconsole)
   /peripheral (container)
   /s390-ipl (s390-ipl)
   /virtual-css-bridge (virtual-css-bridge)

... the sclpconsole now shows up under /unattached instead of
/peripheral-anon ... does that matter? (I don't have a clue)

 Thomas

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Cornelia Huck]

On Tue, 24 Apr 2018 16:22:26 +0200
Thomas Huth <thuth@redhat.com> wrote:

> On 24.04.2018 16:09, Cornelia Huck wrote:
> > On Tue, 24 Apr 2018 16:02:59 +0200
> > Thomas Huth <thuth@redhat.com> wrote:
> >   
> >> On 24.04.2018 13:49, Christian Borntraeger wrote:  
> >>>
> >>>
> >>> On 04/24/2018 01:44 PM, Thomas Huth wrote:    
> >>>> The consoles ("sclpconsole" and "sclplmconsole") can only be configured
> >>>> with "-device" and "-chardev" so far. Other machines use the convenience
> >>>> option "-serial" to configure the default consoles, too, even for virtual
> >>>> consoles like spapr-vty on the pseries machine. So let's support this
> >>>> option on s390x, too, so we can easily enable the serial console here
> >>>> again with "-nodefaults", for example. Also map the second -serial
> >>>> option to the "sclplmconsole", so that there is now an easy way to
> >>>> configure this second console on s390x, too.
> >>>> Additionally, the new code is also smaller than the old one and we have
> >>>> less s390x-specific code in vl.c :-)    
> [...]
> >>> I read that as "the current command lines continue to work". Correct?    
> >>
> >> Right. I was a little bit afraid that this might break migration, but I
> >> gave it a quick check and it still seems to work fine here.
> >> "info qom-tree" and "info qtree" at the HMP monitor show slightly
> >> different output, though ... not sure whether that's critical or not?  
> > 
> > What looks different? If we still send/expect the same kind of
> > information, it should not have any impact, I guess.  
> 
> $ diff -u sclp-qtree-before.txt sclp-qtree-after.txt
> --- sclp-qtree-before.txt	2018-04-24 16:17:14.527921927 +0200
> +++ sclp-qtree-after.txt	2018-04-24 16:17:35.582877042 +0200
> @@ -62,6 +62,6 @@
>      bus: s390-sclp-events-bus.0
>        type s390-sclp-events-bus
>        dev: sclpconsole, id ""
> -        chardev = "sclpcon0"
> +        chardev = "serial0"
>        dev: sclp-cpu-hotplug, id ""
>        dev: sclpquiesce, id ""
> 
> ... i.e. just the label of the chardev changed. I think that's ok, and
> migration likely does not care about that.

Nod.

> 
> $ diff -u sclp-qom-tree-before.txt sclp-qom-tree-after.txt
> --- sclp-qom-tree-before.txt	2018-04-24 16:17:48.462849585 +0200
> +++ sclp-qom-tree-after.txt	2018-04-24 16:18:02.222820256 +0200
> @@ -1,13 +1,14 @@
> -(qemu) info qom-tree
> +(qemu) info qom-tree
>  /machine (s390-ccw-virtio-2.12-machine)
>    /unattached (container)
> -    /system[0] (qemu:memory-region)
>      /sysbus (System)
> -    /device[1] (virtio-net-ccw)
> +    /device[0] (qemu-s390x-cpu)
> +    /s390.ram[0] (qemu:memory-region)
> +    /device[1] (sclpconsole)
> +    /device[2] (virtio-net-ccw)
>        /virtio-backend (virtio-net-device)
>        /virtio-bus (virtio-ccw-bus)
> -    /s390.ram[0] (qemu:memory-region)
> -    /device[0] (qemu-s390x-cpu)
> +    /system[0] (qemu:memory-region)
>      /io[0] (qemu:memory-region)
>    /sclp (sclp)
>      /s390-sclp-event-facility (s390-sclp-event-facility)
> @@ -17,7 +18,6 @@
>    /s390-flic-qemu (s390-flic-qemu)
>    /s390-skeys (s390-skeys-qemu)
>    /peripheral-anon (container)
> -    /device[0] (sclpconsole)
>    /peripheral (container)
>    /s390-ipl (s390-ipl)
>    /virtual-css-bridge (virtual-css-bridge)
> 
> ... the sclpconsole now shows up under /unattached instead of
> /peripheral-anon ... does that matter? (I don't have a clue)

That's because the device is no longer created through
qdev_device_add(), which attaches it to peripheral-anon. Not sure what
peripheral{,-anon} are used for?

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Paolo Bonzini]

On 24/04/2018 16:22, Thomas Huth wrote:
> $ diff -u sclp-qom-tree-before.txt sclp-qom-tree-after.txt
> --- sclp-qom-tree-before.txt	2018-04-24 16:17:48.462849585 +0200
> +++ sclp-qom-tree-after.txt	2018-04-24 16:18:02.222820256 +0200
> @@ -1,13 +1,14 @@
> -(qemu) info qom-tree
> +(qemu) info qom-tree
>  /machine (s390-ccw-virtio-2.12-machine)
>    /unattached (container)
> -    /system[0] (qemu:memory-region)
>      /sysbus (System)
> -    /device[1] (virtio-net-ccw)
> +    /device[0] (qemu-s390x-cpu)
> +    /s390.ram[0] (qemu:memory-region)
> +    /device[1] (sclpconsole)
> +    /device[2] (virtio-net-ccw)
>        /virtio-backend (virtio-net-device)
>        /virtio-bus (virtio-ccw-bus)
> -    /s390.ram[0] (qemu:memory-region)
> -    /device[0] (qemu-s390x-cpu)
> +    /system[0] (qemu:memory-region)
>      /io[0] (qemu:memory-region)
>    /sclp (sclp)
>      /s390-sclp-event-facility (s390-sclp-event-facility)
> @@ -17,7 +18,6 @@
>    /s390-flic-qemu (s390-flic-qemu)
>    /s390-skeys (s390-skeys-qemu)
>    /peripheral-anon (container)
> -    /device[0] (sclpconsole)
>    /peripheral (container)
>    /s390-ipl (s390-ipl)
>    /virtual-css-bridge (virtual-css-bridge)
> 
> ... the sclpconsole now shows up under /unattached instead of
> /peripheral-anon ... does that matter? (I don't have a clue)

If the devices are created in the same order, before and after the
patch, it should be okay.

/peripheral-anon is for devices that are created with -device but
without an "id" option.

Paolo

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[David Hildenbrand]

On 24.04.2018 13:44, Thomas Huth wrote:
> The consoles ("sclpconsole" and "sclplmconsole") can only be configured
> with "-device" and "-chardev" so far. Other machines use the convenience
> option "-serial" to configure the default consoles, too, even for virtual
> consoles like spapr-vty on the pseries machine. So let's support this
> option on s390x, too, so we can easily enable the serial console here
> again with "-nodefaults", for example. Also map the second -serial
> option to the "sclplmconsole", so that there is now an easy way to
> configure this second console on s390x, too.
> Additionally, the new code is also smaller than the old one and we have
> less s390x-specific code in vl.c :-)
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/event-facility.c         | 14 +++++++++++
>  hw/s390x/s390-virtio-ccw.c        | 19 +++++++++++++--
>  include/hw/boards.h               |  1 -
>  include/hw/s390x/event-facility.h |  2 ++
>  vl.c                              | 50 ---------------------------------------
>  5 files changed, 33 insertions(+), 53 deletions(-)
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 9c24bc6..e6940a2 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -511,3 +511,17 @@ static void register_types(void)
>  }
>  
>  type_init(register_types)
> +
> +BusState *sclp_get_event_facility_bus(void)
> +{
> +    Object *busobj;
> +    SCLPEventsBus *sbus;
> +
> +    busobj = object_resolve_path_type("", TYPE_SCLP_EVENTS_BUS, NULL);
> +    sbus = OBJECT_CHECK(SCLPEventsBus, busobj, TYPE_SCLP_EVENTS_BUS);
> +    if (!sbus) {
> +        return NULL;
> +    }
> +
> +    return &sbus->qbus;
> +}
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 435f7c9..fe28514 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -288,6 +288,15 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>      }
>  }
>  
> +static void s390_create_sclpconsole(const char *type, Chardev *chardev)
> +{
> +    DeviceState *dev;
> +
> +    dev = qdev_create(sclp_get_event_facility_bus(), type);
> +    qdev_prop_set_chr(dev, "chardev", chardev);
> +    qdev_init_nofail(dev);
> +}
> +
>  static void ccw_init(MachineState *machine)
>  {
>      int ret;
> @@ -311,6 +320,14 @@ static void ccw_init(MachineState *machine)
>                        machine->initrd_filename, "s390-ccw.img",
>                        "s390-netboot.img", true);
>  
> +    /* init consoles */
> +    if (serial_hds[0]) {
> +        s390_create_sclpconsole("sclpconsole", serial_hds[0]);
> +    }
> +    if (serial_hds[1]) {
> +        s390_create_sclpconsole("sclplmconsole", serial_hds[1]);
> +    }
> +
>      /*
>       * We cannot easily make the pci host bridge conditional as older QEMUs
>       * always created it. Doing so would break migration across QEMU versions.
> @@ -470,10 +487,8 @@ static void ccw_machine_class_init(ObjectClass *oc, void *data)
>      mc->block_default_type = IF_VIRTIO;
>      mc->no_cdrom = 1;
>      mc->no_floppy = 1;
> -    mc->no_serial = 1;
>      mc->no_parallel = 1;
>      mc->no_sdcard = 1;
> -    mc->use_sclp = 1;
>      mc->max_cpus = S390_MAX_CPUS;
>      mc->has_hotpluggable_cpus = true;
>      mc->get_hotplug_handler = s390_get_hotplug_handler;
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index a609239..5c5eee5 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -180,7 +180,6 @@ struct MachineClass {
>      unsigned int no_serial:1,
>          no_parallel:1,
>          use_virtcon:1,
> -        use_sclp:1,
>          no_floppy:1,
>          no_cdrom:1,
>          no_sdcard:1,
> diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> index 5698e5e..5cc16f6 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -210,4 +210,6 @@ typedef struct SCLPEventFacilityClass {
>      bool (*event_pending)(SCLPEventFacility *ef);
>  } SCLPEventFacilityClass;
>  
> +BusState *sclp_get_event_facility_bus(void);
> +
>  #endif
> diff --git a/vl.c b/vl.c
> index fce1fd1..b32340c 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -133,7 +133,6 @@ int main(int argc, char **argv)
>  #include "sysemu/iothread.h"
>  
>  #define MAX_VIRTIO_CONSOLES 1
> -#define MAX_SCLP_CONSOLES 1
>  
>  static const char *data_dir[16];
>  static int data_dir_idx;
> @@ -157,7 +156,6 @@ int no_frame;
>  Chardev *serial_hds[MAX_SERIAL_PORTS];
>  Chardev *parallel_hds[MAX_PARALLEL_PORTS];
>  Chardev *virtcon_hds[MAX_VIRTIO_CONSOLES];
> -Chardev *sclp_hds[MAX_SCLP_CONSOLES];
>  int win2k_install_hack = 0;
>  int singlestep = 0;
>  int smp_cpus;
> @@ -209,7 +207,6 @@ static int has_defaults = 1;
>  static int default_serial = 1;
>  static int default_parallel = 1;
>  static int default_virtcon = 1;
> -static int default_sclp = 1;
>  static int default_monitor = 1;
>  static int default_floppy = 1;
>  static int default_cdrom = 1;
> @@ -2571,39 +2568,6 @@ static int virtcon_parse(const char *devname)
>      return 0;
>  }
>  
> -static int sclp_parse(const char *devname)
> -{
> -    QemuOptsList *device = qemu_find_opts("device");
> -    static int index = 0;
> -    char label[32];
> -    QemuOpts *dev_opts;
> -
> -    if (strcmp(devname, "none") == 0) {
> -        return 0;
> -    }
> -    if (index == MAX_SCLP_CONSOLES) {
> -        error_report("too many sclp consoles");
> -        exit(1);
> -    }
> -
> -    assert(arch_type == QEMU_ARCH_S390X);
> -
> -    dev_opts = qemu_opts_create(device, NULL, 0, NULL);
> -    qemu_opt_set(dev_opts, "driver", "sclpconsole", &error_abort);
> -
> -    snprintf(label, sizeof(label), "sclpcon%d", index);
> -    sclp_hds[index] = qemu_chr_new(label, devname);
> -    if (!sclp_hds[index]) {
> -        error_report("could not connect sclp console"
> -                     " to character backend '%s'", devname);
> -        return -1;
> -    }
> -    qemu_opt_set(dev_opts, "chardev", label, &error_abort);
> -
> -    index++;
> -    return 0;
> -}
> -
>  static int debugcon_parse(const char *devname)
>  {
>      QemuOpts *opts;
> @@ -4237,9 +4201,6 @@ int main(int argc, char **argv, char **envp)
>      if (!has_defaults || !machine_class->use_virtcon) {
>          default_virtcon = 0;
>      }
> -    if (!has_defaults || !machine_class->use_sclp) {
> -        default_sclp = 0;
> -    }
>      if (!has_defaults || machine_class->no_floppy) {
>          default_floppy = 0;
>      }
> @@ -4286,16 +4247,11 @@ int main(int argc, char **argv, char **envp)
>              add_device_config(DEV_SERIAL, "mon:stdio");
>          } else if (default_virtcon && default_monitor) {
>              add_device_config(DEV_VIRTCON, "mon:stdio");
> -        } else if (default_sclp && default_monitor) {
> -            add_device_config(DEV_SCLP, "mon:stdio");
>          } else {
>              if (default_serial)
>                  add_device_config(DEV_SERIAL, "stdio");
>              if (default_virtcon)
>                  add_device_config(DEV_VIRTCON, "stdio");
> -            if (default_sclp) {
> -                add_device_config(DEV_SCLP, "stdio");
> -            }
>              if (default_monitor)
>                  monitor_parse("stdio", "readline", false);
>          }
> @@ -4308,9 +4264,6 @@ int main(int argc, char **argv, char **envp)
>              monitor_parse("vc:80Cx24C", "readline", false);
>          if (default_virtcon)
>              add_device_config(DEV_VIRTCON, "vc:80Cx24C");
> -        if (default_sclp) {
> -            add_device_config(DEV_SCLP, "vc:80Cx24C");
> -        }
>      }
>  
>  #if defined(CONFIG_VNC)
> @@ -4560,9 +4513,6 @@ int main(int argc, char **argv, char **envp)
>          exit(1);
>      if (foreach_device_config(DEV_VIRTCON, virtcon_parse) < 0)
>          exit(1);
> -    if (foreach_device_config(DEV_SCLP, sclp_parse) < 0) {
> -        exit(1);
> -    }
>      if (foreach_device_config(DEV_DEBUGCON, debugcon_parse) < 0)
>          exit(1);
>  
> 

If this doesn't break any setups we have, I really like it :)

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Eduardo Habkost]

On Tue, Apr 24, 2018 at 06:19:35PM +0200, Paolo Bonzini wrote:
> On 24/04/2018 16:22, Thomas Huth wrote:
> > $ diff -u sclp-qom-tree-before.txt sclp-qom-tree-after.txt
> > --- sclp-qom-tree-before.txt	2018-04-24 16:17:48.462849585 +0200
> > +++ sclp-qom-tree-after.txt	2018-04-24 16:18:02.222820256 +0200
> > @@ -1,13 +1,14 @@
> > -(qemu) info qom-tree
> > +(qemu) info qom-tree
> >  /machine (s390-ccw-virtio-2.12-machine)
> >    /unattached (container)
> > -    /system[0] (qemu:memory-region)
> >      /sysbus (System)
> > -    /device[1] (virtio-net-ccw)
> > +    /device[0] (qemu-s390x-cpu)
> > +    /s390.ram[0] (qemu:memory-region)
> > +    /device[1] (sclpconsole)
> > +    /device[2] (virtio-net-ccw)
> >        /virtio-backend (virtio-net-device)
> >        /virtio-bus (virtio-ccw-bus)
> > -    /s390.ram[0] (qemu:memory-region)
> > -    /device[0] (qemu-s390x-cpu)
> > +    /system[0] (qemu:memory-region)
> >      /io[0] (qemu:memory-region)
> >    /sclp (sclp)
> >      /s390-sclp-event-facility (s390-sclp-event-facility)
> > @@ -17,7 +18,6 @@
> >    /s390-flic-qemu (s390-flic-qemu)
> >    /s390-skeys (s390-skeys-qemu)
> >    /peripheral-anon (container)
> > -    /device[0] (sclpconsole)
> >    /peripheral (container)
> >    /s390-ipl (s390-ipl)
> >    /virtual-css-bridge (virtual-css-bridge)
> > 
> > ... the sclpconsole now shows up under /unattached instead of
> > /peripheral-anon ... does that matter? (I don't have a clue)
> 
> If the devices are created in the same order, before and after the
> patch, it should be okay.

Is there a way to find out if the devices are created in the same
order using only QMP commands?  It would be very useful to
automatically validate compatibility between QEMU versions.

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH] hw/s390x: Allow to configure the consoles with the "-serial" parameter

[Paolo Bonzini]

On 27/04/2018 01:55, Eduardo Habkost wrote:
>>>
>>> ... the sclpconsole now shows up under /unattached instead of
>>> /peripheral-anon ... does that matter? (I don't have a clue)
>> If the devices are created in the same order, before and after the
>> patch, it should be okay.
> Is there a way to find out if the devices are created in the same
> order using only QMP commands?  It would be very useful to
> automatically validate compatibility between QEMU versions.

We could add QOM properties for the migration "instance id" and all the
other bits that are placed in the migration stream.

Thanks,

Paolo