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

[Mark Cave-Ayland]

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.


ATB,

Mark.