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

[Thomas Huth]

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