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

[Thomas Huth <thuth@redhat.com>]

On 06/03/2025 13.23, shalini wrote:
> 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)),

That indentation looks fine, thanks!

...
>>> +##
>>> +{ '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'

Well, the idea of QOM/QAPI is: It's an *API* for machines, not meant for 
direct consumption by the end user. So passing an integer as a string is not 
the right way here. For the user, you'd require some upper level instead 
that renders the integer in the right shape for the user. So please don't 
use a string for this at the QOM/QAPI level.

  Thanks,
   Thomas