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

[Thomas Huth <thuth@redhat.com>]

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