hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

[PATCH v2] hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

Posted by Thomas Huth 2 weeks, 3 days ago

The sclpconsole currently does not have a proper parent in the QOM
tree, so it shows up under /machine/unattached - which is somewhat
ugly. We should rather attach it to /machine/sclp/s390-sclp-event-facility
where the other devices of type TYPE_SCLP_EVENT already reside.

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/s390x/s390-virtio-ccw.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 5c83d1ea17..41be8bf857 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -242,11 +242,13 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
 
 static void s390_create_sclpconsole(const char *type, Chardev *chardev)
 {
+    BusState *ev_fac_bus = sclp_get_event_facility_bus();
     DeviceState *dev;
 
     dev = qdev_new(type);
+    object_property_add_child(OBJECT(ev_fac_bus->parent), type, OBJECT(dev));
     qdev_prop_set_chr(dev, "chardev", chardev);
-    qdev_realize_and_unref(dev, sclp_get_event_facility_bus(), &error_fatal);
+    qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
 }
 
 static void ccw_init(MachineState *machine)
-- 
2.44.0

Re: [PATCH v2] hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

Posted by Cédric Le Goater 2 weeks, 1 day ago

On 4/30/24 21:08, Thomas Huth wrote:
> The sclpconsole currently does not have a proper parent in the QOM
> tree, so it shows up under /machine/unattached - which is somewhat
> ugly. We should rather attach it to /machine/sclp/s390-sclp-event-facility
> where the other devices of type TYPE_SCLP_EVENT already reside.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

> ---
>   hw/s390x/s390-virtio-ccw.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 5c83d1ea17..41be8bf857 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -242,11 +242,13 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>   
>   static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>   {
> +    BusState *ev_fac_bus = sclp_get_event_facility_bus();

This routine sclp_get_event_facility_bus() which scans all the machine
could be avoided and even removed. For that, SCLPDevice should be an
attribute of the machine. This means reshuffling definitions and
object instanciations a little in the code. Would you be OK with that ?

Other machine units/models could undergo the same kind of changes.

Thanks,

C.

>       DeviceState *dev;
>   
>       dev = qdev_new(type);
> +    object_property_add_child(OBJECT(ev_fac_bus->parent), type, OBJECT(dev));
>       qdev_prop_set_chr(dev, "chardev", chardev);
> -    qdev_realize_and_unref(dev, sclp_get_event_facility_bus(), &error_fatal);
> +    qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>   }
>   
>   static void ccw_init(MachineState *machine)

Re: [PATCH v2] hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

Posted by Thomas Huth 2 weeks, 1 day ago

On 02/05/2024 09.57, Cédric Le Goater wrote:
> On 4/30/24 21:08, Thomas Huth wrote:
>> The sclpconsole currently does not have a proper parent in the QOM
>> tree, so it shows up under /machine/unattached - which is somewhat
>> ugly. We should rather attach it to /machine/sclp/s390-sclp-event-facility
>> where the other devices of type TYPE_SCLP_EVENT already reside.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> 
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 5c83d1ea17..41be8bf857 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -242,11 +242,13 @@ static void s390_create_virtio_net(BusState *bus, 
>> const char *name)
>>   static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>>   {
>> +    BusState *ev_fac_bus = sclp_get_event_facility_bus();
> 
> This routine sclp_get_event_facility_bus() which scans all the machine
> could be avoided and even removed. For that, SCLPDevice should be an
> attribute of the machine. This means reshuffling definitions and
> object instanciations a little in the code. Would you be OK with that ?

Sounds like a good clean-up, yes!

  Thomas

Re: [PATCH v2] hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

Posted by Eric Farman 2 weeks, 2 days ago

On Tue, 2024-04-30 at 21:08 +0200, Thomas Huth wrote:
> The sclpconsole currently does not have a proper parent in the QOM
> tree, so it shows up under /machine/unattached - which is somewhat
> ugly. We should rather attach it to /machine/sclp/s390-sclp-event-
> facility
> where the other devices of type TYPE_SCLP_EVENT already reside.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/s390x/s390-virtio-ccw.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Eric Farman <farman@linux.ibm.com>

Re: [PATCH v2] hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

Posted by Philippe Mathieu-Daudé 2 weeks, 3 days ago

On 30/4/24 21:08, Thomas Huth wrote:
> The sclpconsole currently does not have a proper parent in the QOM
> tree, so it shows up under /machine/unattached - which is somewhat
> ugly. We should rather attach it to /machine/sclp/s390-sclp-event-facility
> where the other devices of type TYPE_SCLP_EVENT already reside.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v2] hw/s390x: Attach the sclpconsole to /machine/sclp/s390-sclp-event-facility

Posted by David Hildenbrand 2 weeks, 3 days ago

On 30.04.24 21:08, Thomas Huth wrote:
> The sclpconsole currently does not have a proper parent in the QOM
> tree, so it shows up under /machine/unattached - which is somewhat
> ugly. We should rather attach it to /machine/sclp/s390-sclp-event-facility
> where the other devices of type TYPE_SCLP_EVENT already reside.
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 5c83d1ea17..41be8bf857 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -242,11 +242,13 @@ static void s390_create_virtio_net(BusState *bus, const char *name)
>   
>   static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>   {
> +    BusState *ev_fac_bus = sclp_get_event_facility_bus();
>       DeviceState *dev;
>   
>       dev = qdev_new(type);
> +    object_property_add_child(OBJECT(ev_fac_bus->parent), type, OBJECT(dev));
>       qdev_prop_set_chr(dev, "chardev", chardev);
> -    qdev_realize_and_unref(dev, sclp_get_event_facility_bus(), &error_fatal);
> +    qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>   }
>   
>   static void ccw_init(MachineState *machine)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb