Re: [Qemu-devel] [PATCH RFC] ps2: set the keybord output buffer size as the same as kernel

[Gonglei (Arei)]

Hi, Gerd and Juan.

Thanks for your guides about the confuse live migration about changing the 
keyboard buffer size. 
According your suggestion, I got two solutions to address the issue:

- Keep the data array 256 bytes long, change the rptr/wptr/count/data array at 
post_load(), both
 Ps/2 keyboard and mouse. This solution can be compatible with older qemu 
versions, which can 
 do live migration each other.

-Change the data array to 16 bytes, still save as PS2_QUEUE_SIZE. Reset the 
rptr/wptr/count at 
post_load(), both ps/2 keyboard and mouse. Add VMSTATE_UNUSED(256-16) in struct 
vmstate_ps2_common.
This solution just save the 16 bytes buffer and drop the rest, So we can't 
migrate vm to older qemu versions.
But migration from old qemu to new qemu is ok.

I have tested the two solutions, but which one is better? Expect your reply. 
Thanks!


Best regards,
-Gonglei


> -----Original Message-----
> From: Juan Quintela [mailto:address@hidden
> Sent: Tuesday, April 22, 2014 8:05 PM
> To: Gerd Hoffmann
> Cc: Gonglei (Arei); address@hidden; address@hidden;
> Huangweidong (C)
> Subject: Re: [PATCH RFC] ps2: set the keybord output buffer size as the same 
> as
> kernel
> 
> Gerd Hoffmann <address@hidden> wrote:
> > On Di, 2014-04-22 at 08:16 +0000, Gonglei (Arei) wrote:
> >> >
> >> > > diff --git a/hw/input/ps2.c b/hw/input/ps2.c
> >> > > index 3412079..a754fef 100644
> >> > > --- a/hw/input/ps2.c
> >> > > +++ b/hw/input/ps2.c
> >> > > @@ -71,7 +71,7 @@
> >> > >  #define MOUSE_STATUS_ENABLED    0x20
> >> > >  #define MOUSE_STATUS_SCALE21    0x10
> >> > >
> >> > > -#define PS2_QUEUE_SIZE 256
> >> > > +#define PS2_QUEUE_SIZE 16     /* Keyboard output buffer size */
> >> > >
> >> > >  typedef struct {
> >> > >      uint8_t data[PS2_QUEUE_SIZE];
> >> >
> >> > This changes ps2 vmstate and breaks live migration.
> >> >
> >> Good catch, Gerd.
> >> I got the information in the destination of live migration:
> >> Unknown savevm section type 24
> >> load of migration failed
> >>
> >> I'm not familiar with the situation of cross-version live migration,
> >> could you give me
> >> some guide ? Thanks.
> >
> > Keep the data array 256 bytes long, best with a comment that
> > compatibility with older qemu versions requires this.
> >
> > Also the post_load function must handle the case that rptr, wptr & count
> > variables have values which used to be valid for the older qemu versions
> > but are not valid any more with the smaller queue.  In the (unlikely)
> > case that count is larger than 16 the best you can do is probably simply
> > throw away the queue.  16 and less queue elements you can move to the
> > start of the data array (so they are within the 16 bytes still used
> > after your patch is merged) and adjust rptr+wptr accordingly.
> >
> > Cc'ing Juan for additional insights.
> >
> > HTH,
> >   Gerd
> 
> 
> static int ps2_common_post_load(void *opaque, int version_id)
> {
>     PS2State *s = opaque;
> 
>     /* Here goes the code that resets rptr/wptr/count if it is bigger
>        than p16
>        Gerd said that dropping the queue is a good idea.
>      */
> 
>     return 0;
> }
> 
> static const VMStateDescription vmstate_ps2_common = {
>     .name = "PS2 Common State",
>     .version_id = 3,
>     .minimum_version_id = 2,
>     .minimum_version_id_old = 2,
>     .post_load = ps2_common_post_load,
>     .fields = (VMStateField[]) {
>         VMSTATE_INT32(write_cmd, PS2State),
>         VMSTATE_INT32(queue.rptr, PS2State),
>         VMSTATE_INT32(queue.wptr, PS2State),
>         VMSTATE_INT32(queue.count, PS2State),
>         VMSTATE_BUFFER(queue.data, PS2State),
>         /* A coment here explaining why we changed the queue from 256 to
>           16 could be a good idea */
>         VMSTATE_UNUSED_BUFFER(256-16 );
>         VMSTATE_END_OF_LIST()
>     }
> };
> 
> 
> Hope it helps.
> 
> Later, Juan.