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

[Shalini Chellathurai Saroja <shalini@linux.ibm.com>]

On 2025-04-09 07:30, Thomas Huth wrote:
> 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?
> 

Hi Thomas,
I will do so, 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