[RFC] ps2: set the keybord output buffer size as the same as kernel

From: Gonglei <arei.gonglei@huawei.com>

According to the PS/2 Mouse/Keyboard Protocol, the keyboard outupt buffer size
is 16 bytes. And the PS2_QUEUE_SIZE 256 was introduced in Qemu from the very
beginning.

When I started a  redhat5.6 32bit guest, meanwhile tapped the keyboard as quickly as
possible, the screen would show me "i8042.c: No controller found". As a result,
I couldn't use the keyboard in the VNC client.

Previous discussion about the issue in maillist:
http://thread.gmane.org/gmane.comp.emulators.qemu/43294/focus=47180

This patch has been tested on redhat5.6 32-bit/suse11sp3 64-bit guests.
More easy meathod to reproduce:
1.boot a guest with libvirt.
2.connect to VNC client.
3.as you see the BIOS, bootloader, Linux booting, run the follow simply shell script:
for((i=0;i<10000000;i++)) do virsh send-key redhat5.6 KEY_A; done

Actual results:
dmesg show "i8042.c: No controller found." And the keyboard is out of work.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/input/ps2.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

Hi,

> -----Original Message-----
> From: Gonglei (Arei)
> Sent: Thursday, April 17, 2014 9:16 PM
> To: qemu-devel@nongnu.org
> Cc: kraxel@redhat.com; aliguori@amazon.com; Huangweidong (C); Gonglei
> (Arei)
> Subject: [PATCH RFC] ps2: set the keybord output buffer size as the same as
> kernel
> 
> From: Gonglei <arei.gonglei@huawei.com>
> 
> According to the PS/2 Mouse/Keyboard Protocol, the keyboard outupt buffer
> size
> is 16 bytes. And the PS2_QUEUE_SIZE 256 was introduced in Qemu from the
> very
> beginning.
> 
> When I started a  redhat5.6 32bit guest, meanwhile tapped the keyboard as
> quickly as
> possible, the screen would show me "i8042.c: No controller found". As a result,
> I couldn't use the keyboard in the VNC client.
> 
> Previous discussion about the issue in maillist:
> http://thread.gmane.org/gmane.comp.emulators.qemu/43294/focus=47180
> 
> This patch has been tested on redhat5.6 32-bit/suse11sp3 64-bit guests.
> More easy meathod to reproduce:
> 1.boot a guest with libvirt.
> 2.connect to VNC client.
> 3.as you see the BIOS, bootloader, Linux booting, run the follow simply shell
> script:
> for((i=0;i<10000000;i++)) do virsh send-key redhat5.6 KEY_A; done
> 
> Actual results:
> dmesg show "i8042.c: No controller found." And the keyboard is out of work.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  hw/input/ps2.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> 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];
> @@ -137,7 +137,7 @@ void ps2_queue(void *opaque, int b)
>      PS2State *s = (PS2State *)opaque;
>      PS2Queue *q = &s->queue;
> 
> -    if (q->count >= PS2_QUEUE_SIZE)
> +    if (q->count >= PS2_QUEUE_SIZE - 1)
>          return;
>      q->data[q->wptr] = b;
>      if (++q->wptr == PS2_QUEUE_SIZE)
> @@ -375,7 +375,7 @@ static void ps2_mouse_event(void *opaque,
>      }
> 
>      if (!(s->mouse_status & MOUSE_STATUS_REMOTE) &&
> -        (s->common.queue.count < (PS2_QUEUE_SIZE - 16))) {
> +        (s->common.queue.count < PS2_QUEUE_SIZE)) {
>          for(;;) {
>              /* if not remote, send event. Multiple events are sent if
>                 too big deltas */
> --
> 1.6.0.2
> 

Ping...
Any comments will be appreciated.


Best regards,
-Gonglei

> 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.

cheers,
  Gerd

> 

> > 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.

Best regards,
-Gonglei

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

Gerd Hoffmann <kraxel@redhat.com> 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.

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:quintela@redhat.com]
> Sent: Tuesday, April 22, 2014 8:05 PM
> To: Gerd Hoffmann
> Cc: Gonglei (Arei); qemu-devel@nongnu.org; aliguori@amazon.com;
> Huangweidong (C)
> Subject: Re: [PATCH RFC] ps2: set the keybord output buffer size as the same as
> kernel
> 
> Gerd Hoffmann <kraxel@redhat.com> 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.

> 
> 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.
> 
Sorry, the second solution also support cross-version live migration each other.

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

Best regards,
-Gonglei

On Mi, 2014-04-23 at 08:06 +0000, Gonglei (Arei) wrote:
> 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 think long term we want #2, but using #1 as step inbetween for a few
releases until 16 byte buffer size is widely used will might be useful.

Completely separate question:  Have you figured what the root cause for
the bug is?  While wading through the code I've figured the queue size
isn't (directly) exposed to the guest.  And it's used for both mouse and
kbd.  For the kbd 16 byte buffer is probably ok, but for mouse events we
probably want keep some more space to buffer events ...

cheers,
  Gerd

"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
>> 
>> 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.
>> 
> Sorry, the second solution also support cross-version live migration each other.

Second solution is better in the sense that it allows forward and
backward migration.   The first one only allows forward migration.

Please use second one.

Later, Juan.

>
>> I have tested the two solutions, but which one is better? Expect your reply.
>> Thanks!
>> 
>
> Best regards,
> -Gonglei

On Mi, 2014-04-23 at 09:32 +0000, Gonglei (Arei) wrote:
> > 
> > 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.
> > 
> Sorry, the second solution also support cross-version live migration each other.

But you loose anything in the buffer when migrating from old -> new.

cheers,
  Gerd

On Mi, 2014-04-23 at 11:45 +0200, Juan Quintela wrote:
> "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> >> 
> >> 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.
> >> 
> > Sorry, the second solution also support cross-version live migration each other.
> 
> Second solution is better in the sense that it allows forward and
> backward migration.   The first one only allows forward migration.

No, first goes both ways too, and unlike the second it doesn't loose the
buffer content as long as less than 16 bytes are actually used.

cheers,
  Gerd

Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mi, 2014-04-23 at 09:32 +0000, Gonglei (Arei) wrote:
>> > 
>> > 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.
>> > 
>> Sorry, the second solution also support cross-version live migration
>> each other.

> But you loose anything in the buffer when migrating from old -> new.

Anything bigger than 16bytes, no?  And that is the whole point that we
are talking about?  Or the 16bytes that we are using can be at any place
on the buffer?  If that is the case, I agree that the first option is
better for some versions.  Sorry for my missunderstanding.

Later, Juan.

PD.  No, I don't claim to understand how PS2 work at all.

> On Mi, 2014-04-23 at 08:06 +0000, Gonglei (Arei) wrote:

> > 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 think long term we want #2, but using #1 as step inbetween for a few

> releases until 16 byte buffer size is widely used will might be useful.

> 

> Completely separate question:  Have you figured what the root cause for

> the bug is?  


When the linux kernel booting, will init the i8042 controller (drivers/input/serio/i8042.c)
i8042_init() 
	|-> i8042_controller_check()

static int i8042_controller_check(void)
{
	if (i8042_flush() == I8042_BUFFER_SIZE) {
		pr_err("No controller found\n");
		return -ENODEV;
	}

	return 0;
}

With this 16 byte buffer in drivers/input/serio/i8042.h:

#define I8042_BUFFER_SIZE       16

and this piece of code in drivers/input/serio/i8042.c:

/*
 * i8042_flush() flushes all data that may be in the *keyboard* and *mouse* buffers
 * of the i8042 down the toilet.
 */

static int i8042_flush(void)
{
        unsigned long flags;
        unsigned char data, str;
        int i = 0;

        spin_lock_irqsave(&i8042_lock, flags);

        while (((str = i8042_read_status()) & I8042_STR_OBF) && (i < I8042_BUFFER_SIZE)) {
                udelay(50);
                data = i8042_read_data();
                i++;
                dbg("%02x <- i8042 (flush, %s)", data,
                        str & I8042_STR_AUXDATA ? "aux" : "kbd");
        }

        spin_unlock_irqrestore(&i8042_lock, flags);

        return i;
}

So, if we type anything on keyboard (or move mouse) over 16 bytes while 
Linux kernel is still booting, Linux kernel will get confused. And as a result, 
decides there is no controller found.

>While wading through the code I've figured the queue size

> isn't (directly) exposed to the guest.  

When we type the keyboard or move mouse, the ps2_queue() will be called,
Account the wptr and count value, and fill the data array. Then the qemu 
will update irq. In kbd_update_irq() the kbd(i8042) state will be set:
	s->status |= KBD_STAT_OBF;
	s->outport |= KBD_OUT_OBF;  
then send irq to announce Guest OS.
The process correspond with the above linux kernel code, i8042_flush(void).

> And it's used for both mouse and

> kbd. For the kbd 16 byte buffer is probably ok,

  
Yes, the i8042 controller used by both ps/2 keyboard and ps/2 mouse. So, 
The buffer size is shared by ps/2 kbd and mouse.

> but for mouse events we

> probably want keep some more space to buffer events ...

>

Maybe you are right, and I worry about it too. At present,
but I haven't feel uncomfortable by VNC client then before.

Best regards,
-Gonglei

> Gerd Hoffmann <kraxel@redhat.com> wrote:
> > On Mi, 2014-04-23 at 09:32 +0000, Gonglei (Arei) wrote:
> >> >
> >> > 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.
> >> >
> >> Sorry, the second solution also support cross-version live migration
> >> each other.
> 
> > But you loose anything in the buffer when migrating from old -> new.
> 
> Anything bigger than 16bytes, no?  And that is the whole point that we
> are talking about?  Or the 16bytes that we are using can be at any place
> on the buffer?  If that is the case, I agree that the first option is
> better for some versions.  Sorry for my missunderstanding.
> 
Actually, in my hundreds of times testing, about the queue buffer value, at the migration dest end
the rptr always equal with wptr, the value range is [0, 256), and count value always is 0.
So, in the old qemu versions, the 16 bytes data maybe at any place on the buffer. The queue just be 
realized to a lined container by data array, the rptr/wptr will fill it cyclically. 

So, I want to post a normal patch about the first solution, can I? Thanks.


Best regards,
-Gonglei

Hi,

> Anything bigger than 16bytes, no?  And that is the whole point that we
> are talking about?  Or the 16bytes that we are using can be at any place
> on the buffer?

Yes.  It's a ring buffer, with rptr pointing to the first used element
and wptr pointing to the first free element.

So, what we need is a function to move the content we are interested
(between rptr and wptr) in to the head of the ring buffer, set rptr to
0, set wptr to count.  We obviously need to do that in post_load(), but
as I've noticed meanwhile also in pre_save, otherwise old qemu will get
things wrong in case the buffer is wrapped (rptr > wptr).

cheers,
  Gerd

>   Hi,

> 

> > Anything bigger than 16bytes, no?  And that is the whole point that we

> > are talking about?  Or the 16bytes that we are using can be at any place

> > on the buffer?

> 

> Yes.  It's a ring buffer, with rptr pointing to the first used element

> and wptr pointing to the first free element.

> 

> So, what we need is a function to move the content we are interested

> (between rptr and wptr) in to the head of the ring buffer, set rptr to

> 0, set wptr to count.  We obviously need to do that in post_load(), but

> as I've noticed meanwhile also in pre_save, otherwise old qemu will get

> things wrong in case the buffer is wrapped (rptr > wptr).

> 

Hi, Gerd. Maybe we just need to do in post_load(). Such as the follow code:

static int ps2_kbd_post_load(void* opaque, int version_id)
{
    PS2KbdState *s = (PS2KbdState*)opaque;
    PS2State *ps2 = &s->common;
    PS2Queue *q = &ps2->queue;
    int size;
    int i;
        
    if (version_id == 2)
        s->scancode_set=2;
    /* the new version id for this patch */
    if (version_id == 4) {
        return 0;
    }
    /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
    size = MIN(q->count, PS2_QUEUE_SIZE);
    printf(" ==kbd: rptr: %d, wptr: %d, count: %d, size: %d\n", q->rptr, q->wptr, q->count, size);
    for (i = 0; i < size; i++) {
        /* move the queue elements to the start of data array */
        q->data[i] = q->data[q->rptr];
        if (++q->rptr == 256) {
            q->rptr = 0;
        }
    }
    /* reset rptr/wptr/count */
    q->rptr = 0;
    q->wptr = size;
    q->count = size;
    ps2->update_irq(ps2->update_arg, q->count != 0);

    return 0;
}
Any problem? Thanks.

Best regards,
-Gonglei

"Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
>>   Hi,
>> 
>> > Anything bigger than 16bytes, no?  And that is the whole point that we
>> > are talking about?  Or the 16bytes that we are using can be at any place
>> > on the buffer?
>> 
>> Yes.  It's a ring buffer, with rptr pointing to the first used element
>> and wptr pointing to the first free element.
>> 
>> So, what we need is a function to move the content we are interested
>> (between rptr and wptr) in to the head of the ring buffer, set rptr to
>> 0, set wptr to count.  We obviously need to do that in post_load(), but
>> as I've noticed meanwhile also in pre_save, otherwise old qemu will get
>> things wrong in case the buffer is wrapped (rptr > wptr).
>> 
> Hi, Gerd. Maybe we just need to do in post_load(). Such as the follow code:
>
> static int ps2_kbd_post_load(void* opaque, int version_id)
> {
>     PS2KbdState *s = (PS2KbdState*)opaque;

cast not needed.

>     PS2State *ps2 = &s->common;
>     PS2Queue *q = &ps2->queue;
>     int size;
>     int i;
>         
>     if (version_id == 2)
>         s->scancode_set=2;
>     /* the new version id for this patch */
>     if (version_id == 4) {
>         return 0;
>     }
>     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
>     size = MIN(q->count, PS2_QUEUE_SIZE);
>     printf(" ==kbd: rptr: %d, wptr: %d, count: %d, size: %d\n", q->rptr, q->wptr, q->count, size);
>     for (i = 0; i < size; i++) {
>         /* move the queue elements to the start of data array */
>         q->data[i] = q->data[q->rptr];

It is me, or this don't work if the destination and source regions
overlap?  Yes, it depens on how it overlaps.

>         if (++q->rptr == 256) {
>             q->rptr = 0;
>         }
>     }
>     /* reset rptr/wptr/count */
>     q->rptr = 0;
>     q->wptr = size;
>     q->count = size;
>     ps2->update_irq(ps2->update_arg, q->count != 0);
>
>     return 0;
> }
> Any problem? Thanks.
>
> Best regards,
> -Gonglei

> 
> "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
> >>   Hi,
> >>
> >> > Anything bigger than 16bytes, no?  And that is the whole point that we
> >> > are talking about?  Or the 16bytes that we are using can be at any place
> >> > on the buffer?
> >>
> >> Yes.  It's a ring buffer, with rptr pointing to the first used element
> >> and wptr pointing to the first free element.
> >>
> >> So, what we need is a function to move the content we are interested
> >> (between rptr and wptr) in to the head of the ring buffer, set rptr to
> >> 0, set wptr to count.  We obviously need to do that in post_load(), but
> >> as I've noticed meanwhile also in pre_save, otherwise old qemu will get
> >> things wrong in case the buffer is wrapped (rptr > wptr).
> >>
> > Hi, Gerd. Maybe we just need to do in post_load(). Such as the follow code:
> >
> > static int ps2_kbd_post_load(void* opaque, int version_id)
> > {
> >     PS2KbdState *s = (PS2KbdState*)opaque;
> 
> cast not needed.
This is the original code, I don't change it. Maybe I should post the diff file, Sorry.

> 
> >     PS2State *ps2 = &s->common;
> >     PS2Queue *q = &ps2->queue;
> >     int size;
> >     int i;
> >
> >     if (version_id == 2)
> >         s->scancode_set=2;
> >     /* the new version id for this patch */
> >     if (version_id == 4) {
> >         return 0;
> >     }
> >     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
> >     size = MIN(q->count, PS2_QUEUE_SIZE);
> >     printf(" ==kbd: rptr: %d, wptr: %d, count: %d, size: %d\n", q->rptr,
> q->wptr, q->count, size);
> >     for (i = 0; i < size; i++) {
> >         /* move the queue elements to the start of data array */
> >         q->data[i] = q->data[q->rptr];
> 
> It is me, or this don't work if the destination and source regions
> overlap?  Yes, it depens on how it overlaps.
Good catch, Juan, Thanks. It should use a temp data array to save the q->data[] value,
Such as below:
    int size;
    int i;
    int tmp_data[PS2_QUEUE_SIZE];
    
    /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
    size = MIN(q->count, PS2_QUEUE_SIZE);
    printf(" ==kbd: rptr: %d, wptr: %d, count: %d, size: %d\n", q->rptr, q->wptr, q->count, size);
    for (i = 0; i < size; i++) {
        /* move the queue elements to the tmp data array */
        tmp_data[i] = q->data[q->rptr];
        if (++q->rptr == 256) {
            q->rptr = 0;
        }
    }
    for (i = 0; i < size; i++) {
        /* move the queue elements to the start of data array */
        q->data[i] = tmp_data[i];
    }
    /* reset rptr/wptr/count */
    q->rptr = 0;
    q->wptr = size;
    q->count = size;
    s->update_irq(s->update_arg, q->count != 0);
    
    return 0;
}

Best regards,
-Gonglei

Hi,

>     /* the new version id for this patch */
>     if (version_id == 4) {
>         return 0;
>     }

I don't think we need a new version.

>     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
>     size = MIN(q->count, PS2_QUEUE_SIZE);

I'd rather do "size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;"
i.e. if it doesn't fit better throw away everything, which reduces the
chance that we'll deliver a incomplete message to the guest.

>     for (i = 0; i < size; i++) {
>         /* move the queue elements to the start of data array */
>         q->data[i] = q->data[q->rptr];
>         if (++q->rptr == 256) {
>             q->rptr = 0;
>         }
>     }

That fails for the wraparound (rptr > wptr) case.

cheers,
  Gerd

> 

>   Hi,

> 

> >     /* the new version id for this patch */

> >     if (version_id == 4) {

> >         return 0;

> >     }

> 

> I don't think we need a new version.

> 

OK.

> >     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */

> >     size = MIN(q->count, PS2_QUEUE_SIZE);

> 

> I'd rather do "size = q->count > PS2_QUEUE_SIZE ? 0 : q->count;"

> i.e. if it doesn't fit better throw away everything, which reduces the

> chance that we'll deliver a incomplete message to the guest.

> 

Sounds good, Thanks, Gerd.

> >     for (i = 0; i < size; i++) {

> >         /* move the queue elements to the start of data array */

> >         q->data[i] = q->data[q->rptr];

> >         if (++q->rptr == 256) {

> >             q->rptr = 0;

> >         }

> >     }

> 

> That fails for the wraparound (rptr > wptr) case.

> 

Yep, it should use a temporary data array to transfer, which I have written
in the previous email.


Best regards,
-Gonglei

Hi,

> > Completely separate question:  Have you figured what the root cause for
> > the bug is?  
> 
> > While wading through the code I've figured the queue size
> > isn't (directly) exposed to the guest.  

> The process correspond with the above linux kernel code, i8042_flush(void).

So linux tries to flush the buffer, then is confused in case it can read
more than 16 bytes (which is the buffer size of real hardware) without
seeing the status register signaling that the buffer is empty.  So the
qemu internal buffer size is indirectly visible to the guest.

Hmm.

We could try sending the data to the guest in chunks, i.e. signal
"buffer empty", then set up a (short) timer when we'll go make it appear
to the guest as if new data arrived (which in reality just comes from
out large buffer).

Not that easy to do, as we should better avoid splitting messages in the
middle.

> > but for mouse events we
> > probably want keep some more space to buffer events ...
> >
> Maybe you are right, and I worry about it too. At present,
> but I haven't feel uncomfortable by VNC client then before.

Do you have a usb-tablet attached, for more comfortable absolute
coordinates?  If so, then the ps/2 mouse will not be used to deliver
events to the guest.

And as this is a pretty common setup we maybe should not worry too much
about the ps/2 mouse queue depth ...

cheers,
  Gerd

Hi,

> > >         if (++q->rptr == 256) {
> > >             q->rptr = 0;
> > >         }
> > >     }
> > 
> > That fails for the wraparound (rptr > wptr) case.
> > 
> Yep, it should use a temporary data array to transfer, which I have written
> in the previous email.

Saw it.  And this wraparound handling is also the reason why pre-save
needs to do this too.  You want qemu versions with both 16 and 256 bytes
buffer size interpret the vmstate stream correctly.  Therefore you
should write out the ring buffer in a state where the buffer size is not
needed to read it, i.e. where rptr + count < PS2_QUEUE_SIZE so you don't
need to wrap around.

cheers,
  Gerd

> 

>   Hi,

> 

> > > >         if (++q->rptr == 256) {

> > > >             q->rptr = 0;

> > > >         }

> > > >     }

> > >

> > > That fails for the wraparound (rptr > wptr) case.

> > >

> > Yep, it should use a temporary data array to transfer, which I have written

> > in the previous email.

> 

> Saw it.  And this wraparound handling is also the reason why pre-save

> needs to do this too.  You want qemu versions with both 16 and 256 bytes

> buffer size interpret the vmstate stream correctly.  Therefore you

> should write out the ring buffer in a state where the buffer size is not

> needed to read it, i.e. where rptr + count < PS2_QUEUE_SIZE so you don't

> need to wrap around.

> 

So do you agree with my method to address the issue? Gerd. Thanks,


Best regards,
-Gonglei

"Gonglei (Arei)" <arei.gonglei@huawei.com> writes:

>> 
>> "Gonglei (Arei)" <arei.gonglei@huawei.com> wrote:
>> >>   Hi,
>> >>
>> >> > Anything bigger than 16bytes, no?  And that is the whole point that we
>> >> > are talking about?  Or the 16bytes that we are using can be at any place
>> >> > on the buffer?
>> >>
>> >> Yes.  It's a ring buffer, with rptr pointing to the first used element
>> >> and wptr pointing to the first free element.
>> >>
>> >> So, what we need is a function to move the content we are interested
>> >> (between rptr and wptr) in to the head of the ring buffer, set rptr to
>> >> 0, set wptr to count.  We obviously need to do that in post_load(), but
>> >> as I've noticed meanwhile also in pre_save, otherwise old qemu will get
>> >> things wrong in case the buffer is wrapped (rptr > wptr).
>> >>
>> > Hi, Gerd. Maybe we just need to do in post_load(). Such as the follow code:
>> >
>> > static int ps2_kbd_post_load(void* opaque, int version_id)
>> > {
>> >     PS2KbdState *s = (PS2KbdState*)opaque;
>> 
>> cast not needed.
> This is the original code, I don't change it. Maybe I should post the
> diff file, Sorry.
>
>> 
>> >     PS2State *ps2 = &s->common;
>> >     PS2Queue *q = &ps2->queue;
>> >     int size;
>> >     int i;
>> >
>> >     if (version_id == 2)
>> >         s->scancode_set=2;
>> >     /* the new version id for this patch */
>> >     if (version_id == 4) {
>> >         return 0;
>> >     }
>> >     /* set the useful data buffer queue size, < PS2_QUEUE_SIZE */
>> >     size = MIN(q->count, PS2_QUEUE_SIZE);
>> >     printf(" ==kbd: rptr: %d, wptr: %d, count: %d, size: %d\n", q->rptr,
>> q->wptr, q->count, size);
>> >     for (i = 0; i < size; i++) {
>> >         /* move the queue elements to the start of data array */
>> >         q->data[i] = q->data[q->rptr];
>> 
>> It is me, or this don't work if the destination and source regions
>> overlap?  Yes, it depens on how it overlaps.
> Good catch, Juan, Thanks. It should use a temp data array to save the q->data[] value,

Check out memmove().

> 

>   Hi,

> 

> > > Completely separate question:  Have you figured what the root cause for

> > > the bug is?

> >

> > > While wading through the code I've figured the queue size

> > > isn't (directly) exposed to the guest.

> 

> > The process correspond with the above linux kernel code, i8042_flush(void).

> 

> So linux tries to flush the buffer, then is confused in case it can read

> more than 16 bytes (which is the buffer size of real hardware) without

> seeing the status register signaling that the buffer is empty.  So the

> qemu internal buffer size is indirectly visible to the guest.

> 

Agreed.

> Hmm.

> 

> We could try sending the data to the guest in chunks, i.e. signal

> "buffer empty", then set up a (short) timer when we'll go make it appear

> to the guest as if new data arrived (which in reality just comes from

> out large buffer).

> 

> Not that easy to do, as we should better avoid splitting messages in the

> middle.

> 

> > > but for mouse events we

> > > probably want keep some more space to buffer events ...

> > >

> > Maybe you are right, and I worry about it too. At present,

> > but I haven't feel uncomfortable by VNC client then before.

> 

> Do you have a usb-tablet attached, for more comfortable absolute

> coordinates?  If so, then the ps/2 mouse will not be used to deliver

> events to the guest.

> 

I have tested the two situation, both using usb-tablet and not using usb-tablet .

> And as this is a pretty common setup we maybe should not worry too much

> about the ps/2 mouse queue depth ...

> 

> cheers,

>   Gerd

> 


Best regards,
-Gonglei

Hi,

> So do you agree with my method to address the issue?

Using a temporary buffer is fine, simple and save choice.  For the
second copy you can use a simple memcpy instead of the loop.

cheers,
  Gerd

Hi,

> > Good catch, Juan, Thanks. It should use a temp data array to save the q->data[] value,
> 
> Check out memmove().

Doesn't help if you have to move two chunks (wraparound case).

cheers,
  Gerd

Hi,

> Subject: Re: [PATCH RFC] ps2: set the keybord output buffer size as the same as

> 

> > So do you agree with my method to address the issue?

> 

> Using a temporary buffer is fine, simple and save choice.  For the

> second copy you can use a simple memcpy instead of the loop.

> 

Okay, I will post a formal patch, please help me to review it, thanks!

 
Best regards,
-Gonglei