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

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

[Shalini Chellathurai Saroja]

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);
+}
+
 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' } }
-- 
2.47.0

[PATCH qemu v2 2/3] hw/s390x: add SCLP event type CPI

[Shalini Chellathurai Saroja]

Implement the SCLP event type Control-Program Identification
(CPI) in QEMU. This event is used to send CPI identifiers,
which provide information about the guest OS to the host.

Save the information obtained from the SCLP CPI event to the
QOM along with the timestamp in which the data was received.

Example:
virsh # qemu-monitor-command vm --pretty '{
"execute":"qom-get","arguments": {
"path": "/machine", "property": "s390-control-program_id"}}'
{
  "return": {
    "timestamp": 1711620874948254000,
    "system-level": "0x50e00",
    "sysplex-name": "SYSPLEX ",
    "system-name": "TESTVM  ",
    "system-type": "LINUX   "
  },
  "id": "libvirt-15"
}

Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 hw/s390x/event-facility.c         |  12 +++-
 hw/s390x/meson.build              |   1 +
 hw/s390x/sclpcpi.c                | 105 ++++++++++++++++++++++++++++++
 include/hw/s390x/event-facility.h |   3 +
 4 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 hw/s390x/sclpcpi.c

diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
index 2b0332c20e..c0fb6e098c 100644
--- a/hw/s390x/event-facility.c
+++ b/hw/s390x/event-facility.c
@@ -39,7 +39,7 @@ typedef struct SCLPEventsBus {
 struct SCLPEventFacility {
     SysBusDevice parent_obj;
     SCLPEventsBus sbus;
-    SCLPEvent quiesce, cpu_hotplug;
+    SCLPEvent quiesce, cpu_hotplug, cpi;
     /* guest's receive mask */
     union {
         uint32_t receive_mask_pieces[2];
@@ -436,6 +436,10 @@ static void init_event_facility(Object *obj)
     object_initialize_child(obj, TYPE_SCLP_CPU_HOTPLUG,
                             &event_facility->cpu_hotplug,
                             TYPE_SCLP_CPU_HOTPLUG);
+
+    object_initialize_child(obj, TYPE_SCLP_CPI,
+                            &event_facility->cpi,
+                            TYPE_SCLP_CPI);
 }
 
 static void realize_event_facility(DeviceState *dev, Error **errp)
@@ -451,6 +455,12 @@ static void realize_event_facility(DeviceState *dev, Error **errp)
         qdev_unrealize(DEVICE(&event_facility->quiesce));
         return;
     }
+    if (!qdev_realize(DEVICE(&event_facility->cpi),
+                      BUS(&event_facility->sbus), errp)) {
+        qdev_unrealize(DEVICE(&event_facility->quiesce));
+        qdev_unrealize(DEVICE(&event_facility->cpu_hotplug));
+        return;
+    }
 }
 
 static void reset_event_facility(DeviceState *dev)
diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
index 3bbebfd817..eb7950489c 100644
--- a/hw/s390x/meson.build
+++ b/hw/s390x/meson.build
@@ -13,6 +13,7 @@ s390x_ss.add(files(
   's390-skeys.c',
   's390-stattrib.c',
   'sclp.c',
+  'sclpcpi.c',
   'sclpcpu.c',
   'sclpquiesce.c',
   'tod.c',
diff --git a/hw/s390x/sclpcpi.c b/hw/s390x/sclpcpi.c
new file mode 100644
index 0000000000..f2830d2d57
--- /dev/null
+++ b/hw/s390x/sclpcpi.c
@@ -0,0 +1,105 @@
+/*
+ * SCLP event type 11 - Control-Program Identification(CPI):
+ *    CPI is used to send program identifiers from the control-program to the
+ *    SCLP. The program identifiers provide data about the guest instance. It
+ *    is not sent by the SCLP.
+ *
+ *    The program identifiers are system type, system name, sysplex name and
+ *    system level. The system type, system name, and sysplex name use EBCDIC
+ *    ucharacters from this set: capital A-Z, 0-9, $, @, #, and blank. The
+ *    system level is a hex value.
+ *
+ * Copyright IBM, Corp. 2024
+ *
+ * Authors:
+ *  Shalini Chellathurai Saroja <shalini@linux.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or (at your
+ * option) any later version.  See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/timer.h"
+#include "hw/s390x/sclp.h"
+#include "hw/s390x/event-facility.h"
+#include "hw/s390x/ebcdic.h"
+#include "hw/s390x/s390-virtio-ccw.h"
+
+typedef struct Data {
+    uint8_t id_format;
+    uint8_t reserved0;
+    uint8_t system_type[8];
+    uint64_t reserved1;
+    uint8_t system_name[8];
+    uint64_t reserved2;
+    uint64_t system_level;
+    uint64_t reserved3;
+    uint8_t sysplex_name[8];
+    uint8_t reserved4[16];
+} QEMU_PACKED Data;
+
+typedef struct ControlProgramIdMsg {
+    EventBufferHeader ebh;
+    Data data;
+} QEMU_PACKED ControlProgramIdMsg;
+
+static bool can_handle_event(uint8_t type)
+{
+    return type == SCLP_EVENT_CPI;
+}
+
+static sccb_mask_t send_mask(void)
+{
+    return 0;
+}
+
+/* Enable SCLP to accept buffers of event type CPI from the control-program. */
+static sccb_mask_t receive_mask(void)
+{
+    return SCLP_EVENT_MASK_CPI;
+}
+
+static int write_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr)
+{
+    ControlProgramIdMsg *cpi = container_of(evt_buf_hdr, ControlProgramIdMsg,
+                                            ebh);
+    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
+
+    ascii_put(ms->cpi.system_type, (char *)cpi->data.system_type, 8);
+    ascii_put(ms->cpi.system_name, (char *)cpi->data.system_name, 8);
+    ascii_put(ms->cpi.sysplex_name, (char *)cpi->data.sysplex_name, 8);
+    ms->cpi.system_level = be64_to_cpu(cpi->data.system_level);
+    ms->cpi.timestamp = qemu_clock_get_ns(QEMU_CLOCK_HOST);
+
+    cpi->ebh.flags = SCLP_EVENT_BUFFER_ACCEPTED;
+    return SCLP_RC_NORMAL_COMPLETION;
+}
+
+static void cpi_class_init(ObjectClass *klass, void *data)
+{
+    SCLPEventClass *sclp_cpi = SCLP_EVENT_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    sclp_cpi->can_handle_event = can_handle_event;
+    sclp_cpi->get_send_mask = send_mask;
+    sclp_cpi->get_receive_mask = receive_mask;
+    sclp_cpi->write_event_data = write_event_data;
+    dc->user_creatable = false;
+}
+
+static const TypeInfo sclp_cpi_info = {
+    .name          = TYPE_SCLP_CPI,
+    .parent        = TYPE_SCLP_EVENT,
+    .instance_size = sizeof(SCLPEvent),
+    .class_init    = cpi_class_init,
+    .class_size    = sizeof(SCLPEventClass),
+};
+
+static void sclp_cpi_register_types(void)
+{
+    type_register_static(&sclp_cpi_info);
+}
+
+type_init(sclp_cpi_register_types)
+
diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
index ff874e792d..f445d2f9f5 100644
--- a/include/hw/s390x/event-facility.h
+++ b/include/hw/s390x/event-facility.h
@@ -25,6 +25,7 @@
 #define SCLP_EVENT_MESSAGE                      0x02
 #define SCLP_EVENT_CONFIG_MGT_DATA              0x04
 #define SCLP_EVENT_PMSGCMD                      0x09
+#define SCLP_EVENT_CPI                          0x0b
 #define SCLP_EVENT_ASCII_CONSOLE_DATA           0x1a
 #define SCLP_EVENT_SIGNAL_QUIESCE               0x1d
 
@@ -35,6 +36,7 @@
 #define SCLP_EVENT_MASK_MSG             SCLP_EVMASK(SCLP_EVENT_MESSAGE)
 #define SCLP_EVENT_MASK_CONFIG_MGT_DATA SCLP_EVMASK(SCLP_EVENT_CONFIG_MGT_DATA)
 #define SCLP_EVENT_MASK_PMSGCMD         SCLP_EVMASK(SCLP_EVENT_PMSGCMD)
+#define SCLP_EVENT_MASK_CPI             SCLP_EVMASK(SCLP_EVENT_CPI)
 #define SCLP_EVENT_MASK_MSG_ASCII       SCLP_EVMASK(SCLP_EVENT_ASCII_CONSOLE_DATA)
 #define SCLP_EVENT_MASK_SIGNAL_QUIESCE  SCLP_EVMASK(SCLP_EVENT_SIGNAL_QUIESCE)
 
@@ -46,6 +48,7 @@ OBJECT_DECLARE_TYPE(SCLPEvent, SCLPEventClass,
                     SCLP_EVENT)
 
 #define TYPE_SCLP_CPU_HOTPLUG "sclp-cpu-hotplug"
+#define TYPE_SCLP_CPI "sclpcpi"
 #define TYPE_SCLP_QUIESCE "sclpquiesce"
 
 #define SCLP_EVENT_MASK_LEN_MAX 1021
-- 
2.47.0

Re: [PATCH qemu v2 2/3] hw/s390x: add SCLP event type CPI

[Thomas Huth]

On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
> Implement the SCLP event type Control-Program Identification
> (CPI) in QEMU. This event is used to send CPI identifiers,
> which provide information about the guest OS to the host.
> 
> Save the information obtained from the SCLP CPI event to the
> QOM along with the timestamp in which the data was received.
> 
> Example:
> virsh # qemu-monitor-command vm --pretty '{
> "execute":"qom-get","arguments": {
> "path": "/machine", "property": "s390-control-program_id"}}'

I guess it should be a "-" instead of a "_" between "program" and "id"?

> {
>    "return": {
>      "timestamp": 1711620874948254000,
>      "system-level": "0x50e00",
>      "sysplex-name": "SYSPLEX ",
>      "system-name": "TESTVM  ",
>      "system-type": "LINUX   "
>    },
>    "id": "libvirt-15"
> }
> 
> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>   hw/s390x/event-facility.c         |  12 +++-
>   hw/s390x/meson.build              |   1 +
>   hw/s390x/sclpcpi.c                | 105 ++++++++++++++++++++++++++++++
>   include/hw/s390x/event-facility.h |   3 +
>   4 files changed, 120 insertions(+), 1 deletion(-)
>   create mode 100644 hw/s390x/sclpcpi.c
> 
> diff --git a/hw/s390x/event-facility.c b/hw/s390x/event-facility.c
> index 2b0332c20e..c0fb6e098c 100644
> --- a/hw/s390x/event-facility.c
> +++ b/hw/s390x/event-facility.c
> @@ -39,7 +39,7 @@ typedef struct SCLPEventsBus {
>   struct SCLPEventFacility {
>       SysBusDevice parent_obj;
>       SCLPEventsBus sbus;
> -    SCLPEvent quiesce, cpu_hotplug;
> +    SCLPEvent quiesce, cpu_hotplug, cpi;
>       /* guest's receive mask */
>       union {
>           uint32_t receive_mask_pieces[2];
> @@ -436,6 +436,10 @@ static void init_event_facility(Object *obj)
>       object_initialize_child(obj, TYPE_SCLP_CPU_HOTPLUG,
>                               &event_facility->cpu_hotplug,
>                               TYPE_SCLP_CPU_HOTPLUG);
> +
> +    object_initialize_child(obj, TYPE_SCLP_CPI,
> +                            &event_facility->cpi,
> +                            TYPE_SCLP_CPI);
>   }
>   
>   static void realize_event_facility(DeviceState *dev, Error **errp)
> @@ -451,6 +455,12 @@ static void realize_event_facility(DeviceState *dev, Error **errp)
>           qdev_unrealize(DEVICE(&event_facility->quiesce));
>           return;
>       }
> +    if (!qdev_realize(DEVICE(&event_facility->cpi),
> +                      BUS(&event_facility->sbus), errp)) {
> +        qdev_unrealize(DEVICE(&event_facility->quiesce));
> +        qdev_unrealize(DEVICE(&event_facility->cpu_hotplug));
> +        return;
> +    }
>   }
>   
>   static void reset_event_facility(DeviceState *dev)
> diff --git a/hw/s390x/meson.build b/hw/s390x/meson.build
> index 3bbebfd817..eb7950489c 100644
> --- a/hw/s390x/meson.build
> +++ b/hw/s390x/meson.build
> @@ -13,6 +13,7 @@ s390x_ss.add(files(
>     's390-skeys.c',
>     's390-stattrib.c',
>     'sclp.c',
> +  'sclpcpi.c',
>     'sclpcpu.c',
>     'sclpquiesce.c',
>     'tod.c',
> diff --git a/hw/s390x/sclpcpi.c b/hw/s390x/sclpcpi.c
> new file mode 100644
> index 0000000000..f2830d2d57
> --- /dev/null
> +++ b/hw/s390x/sclpcpi.c
> @@ -0,0 +1,105 @@
> +/*

scripts/checkpatch.pl recently learnt a new check to mandate SPDX license 
tags now for new files ... so please add one here now!

> + * SCLP event type 11 - Control-Program Identification(CPI):
> + *    CPI is used to send program identifiers from the control-program to the
> + *    SCLP. The program identifiers provide data about the guest instance. It
> + *    is not sent by the SCLP.
> + *
> + *    The program identifiers are system type, system name, sysplex name and
> + *    system level. The system type, system name, and sysplex name use EBCDIC
> + *    ucharacters from this set: capital A-Z, 0-9, $, @, #, and blank. The
> + *    system level is a hex value.
> + *
> + * Copyright IBM, Corp. 2024
> + *
> + * Authors:
> + *  Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or (at your
> + * option) any later version.  See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/timer.h"
> +#include "hw/s390x/sclp.h"
> +#include "hw/s390x/event-facility.h"
> +#include "hw/s390x/ebcdic.h"
> +#include "hw/s390x/s390-virtio-ccw.h"
> +
> +typedef struct Data {
> +    uint8_t id_format;
> +    uint8_t reserved0;
> +    uint8_t system_type[8];
> +    uint64_t reserved1;
> +    uint8_t system_name[8];
> +    uint64_t reserved2;
> +    uint64_t system_level;
> +    uint64_t reserved3;
> +    uint8_t sysplex_name[8];
> +    uint8_t reserved4[16];
> +} QEMU_PACKED Data;
> +
> +typedef struct ControlProgramIdMsg {
> +    EventBufferHeader ebh;
> +    Data data;
> +} QEMU_PACKED ControlProgramIdMsg;
> +
> +static bool can_handle_event(uint8_t type)
> +{
> +    return type == SCLP_EVENT_CPI;
> +}
> +
> +static sccb_mask_t send_mask(void)
> +{
> +    return 0;
> +}
> +
> +/* Enable SCLP to accept buffers of event type CPI from the control-program. */
> +static sccb_mask_t receive_mask(void)
> +{
> +    return SCLP_EVENT_MASK_CPI;
> +}
> +
> +static int write_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr)
> +{
> +    ControlProgramIdMsg *cpi = container_of(evt_buf_hdr, ControlProgramIdMsg,
> +                                            ebh);
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> +
> +    ascii_put(ms->cpi.system_type, (char *)cpi->data.system_type, 8);
> +    ascii_put(ms->cpi.system_name, (char *)cpi->data.system_name, 8);
> +    ascii_put(ms->cpi.sysplex_name, (char *)cpi->data.sysplex_name, 8);
> +    ms->cpi.system_level = be64_to_cpu(cpi->data.system_level);

Can we be confident that system_level is always properly aligned?
Otherwise it's maybe better to use ldq_be_p() instead?

  Thomas

Re: [PATCH qemu v2 2/3] hw/s390x: add SCLP event type CPI

[Nina Schoetterl-Glausch]

On Wed, 2025-03-05 at 19:04 +0100, Thomas Huth wrote:

[...]

> > +
> > +static int write_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr)
> > +{
> > +    ControlProgramIdMsg *cpi = container_of(evt_buf_hdr, ControlProgramIdMsg,
> > +                                            ebh);
> > +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
> > +
> > +    ascii_put(ms->cpi.system_type, (char *)cpi->data.system_type, 8);
> > +    ascii_put(ms->cpi.system_name, (char *)cpi->data.system_name, 8);
> > +    ascii_put(ms->cpi.sysplex_name, (char *)cpi->data.sysplex_name, 8);
> > +    ms->cpi.system_level = be64_to_cpu(cpi->data.system_level);
> 
> Can we be confident that system_level is always properly aligned?

It has offset 40 from the start of a heap allocation, so yes, but it is far
from obvious so I wouldn't mind ldq_be_p.

> Otherwise it's maybe better to use ldq_be_p() instead?
> 
>   Thomas
> 
> 

-- 
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH qemu v2 2/3] hw/s390x: add SCLP event type CPI

[shalini]

On 2025-03-05 20:00, Nina Schoetterl-Glausch wrote:
> On Wed, 2025-03-05 at 19:04 +0100, Thomas Huth wrote:
> 
> [...]
> 
>> > +
>> > +static int write_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr)
>> > +{
>> > +    ControlProgramIdMsg *cpi = container_of(evt_buf_hdr, ControlProgramIdMsg,
>> > +                                            ebh);
>> > +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
>> > +
>> > +    ascii_put(ms->cpi.system_type, (char *)cpi->data.system_type, 8);
>> > +    ascii_put(ms->cpi.system_name, (char *)cpi->data.system_name, 8);
>> > +    ascii_put(ms->cpi.sysplex_name, (char *)cpi->data.sysplex_name, 8);
>> > +    ms->cpi.system_level = be64_to_cpu(cpi->data.system_level);
>> 
>> Can we be confident that system_level is always properly aligned?
> 
> It has offset 40 from the start of a heap allocation, so yes, but it is 
> far
> from obvious so I wouldn't mind ldq_be_p.
> 

ok, I will use ldc_be_p() instead, thank you Nina.

>> Otherwise it's maybe better to use ldq_be_p() instead?
>> 
>>   Thomas
>> 
>> 

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

Re: [PATCH qemu v2 2/3] hw/s390x: add SCLP event type CPI

[shalini]

On 2025-03-05 19:04, Thomas Huth wrote:
> On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
>> Implement the SCLP event type Control-Program Identification
>> (CPI) in QEMU. This event is used to send CPI identifiers,
>> which provide information about the guest OS to the host.
>> 
>> Save the information obtained from the SCLP CPI event to the
>> QOM along with the timestamp in which the data was received.
>> 
>> Example:
>> virsh # qemu-monitor-command vm --pretty '{
>> "execute":"qom-get","arguments": {
>> "path": "/machine", "property": "s390-control-program_id"}}'
> 
> I guess it should be a "-" instead of a "_" between "program" and "id"?

Hello Thomas,

Thank you very much for the review. Yes, that is correct. I will change 
it.

[...]

>> diff --git a/hw/s390x/sclpcpi.c b/hw/s390x/sclpcpi.c
>> new file mode 100644
>> index 0000000000..f2830d2d57
>> --- /dev/null
>> +++ b/hw/s390x/sclpcpi.c
>> @@ -0,0 +1,105 @@
>> +/*
> 
> scripts/checkpatch.pl recently learnt a new check to mandate SPDX
> license tags now for new files ... so please add one here now!
> 

ok.

[...]

>> +static int write_event_data(SCLPEvent *event, EventBufferHeader 
>> *evt_buf_hdr)
>> +{
>> +    ControlProgramIdMsg *cpi = container_of(evt_buf_hdr, 
>> ControlProgramIdMsg,
>> +                                            ebh);
>> +    S390CcwMachineState *ms = S390_CCW_MACHINE(qdev_get_machine());
>> +
>> +    ascii_put(ms->cpi.system_type, (char *)cpi->data.system_type, 8);
>> +    ascii_put(ms->cpi.system_name, (char *)cpi->data.system_name, 8);
>> +    ascii_put(ms->cpi.sysplex_name, (char *)cpi->data.sysplex_name, 
>> 8);
>> +    ms->cpi.system_level = be64_to_cpu(cpi->data.system_level);
> 
> Can we be confident that system_level is always properly aligned?
> Otherwise it's maybe better to use ldq_be_p() instead?

ok, I will use ldq_be_p() instead.

> 
>  Thomas

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

[PATCH qemu v2 3/3] hw/s390x: support migration of CPI values

[Shalini Chellathurai Saroja]

Register Control-Program Identification data with the live
migration infrastructure.

Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
---
 hw/s390x/s390-virtio-ccw.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index 13ea8db1b0..4d0838d037 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -260,6 +260,20 @@ static void s390_create_sclpconsole(SCLPDevice *sclp,
     qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
 }
 
+static const VMStateDescription vmstate_control_program_id = {
+    .name = "s390_control_program_id",
+    .version_id = 0,
+    .minimum_version_id = 0,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT8_ARRAY(system_type, ControlProgramId, 8),
+        VMSTATE_UINT8_ARRAY(system_name, ControlProgramId, 8),
+        VMSTATE_UINT64(system_level, ControlProgramId),
+        VMSTATE_UINT8_ARRAY(sysplex_name, ControlProgramId, 8),
+        VMSTATE_UINT64(timestamp, ControlProgramId),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void ccw_init(MachineState *machine)
 {
     MachineClass *mc = MACHINE_GET_CLASS(machine);
@@ -308,6 +322,9 @@ static void ccw_init(MachineState *machine)
     ret = css_create_css_image(VIRTUAL_CSSID, true);
     assert(ret == 0);
 
+    /* register CPI values */
+    vmstate_register_any(NULL, &vmstate_control_program_id, &ms->cpi);
+
     css_register_vmstate();
 
     /* Create VirtIO network adapters */
-- 
2.47.0

Re: [PATCH qemu v2 3/3] hw/s390x: support migration of CPI values

[Thomas Huth]

On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
> Register Control-Program Identification data with the live
> migration infrastructure.
> 
> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> ---
>   hw/s390x/s390-virtio-ccw.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 13ea8db1b0..4d0838d037 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -260,6 +260,20 @@ static void s390_create_sclpconsole(SCLPDevice *sclp,
>       qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>   }
>   
> +static const VMStateDescription vmstate_control_program_id = {
> +    .name = "s390_control_program_id",
> +    .version_id = 0,
> +    .minimum_version_id = 0,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT8_ARRAY(system_type, ControlProgramId, 8),
> +        VMSTATE_UINT8_ARRAY(system_name, ControlProgramId, 8),
> +        VMSTATE_UINT64(system_level, ControlProgramId),
> +        VMSTATE_UINT8_ARRAY(sysplex_name, ControlProgramId, 8),
> +        VMSTATE_UINT64(timestamp, ControlProgramId),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static void ccw_init(MachineState *machine)
>   {
>       MachineClass *mc = MACHINE_GET_CLASS(machine);
> @@ -308,6 +322,9 @@ static void ccw_init(MachineState *machine)
>       ret = css_create_css_image(VIRTUAL_CSSID, true);
>       assert(ret == 0);
>   
> +    /* register CPI values */
> +    vmstate_register_any(NULL, &vmstate_control_program_id, &ms->cpi);

  Hi again,

after looking at this for a while, I think it might be cleaner to store the 
state in the TYPE_SCLP_CPI device instead of storing it in the machine 
state. Then you can also use dc->vmsd there instead of using the legacy 
vmstate_register_any() function.

Additionally, I think you need some compat handling for backward migration 
in your patches. E.g. have you tried migrating from an old version of QEMU 
to a newer one (that includes your patches) and then back to the old one?
I think the TYPE_SCLP_CPI device should only be instantiated for the machine 
types >= 10.0, but not for the older machine types, e.g. by introducing a 
"use-cpi" property to the TYPE_SCLP_EVENT_FACILITY (set to true by default). 
Then in ccw_machine_9_2_class_options(), make sure that this property gets 
switched to "off" again, so that older machine types don't have the new 
TYPE_SCLP_CPI device. WDYT?

  Thomas

Re: [PATCH qemu v2 3/3] hw/s390x: support migration of CPI values

[shalini]

On 2025-03-05 19:33, Thomas Huth wrote:
> On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
>> Register Control-Program Identification data with the live
>> migration infrastructure.
>> 
>> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 13ea8db1b0..4d0838d037 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -260,6 +260,20 @@ static void s390_create_sclpconsole(SCLPDevice 
>> *sclp,
>>       qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>>   }
>>   +static const VMStateDescription vmstate_control_program_id = {
>> +    .name = "s390_control_program_id",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (const VMStateField[]) {
>> +        VMSTATE_UINT8_ARRAY(system_type, ControlProgramId, 8),
>> +        VMSTATE_UINT8_ARRAY(system_name, ControlProgramId, 8),
>> +        VMSTATE_UINT64(system_level, ControlProgramId),
>> +        VMSTATE_UINT8_ARRAY(sysplex_name, ControlProgramId, 8),
>> +        VMSTATE_UINT64(timestamp, ControlProgramId),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void ccw_init(MachineState *machine)
>>   {
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>> @@ -308,6 +322,9 @@ static void ccw_init(MachineState *machine)
>>       ret = css_create_css_image(VIRTUAL_CSSID, true);
>>       assert(ret == 0);
>>   +    /* register CPI values */
>> +    vmstate_register_any(NULL, &vmstate_control_program_id, 
>> &ms->cpi);
> 
>  Hi again,
> 
> after looking at this for a while, I think it might be cleaner to
> store the state in the TYPE_SCLP_CPI device instead of storing it in
> the machine state. Then you can also use dc->vmsd there instead of
> using the legacy vmstate_register_any() function.
> 

Hello Thomas,

The SCLP event type CPI is used to transfer the data mentioned below 
from guest operating system to QEMU. The job of the CPI event is 
complete, when this transfer is done.

The received data must be saved in the QEMU object model, so that it is 
possible for the user to retrieve the data via QMP. The received data 
provides information about the guest virtual machine.

System type - Operating system of the guest (eg: LINUX)
System name - Hostname provided to the guest (eg: TESTVM)
System level - Kernel version of the guest operating system (eg: 
0x50e00)
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)

In my perspective, I believe that the s390 machine state is appropriate 
for storing the data about the guest virtual machine. Please do let me 
know if you still want to change this?, thank you.

> Additionally, I think you need some compat handling for backward
> migration in your patches. E.g. have you tried migrating from an old
> version of QEMU to a newer one (that includes your patches) and then
> back to the old one?
> I think the TYPE_SCLP_CPI device should only be instantiated for the
> machine types >= 10.0, but not for the older machine types, e.g. by
> introducing a "use-cpi" property to the TYPE_SCLP_EVENT_FACILITY (set
> to true by default). Then in ccw_machine_9_2_class_options(), make
> sure that this property gets switched to "off" again, so that older
> machine types don't have the new TYPE_SCLP_CPI device. WDYT?
> 

I will answer this shortly. Thank you.

>  Thomas

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

Re: [PATCH qemu v2 3/3] hw/s390x: support migration of CPI values

[Thomas Huth]

On 06/03/2025 15.10, shalini wrote:
> On 2025-03-05 19:33, Thomas Huth wrote:
>> On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
>>> Register Control-Program Identification data with the live
>>> migration infrastructure.
>>>
>>> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
>>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>> ---
>>>   hw/s390x/s390-virtio-ccw.c | 17 +++++++++++++++++
>>>   1 file changed, 17 insertions(+)
>>>
>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>> index 13ea8db1b0..4d0838d037 100644
>>> --- a/hw/s390x/s390-virtio-ccw.c
>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>> @@ -260,6 +260,20 @@ static void s390_create_sclpconsole(SCLPDevice *sclp,
>>>       qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>>>   }
>>>   +static const VMStateDescription vmstate_control_program_id = {
>>> +    .name = "s390_control_program_id",
>>> +    .version_id = 0,
>>> +    .minimum_version_id = 0,
>>> +    .fields = (const VMStateField[]) {
>>> +        VMSTATE_UINT8_ARRAY(system_type, ControlProgramId, 8),
>>> +        VMSTATE_UINT8_ARRAY(system_name, ControlProgramId, 8),
>>> +        VMSTATE_UINT64(system_level, ControlProgramId),
>>> +        VMSTATE_UINT8_ARRAY(sysplex_name, ControlProgramId, 8),
>>> +        VMSTATE_UINT64(timestamp, ControlProgramId),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>>   static void ccw_init(MachineState *machine)
>>>   {
>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>> @@ -308,6 +322,9 @@ static void ccw_init(MachineState *machine)
>>>       ret = css_create_css_image(VIRTUAL_CSSID, true);
>>>       assert(ret == 0);
>>>   +    /* register CPI values */
>>> +    vmstate_register_any(NULL, &vmstate_control_program_id, &ms->cpi);
>>
>>  Hi again,
>>
>> after looking at this for a while, I think it might be cleaner to
>> store the state in the TYPE_SCLP_CPI device instead of storing it in
>> the machine state. Then you can also use dc->vmsd there instead of
>> using the legacy vmstate_register_any() function.
>>
> 
> Hello Thomas,
> 
> The SCLP event type CPI is used to transfer the data mentioned below from 
> guest operating system to QEMU. The job of the CPI event is complete, when 
> this transfer is done.
> 
> The received data must be saved in the QEMU object model, so that it is 
> possible for the user to retrieve the data via QMP. The received data 
> provides information about the guest virtual machine.
> 
> System type - Operating system of the guest (eg: LINUX)
> System name - Hostname provided to the guest (eg: TESTVM)
> System level - Kernel version of the guest operating system (eg: 0x50e00)
> 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)
> 
> In my perspective, I believe that the s390 machine state is appropriate for 
> storing the data about the guest virtual machine. Please do let me know if 
> you still want to change this?, thank you.

  Hi Shalini,

yes, please change this patch to put the migration state into the SCLP_CPI 
device.

vmstate_register_any() is a legacy function, see its description in 
include/migration/vmstate.h ... so if anyhow possible, this should be 
avoided in new code.

You introduced a new device in patch 2 which will be handling the related 
data. Devices are the right place for storing migration data in new QEMU 
code, so I think this really should go into dc->vmsd of TYPE_SCLP_CPI.

For retrieving the information, the user should then just get the property 
from /machine/sclp/s390-sclp-event-facility/sclpcpi instead of /machine. 
This should be fine since it is a well-defined location, too.

  Thanks!
   Thomas

Re: [PATCH qemu v2 3/3] hw/s390x: support migration of CPI values

[Shalini Chellathurai Saroja]

On 2025-03-06 16:08, Thomas Huth wrote:
> On 06/03/2025 15.10, shalini wrote:
>> On 2025-03-05 19:33, Thomas Huth wrote:
>>> On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
>>>> Register Control-Program Identification data with the live
>>>> migration infrastructure.
>>>> 
>>>> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
>>>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>>>> ---
>>>>   hw/s390x/s390-virtio-ccw.c | 17 +++++++++++++++++
>>>>   1 file changed, 17 insertions(+)
>>>> 
>>>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>>>> index 13ea8db1b0..4d0838d037 100644
>>>> --- a/hw/s390x/s390-virtio-ccw.c
>>>> +++ b/hw/s390x/s390-virtio-ccw.c
>>>> @@ -260,6 +260,20 @@ static void s390_create_sclpconsole(SCLPDevice 
>>>> *sclp,
>>>>       qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>>>>   }
>>>>   +static const VMStateDescription vmstate_control_program_id = {
>>>> +    .name = "s390_control_program_id",
>>>> +    .version_id = 0,
>>>> +    .minimum_version_id = 0,
>>>> +    .fields = (const VMStateField[]) {
>>>> +        VMSTATE_UINT8_ARRAY(system_type, ControlProgramId, 8),
>>>> +        VMSTATE_UINT8_ARRAY(system_name, ControlProgramId, 8),
>>>> +        VMSTATE_UINT64(system_level, ControlProgramId),
>>>> +        VMSTATE_UINT8_ARRAY(sysplex_name, ControlProgramId, 8),
>>>> +        VMSTATE_UINT64(timestamp, ControlProgramId),
>>>> +        VMSTATE_END_OF_LIST()
>>>> +    }
>>>> +};
>>>> +
>>>>   static void ccw_init(MachineState *machine)
>>>>   {
>>>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>>>> @@ -308,6 +322,9 @@ static void ccw_init(MachineState *machine)
>>>>       ret = css_create_css_image(VIRTUAL_CSSID, true);
>>>>       assert(ret == 0);
>>>>   +    /* register CPI values */
>>>> +    vmstate_register_any(NULL, &vmstate_control_program_id, 
>>>> &ms->cpi);
>>> 
>>>  Hi again,
>>> 
>>> after looking at this for a while, I think it might be cleaner to
>>> store the state in the TYPE_SCLP_CPI device instead of storing it in
>>> the machine state. Then you can also use dc->vmsd there instead of
>>> using the legacy vmstate_register_any() function.
>>> 
>> 
>> Hello Thomas,
>> 
>> The SCLP event type CPI is used to transfer the data mentioned below 
>> from guest operating system to QEMU. The job of the CPI event is 
>> complete, when this transfer is done.
>> 
>> The received data must be saved in the QEMU object model, so that it 
>> is possible for the user to retrieve the data via QMP. The received 
>> data provides information about the guest virtual machine.
>> 
>> System type - Operating system of the guest (eg: LINUX)
>> System name - Hostname provided to the guest (eg: TESTVM)
>> System level - Kernel version of the guest operating system (eg: 
>> 0x50e00)
>> 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)
>> 
>> In my perspective, I believe that the s390 machine state is 
>> appropriate for storing the data about the guest virtual machine. 
>> Please do let me know if you still want to change this?, thank you.
> 
>  Hi Shalini,
> 
> yes, please change this patch to put the migration state into the
> SCLP_CPI device.
> 
> vmstate_register_any() is a legacy function, see its description in
> include/migration/vmstate.h ... so if anyhow possible, this should be
> avoided in new code.
> 
> You introduced a new device in patch 2 which will be handling the
> related data. Devices are the right place for storing migration data
> in new QEMU code, so I think this really should go into dc->vmsd of
> TYPE_SCLP_CPI.
> 
> For retrieving the information, the user should then just get the
> property from /machine/sclp/s390-sclp-event-facility/sclpcpi instead
> of /machine. This should be fine since it is a well-defined location,
> too.
> 

Good Morning Thomas,

Thank you very much for the explanation. I will change this.

>  Thanks!
>   Thomas

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

Re: [PATCH qemu v2 3/3] hw/s390x: support migration of CPI values

[Shalini Chellathurai Saroja]

On 2025-03-05 19:33, Thomas Huth wrote:
> On 24/02/2025 13.04, Shalini Chellathurai Saroja wrote:
>> Register Control-Program Identification data with the live
>> migration infrastructure.
>> 
>> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
>> Reviewed-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
>> ---
>>   hw/s390x/s390-virtio-ccw.c | 17 +++++++++++++++++
>>   1 file changed, 17 insertions(+)
>> 
>> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
>> index 13ea8db1b0..4d0838d037 100644
>> --- a/hw/s390x/s390-virtio-ccw.c
>> +++ b/hw/s390x/s390-virtio-ccw.c
>> @@ -260,6 +260,20 @@ static void s390_create_sclpconsole(SCLPDevice 
>> *sclp,
>>       qdev_realize_and_unref(dev, ev_fac_bus, &error_fatal);
>>   }
>>   +static const VMStateDescription vmstate_control_program_id = {
>> +    .name = "s390_control_program_id",
>> +    .version_id = 0,
>> +    .minimum_version_id = 0,
>> +    .fields = (const VMStateField[]) {
>> +        VMSTATE_UINT8_ARRAY(system_type, ControlProgramId, 8),
>> +        VMSTATE_UINT8_ARRAY(system_name, ControlProgramId, 8),
>> +        VMSTATE_UINT64(system_level, ControlProgramId),
>> +        VMSTATE_UINT8_ARRAY(sysplex_name, ControlProgramId, 8),
>> +        VMSTATE_UINT64(timestamp, ControlProgramId),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>>   static void ccw_init(MachineState *machine)
>>   {
>>       MachineClass *mc = MACHINE_GET_CLASS(machine);
>> @@ -308,6 +322,9 @@ static void ccw_init(MachineState *machine)
>>       ret = css_create_css_image(VIRTUAL_CSSID, true);
>>       assert(ret == 0);
>>   +    /* register CPI values */
>> +    vmstate_register_any(NULL, &vmstate_control_program_id, 
>> &ms->cpi);
> 
>  Hi again,
> 
> after looking at this for a while, I think it might be cleaner to
> store the state in the TYPE_SCLP_CPI device instead of storing it in
> the machine state. Then you can also use dc->vmsd there instead of
> using the legacy vmstate_register_any() function.
> 
> Additionally, I think you need some compat handling for backward
> migration in your patches. E.g. have you tried migrating from an old
> version of QEMU to a newer one (that includes your patches) and then
> back to the old one?
> I think the TYPE_SCLP_CPI device should only be instantiated for the
> machine types >= 10.0, but not for the older machine types, e.g. by
> introducing a "use-cpi" property to the TYPE_SCLP_EVENT_FACILITY (set
> to true by default). Then in ccw_machine_9_2_class_options(), make
> sure that this property gets switched to "off" again, so that older
> machine types don't have the new TYPE_SCLP_CPI device. WDYT?
> 
Hello Thomas,
That is correct. I will change this as per your suggestion. Thank you.

>  Thomas

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

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

[Thomas Huth]

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

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

[shalini]

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

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

[Thomas Huth]

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

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

[Nina Schoetterl-Glausch]

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
> 

-- 
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Shalini Chellathurai Saroja]

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

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

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[shalini]

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

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

[Daniel P. Berrangé]

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
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Nina Schoetterl-Glausch]

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

-- 
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

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

[Shalini Chellathurai Saroja]

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

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

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

[Thomas Huth]

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

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

[shalini]

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

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