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

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

On 2025-03-05 17:06, Daniel P. Berrangé wrote:
> On Mon, Feb 24, 2025 at 01:04:47PM +0100, 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),
> 
> If the data is an integer, we must return it in QMP as an integer,
> not formatted into a hex string.
> 

Hello Daniel,

Thank you very much for the review.

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.

'system-level': '0x10a000000060b00'


>> +        .sysplex_name = g_strndup((char *) ms->cpi.sysplex_name,
>> +                        sizeof(ms->cpi.sysplex_name)),
>> +        .timestamp = ms->cpi.timestamp
>> +    };
> 
>> +##
>> +# @S390ControlProgramId:
>> +#
>> +# Control-program identifiers provide data about Linux instance.
>> +#
>> +# @system-type: operating system of Linux instance
> 
> Is there a list of well known operating system names, or is
> this arbitrary free-form text. Needs to be documented.
> 

ok.

> 
>> +# @system-name: system name of Linux instance
> 
> What is a system name ?  Is that a hostname, or is that something
> else ?
> 

Yes, it is the hostname of the guest virtual machine.

>> +#
>> +# @system-level: distribution and kernel version of Linux instance
> 
> What does this actually mean ?  This is a single field, but the docs
> are describing 2 distinct versions. Even a single version usually
> has multiple digits and even a string suffix. This needs to document
> how the version information actually encoded in practice, and if
> some info is discarded in this process.
> 

Please refer 
https://www.ibm.com/docs/en/linux-on-systems?topic=identification-system-level 
for information on system-level. I will document this.

>> +#
>> +# @sysplex-name: sysplex name of Linux instance
> 
> I guess "sysplex" is some term people from s390 world would
> already understand ? It is possible to explain it or do we just
> have to assume prior knowledge ?
> 

Sysplex name - Sysplex refers to a cluster of logical partitions that 
communicates and co-operates with each other. Sysplex name is the name 
of the cluster which the guest belongs to(If any).(eg: PLEX)

I will document this.

>> +#
>> +# @timestamp: latest update of CPI data
> 
> In what units and epoch ? Seconds since the UNIX epoch, or
> something else ?
> 

In nanoseconds since the UNIX epoch. I will document this. I used the 
inbuilt method qemu_clock_get_ns() to get this epoch value.

Thank you.

>> +#
>> +# Since: 9.2
>> +##
>> +{ 'struct': 'S390ControlProgramId', 'data': {
>> +     'system-type': 'str',
>> +     'system-name': 'str',
>> +     'system-level': 'str',
>> +     'sysplex-name': 'str',
>> +     'timestamp': 'uint64' } }
>> --
>> 2.47.0
>> 
>> 
> 
> With regards,
> Daniel

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