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

[Shalini Chellathurai Saroja <shalini@linux.ibm.com>]

On 2025-06-05 13:56, 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?
> 
>>> +{ 'struct': 'S390ControlProgramId', 'data': {
>>> +     'system-type': 'str',
>>> +     'system-name': 'str',
>>> +     'system-level': 'uint64',
>>> +     'sysplex-name': 'str',
>>> +     'timestamp': 'uint64' } }
>> 
>> This is unused now, so you can get rid of it and put the
>> documentation sclpcpi.c.
> 
> Agreed, that looks like it could be cleaned up now, indeed.
> 

Hello Thomas, Nina,

Yes, I will do this. I am sorry that I missed to do this earlier.
Thank you very much for the quick review.



>  Thomas


-- 
Mit freundlichen Grüßen / Kind regards
Shalini Chellathurai Saroja
Software Developer
Linux on IBM Z & KVM Development
IBM Deutschland Research & Development GmbH
Dept 1419, Schoenaicher Str. 220, 71032 Boeblingen
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht 
Stuttgart, HRB 243294