Re: [PATCH qemu v2 1/3] hw/s390x: add CPI identifiers to QOM

[shalini <shalini@imap.linux.ibm.com>]

On 2025-03-05 16:56, Thomas Huth wrote:
> On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
>> Add Control-Program Identification (CPI) to the QEMU Object
>> Model (QOM). The CPI identifiers provide information about
>> the guest operating system. The CPI identifiers are:
>> system type, system name, system level and sysplex name.
>> 
>> The system type provides the OS type of the guest (e.g. LINUX).
>> The system name provides the name of the guest (e.g. TESTVM).
>> The system level provides the distribution and kernel version
>> of the guest OS (e.g. 0x50e00).
>> The sysplex name provides the sysplex name of the guest
>> (e.g. SYSPLEX).
>> 
>> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c         | 29 
>> +++++++++++++++++++++++++++++
>>   include/hw/s390x/s390-virtio-ccw.h |  8 ++++++++
>>   qapi/machine.json                  | 24 ++++++++++++++++++++++++
>>   3 files changed, 61 insertions(+)
>> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 51ae0c133d..13ea8db1b0 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -50,6 +50,7 @@
>>   #include "hw/s390x/virtio-ccw-md.h"
>>   #include "system/replay.h"
>>   #include CONFIG_DEVICES
>> +#include "qapi/qapi-visit-machine.h"
>>     static Error *pv_mig_blocker;
>>   @@ -803,6 +804,26 @@ static void machine_set_loadparm(Object *obj, 
>> Visitor *v,
>>       s390_ipl_fmt_loadparm(ms->loadparm, val, errp);
>>   }
>>   +static void machine_get_control_program_id(Object *obj, Visitor *v,
>> +                                           const char *name, void 
>> *opaque,
>> +                                           Error **errp)
>> +{
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
>> +    S390ControlProgramId *cpi;
>> +    cpi = &(S390ControlProgramId){
>> +        .system_type = g_strndup((char *) ms->cpi.system_type,
>> +                       sizeof(ms->cpi.system_type)),
>> +        .system_name = g_strndup((char *) ms->cpi.system_name,
>> +                       sizeof(ms->cpi.system_name)),
>> +        .system_level = g_strdup_printf("0x%lx", 
>> ms->cpi.system_level),
>> +        .sysplex_name = g_strndup((char *) ms->cpi.sysplex_name,
>> +                        sizeof(ms->cpi.sysplex_name)),
>> +        .timestamp = ms->cpi.timestamp
>> +    };
> 
> Could you please indend the sizeof() lines with the "(" after the
> g_strndup in the previous line?
> 

Hello Thomas,

Sure, I have provided a sample code below, please let me know if this is 
incorrect. Thank you.

>> +    cpi = &(S390ControlProgramId){
>> +        .system_type = g_strndup((char *) ms->cpi.system_type,
>> +                                 sizeof(ms->cpi.system_type)),


>> +
>> +    visit_type_S390ControlProgramId(v, name, &cpi, &error_abort);
>> +}
>> +
>>   static void ccw_machine_class_init(ObjectClass *oc, void *data)
>>   {
>>       MachineClass *mc = MACHINE_CLASS(oc);
>> @@ -854,6 +875,14 @@ static void ccw_machine_class_init(ObjectClass 
>> *oc, void *data)
>>               "Up to 8 chars in set of [A-Za-z0-9. ] (lower case chars 
>> converted"
>>               " to upper case) to pass to machine loader, boot 
>> manager,"
>>               " and guest kernel");
>> +    object_class_property_add(oc, "s390-control-program-id",
> 
> I think I'd rather drop the "s390-" prefix here. The property is
> already part of the s390-virtio-ccw machine, so it should be obvious
> that this is related to s390.
ok.

> 
>> +                              "S390ControlProgramId",
>> +                              machine_get_control_program_id,
>> +                              NULL, NULL, NULL);
>> +    object_class_property_set_description(oc, 
>> "s390-control-program-id",
>> +        "Control-progam identifiers provide data about the guest "
> 
> s/progam/program/
> 
ok.

>> +        "operating system");
>> +
>>   }
>>     static inline void s390_machine_initfn(Object *obj)

[...]

>> diff --git a/qapi/machine.json b/qapi/machine.json
>> index a6b8795b09..c6cbad87e1 100644
>> --- a/qapi/machine.json
>> +++ b/qapi/machine.json
>> @@ -1898,3 +1898,27 @@
>>   { 'command': 'x-query-interrupt-controllers',
>>     'returns': 'HumanReadableText',
>>     'features': [ 'unstable' ]}
>> +
>> +##
>> +# @S390ControlProgramId:
>> +#
>> +# Control-program identifiers provide data about Linux instance.
> 
> If I understood correctly, this could also theoretically be used by
> other guest operating systems? If so, please replace "Linux instance"
> with "guest operating system".
> 

Yes, that is correct. I will change the description of the attributes 
below based on the comments from you and Daniel, thank you.

>> +#
>> +# @system-type: operating system of Linux instance
> 
> Replace with:
> 
>  @system-type: operating system (e.g. "LINUX")
> 
> ?
> 
>> +#
>> +# @system-name: system name of Linux instance
> 
> Name of the VM instance ?
> 
>> +# @system-level: distribution and kernel version of Linux instance
>> +#
>> +# @sysplex-name: sysplex name of Linux instance
>> +#
>> +# @timestamp: latest update of CPI data
>> +#
>> +# Since: 9.2
> 
> 9.2 has already been released, so this should be 10.0.
> 

ok.

>> +##
>> +{ 'struct': 'S390ControlProgramId', 'data': {
>> +     'system-type': 'str',
>> +     'system-name': 'str',
>> +     'system-level': 'str',
> 
> Not sure, but would it make sense to use a number for the system-level
> instead? At least it's a number in ControlProgramId, not a string.
> 

The system-level, when interpreted as an int provides the output below

'system-level': 74872343805430528

But the desired output below is obtained only when interpreted as a str. 
please refer 
https://www.ibm.com/docs/en/linux-on-systems?topic=identification-system-level 
for details on system-level. I will also document this in the 
description of system-level as suggested by Daniel. Thank you.

'system-level': '0x10a000000060b00'

>  Thomas
> 
> 
>> +     'sysplex-name': 'str',
>> +     'timestamp': 'uint64' } }

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