Re: [PATCH] hw/s390x: Attach the sclpconsole to the /machine/sclp node

[Thomas Huth <thuth@redhat.com>]

On 30/04/2024 13.58, Cédric Le Goater wrote:
> On 4/30/24 10:04, 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. Let's attach it to /machine/sclp instead.
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>   include/hw/s390x/sclp.h    |  2 +-
>>   hw/s390x/s390-virtio-ccw.c | 11 +++++++----
>>   hw/s390x/sclp.c            |  4 +++-
>>   3 files changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/hw/s390x/sclp.h b/include/hw/s390x/sclp.h
>> index b405a387b6..abfd6d8868 100644
>> --- a/include/hw/s390x/sclp.h
>> +++ b/include/hw/s390x/sclp.h
>> @@ -222,7 +222,7 @@ static inline int sccb_data_len(SCCB *sccb)
>>   }
>> -void s390_sclp_init(void);
>> +Object *s390_sclp_init(void);
>>   void sclp_service_interrupt(uint32_t sccb);
>>   void raise_irq_cpu_hotplug(void);
>>   int sclp_service_call(S390CPU *cpu, uint64_t sccb, uint32_t code);
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 4dcc213820..e2f9206ded 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -237,11 +237,13 @@ static void s390_create_virtio_net(BusState *bus, 
>> const char *name)
>>       }
>>   }
>> -static void s390_create_sclpconsole(const char *type, Chardev *chardev)
>> +static void s390_create_sclpconsole(Object *sclp, const char *type,
>> +                                    Chardev *chardev)
>>   {
>>       DeviceState *dev;
>>       dev = qdev_new(type);
>> +    object_property_add_child(sclp, type, OBJECT(dev));
>>       qdev_prop_set_chr(dev, "chardev", chardev);
>>       qdev_realize_and_unref(dev, sclp_get_event_facility_bus(), 
>> &error_fatal);
>>   }
>> @@ -252,8 +254,9 @@ static void ccw_init(MachineState *machine)
>>       int ret;
>>       VirtualCssBus *css_bus;
>>       DeviceState *dev;
>> +    Object *sclp;
>> -    s390_sclp_init();
>> +    sclp = s390_sclp_init();
> 
> I would simply drop s390_sclp_init(), same for :
> 
>    void s390_init_tod(void);
>    void s390_init_ap(void);
>    void s390_stattrib_init(void);
>    void s390_skeys_init(void);
>    void s390_flic_init(void);
> 
> These routines all do the same and are not very useful TBH, and I would
> add pointers under the s390x MachineState possibly.

Some of them seem to do a little bit more things, like checking whether the 
feature is available or not, e.g. s390_init_ap() ... IMHO it makes sense to 
keep at least those?

But for s390_sclp_init ... it could be inlined, indeed, especially if we 
also switch the object_unref + qdev_realize in there into 
qdev_realize_and_unref. Let me try to do that in a v2 ...

  Thomas