| Message ID | 20250224120449.1764114-3-shalini@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [qemu,v2,1/3] hw/s390x: add CPI identifiers to QOM | expand |
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. Additionally, I think you need some compat handling for backward migration in your patches. E.g. have you tried migrating from an old version of QEMU to a newer one (that includes your patches) and then back to the old one? I think the TYPE_SCLP_CPI device should only be instantiated for the machine types >= 10.0, but not for the older machine types, e.g. by introducing a "use-cpi" property to the TYPE_SCLP_EVENT_FACILITY (set to true by default). Then in ccw_machine_9_2_class_options(), make sure that this property gets switched to "off" again, so that older machine types don't have the new TYPE_SCLP_CPI device. WDYT? Thomas
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. > Additionally, I think you need some compat handling for backward > migration in your patches. E.g. have you tried migrating from an old > version of QEMU to a newer one (that includes your patches) and then > back to the old one? > I think the TYPE_SCLP_CPI device should only be instantiated for the > machine types >= 10.0, but not for the older machine types, e.g. by > introducing a "use-cpi" property to the TYPE_SCLP_EVENT_FACILITY (set > to true by default). Then in ccw_machine_9_2_class_options(), make > sure that this property gets switched to "off" again, so that older > machine types don't have the new TYPE_SCLP_CPI device. WDYT? > I will answer this shortly. Thank you. > Thomas
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
On 2025-03-06 16:08, Thomas Huth wrote: > 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. > Good Morning Thomas, Thank you very much for the explanation. I will change this. > Thanks! > Thomas
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. > > Additionally, I think you need some compat handling for backward > migration in your patches. E.g. have you tried migrating from an old > version of QEMU to a newer one (that includes your patches) and then > back to the old one? > I think the TYPE_SCLP_CPI device should only be instantiated for the > machine types >= 10.0, but not for the older machine types, e.g. by > introducing a "use-cpi" property to the TYPE_SCLP_EVENT_FACILITY (set > to true by default). Then in ccw_machine_9_2_class_options(), make > sure that this property gets switched to "off" again, so that older > machine types don't have the new TYPE_SCLP_CPI device. WDYT? > Hello Thomas, That is correct. I will change this as per your suggestion. Thank you. > Thomas
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); + css_register_vmstate(); /* Create VirtIO network adapters */