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

[Thomas Huth <thuth@redhat.com>]

On 30/04/2024 16.24, Thomas Huth wrote:
> 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 ...

Actually, after looking at the code a little bit longer, it seems to me like 
the sclpconsole should be attached to /machine/sclp/s390-sclp-event-facility
instead of just /machine/sclp, since the other devices of type 
TYPE_SCLP_EVENT are also located there. That makes the patch even easier 
since we already have the pointer from sclp_get_event_facility_bus() in that 
function.

  Thomas