Re: [PATCH v3 2/4] hw/s390x: add Control-Program Identification to QOM

[Thomas Huth <thuth@redhat.com>]

On 31/03/2025 16.00, Shalini Chellathurai Saroja wrote:
> Add Control-Program Identification data to the QEMU Object
> Model (QOM), along with the timestamp in which the data was received.
> 
> Example:
> virsh # qemu-monitor-command vm --pretty '{
> "execute": "qom-get",
> "arguments": {
> "path": "/machine/sclp/s390-sclp-event-facility/sclpcpi",
> "property": "control-program-id" }}'
> {
>    "return": {
>      "timestamp": 1742390410685762000,
>      "system-level": 74872343805430528,
>      "sysplex-name": "PLEX ",
>      "system-name": "TESTVM  ",
>      "system-type": "LINUX   "
>    },
>    "id": "libvirt-15"
> }
> 
> Signed-off-by: Shalini Chellathurai Saroja <shalini@linux.ibm.com>
> ---
>   hw/s390x/sclpcpi.c                | 38 ++++++++++++++++++++
>   include/hw/s390x/event-facility.h |  9 +++++
>   qapi/machine.json                 | 58 +++++++++++++++++++++++++++++++
>   3 files changed, 105 insertions(+)
> 
> diff --git a/hw/s390x/sclpcpi.c b/hw/s390x/sclpcpi.c
> index 7ace5dd64e..969c15e43d 100644
> --- a/hw/s390x/sclpcpi.c
> +++ b/hw/s390x/sclpcpi.c
> @@ -57,8 +57,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 "qapi/qapi-visit-machine.h"
>   
>   typedef struct Data {
>       uint8_t id_format;
> @@ -99,10 +102,37 @@ static int write_event_data(SCLPEvent *event, EventBufferHeader *evt_buf_hdr)
>       ControlProgramIdMsg *cpim = container_of(evt_buf_hdr, ControlProgramIdMsg,
>                                                ebh);
>   
> +    ascii_put(event->cpi.system_type, (char *)cpim->data.system_type, 8);
> +    ascii_put(event->cpi.system_name, (char *)cpim->data.system_name, 8);
> +    ascii_put(event->cpi.sysplex_name, (char *)cpim->data.sysplex_name, 8);
> +    event->cpi.system_level = ldq_be_p(&cpim->data.system_level);
> +    event->cpi.timestamp = qemu_clock_get_ns(QEMU_CLOCK_HOST);
> +
>       cpim->ebh.flags = SCLP_EVENT_BUFFER_ACCEPTED;
>       return SCLP_RC_NORMAL_COMPLETION;
>   }
>   
> +static void get_control_program_id(Object *obj, Visitor *v,
> +                                   const char *name, void *opaque,
> +                                   Error **errp)
> +{
> +    SCLPEvent *event = (SCLPEvent *)(obj);
> +    S390ControlProgramId *cpi;
> +
> +    cpi = &(S390ControlProgramId){
> +        .system_type = g_strndup((char *) event->cpi.system_type,
> +                                 sizeof(event->cpi.system_type)),
> +        .system_name = g_strndup((char *) event->cpi.system_name,
> +                                 sizeof(event->cpi.system_name)),
> +        .system_level = event->cpi.system_level,
> +        .sysplex_name = g_strndup((char *) event->cpi.sysplex_name,
> +                                  sizeof(event->cpi.sysplex_name)),
> +        .timestamp = event->cpi.timestamp
> +    };
> +
> +    visit_type_S390ControlProgramId(v, name, &cpi, errp);
> +}
> +
>   static void cpi_class_init(ObjectClass *klass, void *data)
>   {
>       DeviceClass *dc = DEVICE_CLASS(klass);
> @@ -114,6 +144,14 @@ static void cpi_class_init(ObjectClass *klass, void *data)
>       k->get_send_mask = send_mask;
>       k->get_receive_mask = receive_mask;
>       k->write_event_data = write_event_data;
> +
> +    object_class_property_add(klass, "control-program-id",
> +                              "S390ControlProgramId",
> +                              get_control_program_id,
> +                              NULL, NULL, NULL);
> +    object_class_property_set_description(klass, "control-program-id",
> +        "Control-program identifiers provide data about the guest "
> +        "operating system");
>   }
>   
>   static const TypeInfo sclp_cpi_info = {
> diff --git a/include/hw/s390x/event-facility.h b/include/hw/s390x/event-facility.h
> index f445d2f9f5..39e589ed44 100644
> --- a/include/hw/s390x/event-facility.h
> +++ b/include/hw/s390x/event-facility.h
> @@ -169,10 +169,19 @@ typedef struct ReadEventData {
>       };
>   } QEMU_PACKED ReadEventData;
>   
> +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;

Do we need the QEMU_PACKED here? The members seem naturally aligned, and the 
struct does not seem to be involved in the communication with the guest 
directly, so if you don't need it, please drop the QEMU_PACKED here.
(without PACKED, the compiler can optimize the code in a better way, and 
this causes less trouble on exotic platforms like SPARC that cannot access 
unaligned memory addresses)

>   struct SCLPEvent {
>       DeviceState qdev;
>       bool event_pending;
>       char *name;
> +    ControlProgramId cpi;
>   };
>   
>   struct SCLPEventClass {
> diff --git a/qapi/machine.json b/qapi/machine.json
> index a6b8795b09..cd2bcd2d13 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1898,3 +1898,61 @@
>   { 'command': 'x-query-interrupt-controllers',
>     'returns': 'HumanReadableText',
>     'features': [ 'unstable' ]}
> +
> +##
> +# @S390ControlProgramId:
> +#
> +# Control-program identifiers provide data about the guest operating system.
> +# The control-program identifiers are: system type, system name, system level
> +# and sysplex name.
> +#
> +# In Linux, all the control-program identifiers are user configurable. The
> +# system type, system name, and sysplex name use EBCDIC characters from
> +# this set: capital A-Z, 0-9, $, @, #, and blank.  In Linux, the system type,
> +# system name and sysplex name are arbitrary free-form texts.
> +#
> +# In Linux, the 8-byte hexadecimal system-level has the format
> +# 0x<a><b><cc><dd><eeee><ff><gg><hh>, where:
> +# <a>: is one hexadecimal byte, its most significant bit indicates hypervisor
> +# use
> +# <b>: is one digit that represents Linux distributions as follows
> +# 0: generic Linux
> +# 1: Red Hat Enterprise Linux
> +# 2: SUSE Linux Enterprise Server
> +# 3: Canonical Ubuntu
> +# 4: Fedora
> +# 5: openSUSE Leap
> +# 6: Debian GNU/Linux
> +# 7: Red Hat Enterprise Linux CoreOS
> +# <cc>: are two digits for a distribution-specific encoding of the major version
> +# of the distribution
> +# <dd>: are two digits for a distribution-specific encoding of the minor version
> +# of the distribution
> +# <eeee>: are four digits for the patch level of the distribution
> +# <ff>: are two digits for the major version of the kernel
> +# <gg>: are two digits for the minor version of the kernel
> +# <hh>: are two digits for the stable version of the kernel
> +# (e.g. 74872343805430528, when converted to hex is 0x010a000000060b00). On
> +# machines prior to z16, some of the values are not available to display.

You've got the same information in the comment at the beginning of the 
sclpcpi.c file already ... maybe it would be good to have it in one place 
only to avoid double maintenance in case it needs to be changed in the 
future. I'd suggest to change the comment at the beginning of sclpcpi.c to 
say something like "for a detailed description of the contents of the CPI, 
please see the S390ControlProgramId QOM-type description." or something similar?

  Thomas