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

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

On 2025-06-05 14:03, Thomas Huth wrote:
> On 05/06/2025 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?
> 
> By the way, Shalini, could you please add a description of the
> properties via object_property_set_description() here, please?
> 
>  Thanks,
>   Thomas
> 

Hello Thomas,
I will add description for the properties via 
object_class_property_set_description(),
as I will be using object_class_property_add_str and 
object_class_property_add methods
based on Nina's feedback and Markus's summary on properties which you 
pointed out.

Thank you very much.

> PS: FWIW, Markus recently also provided a nice summery about the
> different property types here:
>  https://lore.kernel.org/qemu-devel/87o6vfzdig.fsf@pond.sub.org/

-- 
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