Re: [PATCH 34/40] lasips2: update VMStateDescription for LASIPS2 device

[Peter Maydell]

On Tue, 5 Jul 2022 at 07:48, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 04/07/2022 14:38, Peter Maydell wrote:
>
> > On Wed, 29 Jun 2022 at 13:41, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> Since this series has already introduced a migration break for the HPPA 
> >> B160L
> >> machine, we can use this opportunity to improve the VMStateDescription for
> >> the LASIPS2 device.
> >>
> >> Add the new int_status field to the VMStateDescription and remodel the 
> >> ports
> >> as separate VMSTATE_STRUCT instances.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >> ---
> >>   hw/input/lasips2.c | 25 +++++++++++++++++++------
> >>   1 file changed, 19 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/hw/input/lasips2.c b/hw/input/lasips2.c
> >> index e602e3c986..ea7c07a2ba 100644
> >> --- a/hw/input/lasips2.c
> >> +++ b/hw/input/lasips2.c
> >> @@ -35,15 +35,28 @@
> >>   #include "qapi/error.h"
> >>
> >>
> >> +static const VMStateDescription vmstate_lasips2_port = {
> >> +    .name = "lasips2-port",
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >> +    .fields = (VMStateField[]) {
> >> +        VMSTATE_UINT8(control, LASIPS2Port),
> >> +        VMSTATE_UINT8(buf, LASIPS2Port),
> >> +        VMSTATE_BOOL(loopback_rbne, LASIPS2Port),
> >> +        VMSTATE_END_OF_LIST()
> >> +    }
> >> +};
> >> +
> >>   static const VMStateDescription vmstate_lasips2 = {
> >>       .name = "lasips2",
> >> -    .version_id = 0,
> >> -    .minimum_version_id = 0,
> >> +    .version_id = 1,
> >> +    .minimum_version_id = 1,
> >>       .fields = (VMStateField[]) {
> >> -        VMSTATE_UINT8(kbd_port.parent_obj.control, LASIPS2State),
> >> -        VMSTATE_UINT8(kbd_port.parent_obj.id, LASIPS2State),
> >> -        VMSTATE_UINT8(mouse_port.parent_obj.control, LASIPS2State),
> >> -        VMSTATE_UINT8(mouse_port.parent_obj.id, LASIPS2State),
> >> +        VMSTATE_UINT8(int_status, LASIPS2State),
> >> +        VMSTATE_STRUCT(kbd_port.parent_obj, LASIPS2State, 1,
> >> +                       vmstate_lasips2_port, LASIPS2Port),
> >> +        VMSTATE_STRUCT(mouse_port.parent_obj, LASIPS2State, 1,
> >> +                       vmstate_lasips2_port, LASIPS2Port),
> >>           VMSTATE_END_OF_LIST()
> >>       }
> >>   };
> >
> > The set of things we were migrating and the set of
> > things we now migrate don't seem to line up ?
>
> Yeah I should probably have documented this better in the commit message. The
> existing VMStateDescription isn't correct since it misses both the buf and
> loopback_rbne fields which are accessed across port reads and writes, and the 
> port id
> is not required because it is hardcoded to 0 for the keyboard port and 1 for 
> the
> mouse port.
>
> Rather than have the extra code churn throughout the series, I went for doing 
> the
> minimum to vmstate_lasips2 to make the patch compile and then fix up 
> everything
> (including add the new int_status bitmap) in this one patch. I think this is 
> fine
> since as we're introducing a migration break in the series, there are no 
> concerns
> over backward compatibility.
>
> Would an updated description for this commit message be sufficient? A quick 
> skim over
> the emails indicates that the only queries during review were related to the 
> handling
> of the VMStateDescription.

Yeah, if you comment this in the relevant commit messages that should be
good enough.

-- PMM