Re: [PATCH v1 2/3] hw/s390x: add CPI values to QOM

[Nina Schoetterl-Glausch]

On Wed, 2025-01-15 at 14:31 +0100, Shalini Chellathurai Saroja wrote:
> This commit adds the firmware control-program
> identifiers received from a KVM guest via the
> SCLP event type Control-Program Identification to QOM.
> A timestamp in which the data is received is also
> added to QOM.
> 
> Example:
> virsh # qemu-monitor-command vm --pretty '{
> "execute":"qom-get","arguments": {
> "path": "/machine", "property": "s390-cpi"}}'
> {
>   "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>
> ---
>  hw/s390x/s390-virtio-ccw.c         | 26 ++++++++++++++++++++++++++
>  hw/s390x/sclpcpi.c                 | 10 ++++++++++
>  include/hw/s390x/s390-virtio-ccw.h |  8 ++++++++
>  qapi/machine.json                  | 24 ++++++++++++++++++++++++
>  4 files changed, 68 insertions(+)
> 
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index 38aeba14ee..35fb523af9 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -49,6 +49,7 @@
>  #include "hw/virtio/virtio-md-pci.h"
>  #include "hw/s390x/virtio-ccw-md.h"
>  #include CONFIG_DEVICES
> +#include "qapi/qapi-visit-machine.h"
>  
>  static Error *pv_mig_blocker;
>  
> @@ -773,6 +774,25 @@ static void machine_set_loadparm(Object *obj, Visitor *v,
>      s390_ipl_fmt_loadparm(ms->loadparm, val, errp);
>  }
>  
> +static void machine_get_cpi(Object *obj, Visitor *v,
> +                             const char *name, void *opaque, Error **errp)
> +{
> +    S390CcwMachineState *ms = S390_CCW_MACHINE(obj);
> +    S390Cpi *cpi;
> +    cpi = &(S390Cpi){
> +        .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),

Is there any way in which it would make sense for the qmp caller to
interpret this as a number? If so exposing it as a number would be preferable.

> +        .sysplex_name = g_strndup((char *) ms->cpi.sysplex_name,
> +                        sizeof(ms->cpi.sysplex_name)),
> +        .timestamp = ms->cpi.timestamp
> +    };
> +
> +    visit_type_S390Cpi(v, name, &cpi, &error_abort);
> +}
> +
>  static void ccw_machine_class_init(ObjectClass *oc, void *data)
>  {
>      MachineClass *mc = MACHINE_CLASS(oc);
> @@ -826,6 +846,12 @@ 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-cpi", "S390Cpi",
> +        machine_get_cpi, NULL, NULL, NULL);
> +    object_class_property_set_description(oc, "s390-cpi",
> +        "Control-progam identifiers provide data about the guest "
> +        "operating system");
> +
>  }
>  
>  static inline void s390_machine_initfn(Object *obj)
> diff --git a/hw/s390x/sclpcpi.c b/hw/s390x/sclpcpi.c
> index 64bc730f47..75101b8f61 100644
> --- a/hw/s390x/sclpcpi.c
> +++ b/hw/s390x/sclpcpi.c
> @@ -20,8 +20,11 @@
>   */
>  
>  #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;
> @@ -60,6 +63,13 @@ static sccb_mask_t receive_mask(void)
>  static int write_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr)
>  {
>      CPI *cpi = container_of(evt_buf_hdr, CPI, 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 = cpi->data.system_level;

cpi is overlayed with the sccb copied from the guest, correct?
So you need a endianess conversion here.

> +    ms->cpi.timestamp = qemu_clock_get_ns(QEMU_CLOCK_HOST);
>  
>      cpi->ebh.flags = SCLP_EVENT_BUFFER_ACCEPTED;
>      return SCLP_RC_NORMAL_COMPLETION;
> diff --git a/include/hw/s390x/s390-virtio-ccw.h 
> b/include/hw/s390x/s390-virtio-ccw.h
> index 686d9497d2..c4212ff857 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 Cpi {
> +    uint8_t system_type[8];
> +    uint8_t system_name[8];
> +    uint64_t system_level;
> +    uint8_t sysplex_name[8];
> +    uint64_t timestamp;
> +} QEMU_PACKED Cpi;

I don't like there being a CPI and a Cpi struct. I'd just go ahead and inline 
the
definition into S390CcwMachineState and not name this type.
>  
>  struct S390CcwMachineState {
>      /*< private >*/
> @@ -33,6 +40,7 @@ struct S390CcwMachineState {
>      uint64_t max_pagesize;
>  
>      SCLPDevice *sclp;
> +    Cpi cpi;
>  };
>  
>  static inline uint64_t s390_get_memory_limit(S390CcwMachineState *s390ms)
> diff --git a/qapi/machine.json b/qapi/machine.json
> index a6b8795b09..9dcd2790eb 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1898,3 +1898,27 @@
>  { 'command': 'x-query-interrupt-controllers',
>    'returns': 'HumanReadableText',
>    'features': [ 'unstable' ]}
> +
> +##
> +# @S390Cpi:

Maybe spelling this out would be preferable?
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': 'S390Cpi', 'data': {
> +     'system-type': 'str',
> +     'system-name': 'str',
> +     'system-level': 'str',
> +     'sysplex-name': 'str',
> +     'timestamp': 'uint64' } }