diff mbox series

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

Message ID 20250224120449.1764114-1-shalini@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series [qemu,v2,1/3] hw/s390x: add CPI identifiers to QOM | expand

Commit Message

Shalini Chellathurai Saroja Feb. 24, 2025, 12:04 p.m. UTC
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(+)

Comments

Thomas Huth March 5, 2025, 3:56 p.m. UTC | #1
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?

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

> +                              "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/

> +        "operating system");
> +
>   }
>   
>   static inline void s390_machine_initfn(Object *obj)
> diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
> index 686d9497d2..6872f7a176 100644
> --- a/include/hw/s390x/s390-virtio-ccw.h
> +++ b/include/hw/s390x/s390-virtio-ccw.h
> @@ -19,6 +19,13 @@
>   
>   OBJECT_DECLARE_TYPE(S390CcwMachineState, S390CcwMachineClass, S390_CCW_MACHINE)
>   
> +typedef struct ControlProgramId {
> +    uint8_t system_type[8];
> +    uint8_t system_name[8];
> +    uint64_t system_level;
> +    uint8_t sysplex_name[8];
> +    uint64_t timestamp;
> +} QEMU_PACKED ControlProgramId;
>   
>   struct S390CcwMachineState {
>       /*< private >*/
> @@ -33,6 +40,7 @@ struct S390CcwMachineState {
>       uint64_t max_pagesize;
>   
>       SCLPDevice *sclp;
> +    ControlProgramId cpi;
>   };
>   
>   static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
> 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".

> +#
> +# @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.

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

  Thomas


> +     'sysplex-name': 'str',
> +     'timestamp': 'uint64' } }
Daniel P. Berrangé March 5, 2025, 4:06 p.m. UTC | #2
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.

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


> +# @system-name: system name of Linux instance

What is a system name ?  Is that a hostname, or is that something
else ?

> +#
> +# @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.

> +#
> +# @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 ?

> +#
> +# @timestamp: latest update of CPI data

In what units and epoch ? Seconds since the UNIX epoch, or
something else ?

> +#
> +# 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
Thomas Huth March 5, 2025, 6:05 p.m. UTC | #3
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
> +    };
> +
> +    visit_type_S390ControlProgramId(v, name, &cpi, &error_abort);

Forgot to say: Please use errp here instead -----------^

  Thanks!
   Thomas
shalini March 6, 2025, 12:23 p.m. UTC | #4
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' } }
shalini March 6, 2025, 1:55 p.m. UTC | #5
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
shalini March 6, 2025, 1:57 p.m. UTC | #6
On 2025-03-05 19:05, 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
>> +    };
>> +
>> +    visit_type_S390ControlProgramId(v, name, &cpi, &error_abort);
> 
> Forgot to say: Please use errp here instead -----------^
> 

Hello Thomas,

ok. Thank you.

>  Thanks!
>   Thomas
Thomas Huth March 6, 2025, 2:55 p.m. UTC | #7
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
Daniel P. Berrangé March 6, 2025, 3:06 p.m. UTC | #8
BTW, your email client is possibly mis-configured - your mail
came through with "From: shalini <shalini@imap.linux.ibm.com>"
and attempting to reply to that gets an error saying that
'imap.linux.ibm.com' does not exist.

On Thu, Mar 06, 2025 at 02:55:27PM +0100, shalini wrote:
> 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

Yes, that is correct from a QMP design POV. Data should formatted
in the most appropriate way for machines to consume, using native
JSON data types.

> But the desired output below is obtained only when interpreted as a str.
> 
> 'system-level': '0x10a000000060b00'

If a human wants to read the data in hex format, that should be
formatted by whatever tool is consuming the data from QMP and
presenting it in the user.

> > > +# @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.

Lets rename it to 'system-hostname' to be unambiguous.


With regards,
Daniel
Nina Schoetterl-Glausch March 6, 2025, 3:36 p.m. UTC | #9
On Thu, 2025-03-06 at 15:06 +0000, Daniel P. Berrangé wrote:
> BTW, your email client is possibly mis-configured - your mail
> came through with "From: shalini <shalini@imap.linux.ibm.com>"
> and attempting to reply to that gets an error saying that
> 'imap.linux.ibm.com' does not exist.
> 
> On Thu, Mar 06, 2025 at 02:55:27PM +0100, shalini wrote:
> > 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(+)

[...]
> 
> > > > +# @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.

Is it? Seems to me it it just a user configurable name of the system.
It's empty on the two LPARs I checked.

> 
> Lets rename it to 'system-hostname' to be unambiguous.

In any case "system name" is the established name for this value on s390x.
> 
> 
> With regards,
> Daniel
Nina Schoetterl-Glausch March 6, 2025, 3:44 p.m. UTC | #10
On Thu, 2025-03-06 at 15:55 +0100, Thomas Huth wrote:
> 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 @@

[...]

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

In a sense the system level is a bitfield.
So this could become a struct

{
	'hypervisor-use' : true,
	'distribution-id': 3,	// or an enum?
	'distribution-version-major: 24,
	...
}

Not sure how to handle the 3 undefined bits in the highest byte.

> 
>   Thanks,
>    Thomas
>
Daniel P. Berrangé March 6, 2025, 4:05 p.m. UTC | #11
On Thu, Mar 06, 2025 at 04:44:33PM +0100, Nina Schoetterl-Glausch wrote:
> On Thu, 2025-03-06 at 15:55 +0100, Thomas Huth wrote:
> > 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 @@
> 
> [...]
> 
> > > > > +##
> > > > > +{ '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.
> 
> In a sense the system level is a bitfield.
> So this could become a struct
> 
> {
> 	'hypervisor-use' : true,
> 	'distribution-id': 3,	// or an enum?
> 	'distribution-version-major: 24,
> 	...
> }

Yes, if this is a mandatory format, breaking out the fields makes sense.

> Not sure how to handle the 3 undefined bits in the highest byte.

If they're always left zero, might as well just omit them until
such time as they have semantics defined (if ever)


With regards,
Daniel
Shalini Chellathurai Saroja March 7, 2025, 3:22 p.m. UTC | #12
On 2025-03-06 16:06, Daniel P. Berrangé wrote:
> BTW, your email client is possibly mis-configured - your mail
> came through with "From: shalini <shalini@imap.linux.ibm.com>"
> and attempting to reply to that gets an error saying that
> 'imap.linux.ibm.com' does not exist.
> 

Hello Daniel,
Thank you very much for pointing this out. I have fixed it.

> On Thu, Mar 06, 2025 at 02:55:27PM +0100, shalini wrote:
>> 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
> 
> Yes, that is correct from a QMP design POV. Data should formatted
> in the most appropriate way for machines to consume, using native
> JSON data types.
> 
>> But the desired output below is obtained only when interpreted as a 
>> str.
>> 
>> 'system-level': '0x10a000000060b00'
> 
> If a human wants to read the data in hex format, that should be
> formatted by whatever tool is consuming the data from QMP and
> presenting it in the user.
> 

Thank you for the explanation. I will change this.

>> > > +# @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.
> 
> Lets rename it to 'system-hostname' to be unambiguous.
> 

I was wrong. System name is not the hostname of the guest virtual 
machine. As mentioned by Nina it is a user configurable name of the 
system. So system-name cannot be change to 'system-hostname'. I am sorry 
for the inconvenience. Thank you.

> 
> With regards,
> Daniel
Shalini Chellathurai Saroja March 10, 2025, 3:16 p.m. UTC | #13
On 2025-03-06 17:05, Daniel P. Berrangé wrote:
> On Thu, Mar 06, 2025 at 04:44:33PM +0100, Nina Schoetterl-Glausch 
> wrote:
>> On Thu, 2025-03-06 at 15:55 +0100, Thomas Huth wrote:
>> > 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 @@
>> 
>> [...]
>> 
>> > > > > +##
>> > > > > +{ '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.
>> 
>> In a sense the system level is a bitfield.
>> So this could become a struct
>> 
>> {
>> 	'hypervisor-use' : true,
>> 	'distribution-id': 3,	// or an enum?
>> 	'distribution-version-major: 24,
>> 	...
>> }
> 
> Yes, if this is a mandatory format, breaking out the fields makes 
> sense.
> 
>> Not sure how to handle the 3 undefined bits in the highest byte.
> 
> If they're always left zero, might as well just omit them until
> such time as they have semantics defined (if ever)
> 

Hello Nina, Daniel,

The semantics as suggested by the split is applicable only if the 
system-type is LINUX. So, I will just change the system_level to int 
format.

Thank you.


> With regards,
> Daniel
diff mbox series

Patch

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
+    };
+
+    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",
+                              "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 "
+        "operating system");
+
 }
 
 static inline void s390_machine_initfn(Object *obj)
diff --git a/include/hw/s390x/s390-virtio-ccw.h b/include/hw/s390x/s390-virtio-ccw.h
index 686d9497d2..6872f7a176 100644
--- a/include/hw/s390x/s390-virtio-ccw.h
+++ b/include/hw/s390x/s390-virtio-ccw.h
@@ -19,6 +19,13 @@ 
 
 OBJECT_DECLARE_TYPE(S390CcwMachineState, S390CcwMachineClass, S390_CCW_MACHINE)
 
+typedef struct ControlProgramId {
+    uint8_t system_type[8];
+    uint8_t system_name[8];
+    uint64_t system_level;
+    uint8_t sysplex_name[8];
+    uint64_t timestamp;
+} QEMU_PACKED ControlProgramId;
 
 struct S390CcwMachineState {
     /*< private >*/
@@ -33,6 +40,7 @@  struct S390CcwMachineState {
     uint64_t max_pagesize;
 
     SCLPDevice *sclp;
+    ControlProgramId cpi;
 };
 
 static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
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.
+#
+# @system-type: operating system of Linux instance
+#
+# @system-name: system name of Linux 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
+##
+{ 'struct': 'S390ControlProgramId', 'data': {
+     'system-type': 'str',
+     'system-name': 'str',
+     'system-level': 'str',
+     'sysplex-name': 'str',
+     'timestamp': 'uint64' } }