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

[Thomas Huth <thuth@redhat.com>]

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