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

[Thomas Huth <thuth@redhat.com>]

On 03/04/2025 16.33, Shalini Chellathurai Saroja wrote:
> On 2025-04-01 15:55, Nina Schoetterl-Glausch wrote:
>> On Mon, 2025-03-31 at 16:00 +0200, 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);
>>
>> Do a checked cast with SCLP_EVENT(obj).
> 
> Hello Nina,
> 
> ok, thank you.
>>
>>> +    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;
>>> +
>>>  struct SCLPEvent {
>>>      DeviceState qdev;
>>>      bool event_pending;
>>>      char *name;
>>> +    ControlProgramId cpi;
>>
>> I don't think this should go into SCLPEvent.
>> Rather SCLPEventFacility or SCLPDevice. Otherwise all events,
>> so also quiesce and cpu_hotplug have a cpi field.
>>
> ok, that is correct.
> 
> I gave it a try by moving ControlProgramId to SCLPDevice. With this, the 
> migration data is stored in dc->vmsd of TYPE_SCLP as shown below.
> 
> diff --git a/hw/s390x/sclp.c b/hw/s390x/sclp.c
> index 5945c9b1d8..4d6d5bb857 100644
> --- a/hw/s390x/sclp.c
> +++ b/hw/s390x/sclp.c
> @@ -424,6 +424,29 @@ static void sclp_init(Object *obj)
>       sclp_memory_init(sclp);
>   }
> 
> +static const VMStateDescription vmstate_control_program_id = {
> +    .name = "s390_control_program_id",
> +    .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 const VMStateDescription vmstate_sclpcpi = {
> +    .name = "s390_sclpcpi",
> +    .version_id = 0,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_STRUCT(cpi, SCLPDevice, 0, vmstate_control_program_id,
> +                       ControlProgramId),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>   static void sclp_class_init(ObjectClass *oc, void *data)
>   {
>       SCLPDeviceClass *sc = SCLP_CLASS(oc);
> @@ -438,6 +461,7 @@ static void sclp_class_init(ObjectClass *oc, void *data)
>        * which is a non-pluggable sysbus device
>        */
>       dc->user_creatable = false;
> +    dc->vmsd =  &vmstate_sclpcpi;
> 
>       sc->read_SCP_info = read_SCP_info;
>       sc->read_cpu_info = sclp_read_cpu_info;
> 
> @Thomas Huth: Is this ok?, thank you.

  Hi,

that also does not look like the right place to me ... can't you move the 
migration state to the TYPE_SCLP_CPI device instead?

  Thomas