Re: [PATCH v5 2/3] hw/s390x: add Control-Program Identification to QOM

[Nina Schotterl-Glausch <Nina.Schoetterl-Glausch@ibm.com>]

On Thu, 2025-06-05 at 13:56 +0200, Thomas Huth wrote:
> On 05/06/2025 10.34, Nina Schoetterl-Glausch wrote:
> > On Tue, 2025-06-03 at 15:56 +0200, Shalini Chellathurai Saroja wrote:
> > > Add Control-Program Identification (CPI) data to the QEMU Object
> > > Model (QOM), along with the timestamp in which the data was received
> > > as shown below.
> > 
> > [...]
> > > 
> > > Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> > > ---
> > >   hw/s390x/sclpcpi.c                | 47 +++++++++++++++++++++++++
> > >   include/hw/s390x/event-facility.h |  5 +++
> > >   qapi/machine.json                 | 58 +++++++++++++++++++++++++++++++
> > >   3 files changed, 110 insertions(+)
> > > 
> > > diff --git a/hw/s390x/sclpcpi.c b/hw/s390x/sclpcpi.c
> > > index 935fa87acd..ec711e2291 100644
> > > --- a/hw/s390x/sclpcpi.c
> > > +++ b/hw/s390x/sclpcpi.c
> > > @@ -15,7 +15,9 @@
> > >     */
> > 
> > [...]
> > >   
> > > +static void cpi_init(Object *obj)
> > > +{
> > > +    SCLPEventCPI *e = SCLP_EVENT_CPI(obj);
> > > +
> > > +    object_property_add_str(obj, "system_type", get_system_type, NULL);
> > > +    object_property_add_str(obj, "system_name", get_system_name, NULL);
> > > +    object_property_add_str(obj, "sysplex_name", get_sysplex_name, NULL);
> > > +    object_property_add_uint64_ptr(obj, "system_level", &(e->system_level),
> > > +                                   OBJ_PROP_FLAG_READ);
> > > +    object_property_add_uint64_ptr(obj, "timestamp", &(e->timestamp),
> > > +                                   OBJ_PROP_FLAG_READ);
> > > +}
> > 
> > I think it would be cleaner if those were class properties.
> > You could use object_class_property_add_str in cpi_class_init,
> > but I think it'd be nice to use DEFINE_PROP_(STR|UINT64) and
> > device_class_set_props.
> 
> For "normal" properties I'd say "yes" ... but in this case, this would also 
> allow the user to set the properties from the host side - which would be a 
> little bit weird? So I think it might be cleaner to keep it this way here 
> without the "setter" functions? WDYT?

Oh, yeah I didn't consider this.
Still, object_class_property_add_str would be better I think, all instances
of the class should have these properties.


[...]

-- 
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294