[Qemu-devel] [PATCH] ps2: take exact use of ps2 buffer

[Qemu-devel] [PATCH] ps2: take exact use of ps2 buffer

[Yang Hongyang]

According to PS/2 Mouse/Keyboard Protocol, the keyboard output buffer
size is 16 bytes, but we only use 15 bytes actually, this causes some
problem, for example, if I submit "123456789" in a bunch through VNC,
the actual result will be "12345678888888888...", because the 16th key
event which is "8" key up is dropped.

Signed-off-by: Yang Hongyang <hongyang.yang@easystack.cn>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Gonglei <arei.gonglei@huawei.com>
Cc: Juan Quintela <quintela@redhat.com>
---
 hw/input/ps2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/input/ps2.c b/hw/input/ps2.c
index a8aa36f..0726270 100644
--- a/hw/input/ps2.c
+++ b/hw/input/ps2.c
@@ -143,7 +143,7 @@ void ps2_queue(void *opaque, int b)
     PS2State *s = (PS2State *)opaque;
     PS2Queue *q = &s->queue;
 
-    if (q->count >= PS2_QUEUE_SIZE - 1)
+    if (q->count >= PS2_QUEUE_SIZE)
         return;
     q->data[q->wptr] = b;
     if (++q->wptr == PS2_QUEUE_SIZE)
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] ps2: take exact use of ps2 buffer

[Gerd Hoffmann]

On Do, 2016-06-02 at 14:05 +0800, Yang Hongyang wrote:
> According to PS/2 Mouse/Keyboard Protocol, the keyboard output buffer
> size is 16 bytes, but we only use 15 bytes actually, this causes some
> problem, for example, if I submit "123456789" in a bunch through VNC,
> the actual result will be "12345678888888888...", because the 16th key
> event which is "8" key up is dropped.

What if you try a 10-char string next?  Things are failing again.
Keyboards are low-bandwidth devices, you can't flood them with data and
expect things to work.

Try this one instead: https://patchwork.ozlabs.org/patch/628491/

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ps2: take exact use of ps2 buffer

[Yang Hongyang]

Hi Gerd,
  Thanks for reply!

On Thu, Jun 2, 2016 at 4:37 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Do, 2016-06-02 at 14:05 +0800, Yang Hongyang wrote:
> > According to PS/2 Mouse/Keyboard Protocol, the keyboard output buffer
> > size is 16 bytes, but we only use 15 bytes actually, this causes some
> > problem, for example, if I submit "123456789" in a bunch through VNC,
> > the actual result will be "12345678888888888...", because the 16th key
> > event which is "8" key up is dropped.
>
> What if you try a 10-char string next?


Actually I've tested this patch, I submit multiple 10-char string, things
work
as expected, it only takes the first 8-char.


> Things are failing again.
> Keyboards are low-bandwidth devices, you can't flood them with data and
> expect things to work.
>

The poin is not about to make it work with more then 8 string, it is to
make it competible with the protocol, which is a 16-bytes buffer, apparantly
we are not following this and which do cause the problem.
This chunk of code originally uses full QUEUE_SIZE buffer, and in
this commit 2858ab09e6f708e3, it changes the behaviour implicitly.


> Try this one instead: https://patchwork.ozlabs.org/patch/628491/


This seems like a good feature, we will use this, thanks, but it's not
related to
this patch.


>
>
> cheers,
>   Gerd
>
>
>


-- 
Thanks,
Yang

Re: [Qemu-devel] [PATCH] ps2: take exact use of ps2 buffer

[Gerd Hoffmann]

On Do, 2016-06-02 at 17:19 +0800, Yang Hongyang wrote:
> Hi Gerd,
>   Thanks for reply!
> 
> 
> On Thu, Jun 2, 2016 at 4:37 PM, Gerd Hoffmann <kraxel@redhat.com>
> wrote:
>         On Do, 2016-06-02 at 14:05 +0800, Yang Hongyang wrote:
>         > According to PS/2 Mouse/Keyboard Protocol, the keyboard
>         output buffer
>         > size is 16 bytes, but we only use 15 bytes actually, this
>         causes some
>         > problem, for example, if I submit "123456789" in a bunch
>         through VNC,
>         > the actual result will be "12345678888888888...", because
>         the 16th key
>         > event which is "8" key up is dropped.
>         
>         What if you try a 10-char string next? 
> 
> 
> Actually I've tested this patch, I submit multiple 10-char string,
> things work
> as expected, it only takes the first 8-char.

That is pure luck.  It could have taken the first 9 chars in case the
guest manages to take one out of the queue while you are filling the
queue.  It's a race and nothing you can depend on.

> The poin is not about to make it work with more then 8 string, it is
> to
> make it competible with the protocol, which is a 16-bytes
> buffer, apparantly
> we are not following this and which do cause the problem.

There is no such protocol.

On physical hardware things are working fine because you simply can't
type fast enough to overflow the buffer.  On virtual hardware you can if
you script keyboard input, thereby breaking things, simply because ps/2
wasn't designed for that.  Whenever the buffer is 15 bytes or 16 bytes
or something else (with usb-kbd) doesn't matter, the underlying issue is
always the same.  And the solution is to apply a speed limit to kbd
input.

> This chunk of code originally uses full QUEUE_SIZE buffer, and in
> this commit 2858ab09e6f708e3, it changes the behaviour implicitly.

It's a workaround for older linux kernels.  They misbehave in case they
find 16 chars in the ps2 buffer (this is something which simply doesn't
happen on real hardware).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] ps2: take exact use of ps2 buffer

[Yang Hongyang]

On Thu, Jun 2, 2016 at 6:05 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:

> On Do, 2016-06-02 at 17:19 +0800, Yang Hongyang wrote:
> > Hi Gerd,
> >   Thanks for reply!
> >
> >
> > On Thu, Jun 2, 2016 at 4:37 PM, Gerd Hoffmann <kraxel@redhat.com>
> > wrote:
> >         On Do, 2016-06-02 at 14:05 +0800, Yang Hongyang wrote:
> >         > According to PS/2 Mouse/Keyboard Protocol, the keyboard
> >         output buffer
> >         > size is 16 bytes, but we only use 15 bytes actually, this
> >         causes some
> >         > problem, for example, if I submit "123456789" in a bunch
> >         through VNC,
> >         > the actual result will be "12345678888888888...", because
> >         the 16th key
> >         > event which is "8" key up is dropped.
> >
> >         What if you try a 10-char string next?
> >
> >
> > Actually I've tested this patch, I submit multiple 10-char string,
> > things work
> > as expected, it only takes the first 8-char.
>
> That is pure luck.  It could have taken the first 9 chars in case the
> guest manages to take one out of the queue while you are filling the
> queue.  It's a race and nothing you can depend on.


> > The poin is not about to make it work with more then 8 string, it is
> > to
> > make it competible with the protocol, which is a 16-bytes
> > buffer, apparantly
> > we are not following this and which do cause the problem.
>
> There is no such protocol.
>
> On physical hardware things are working fine because you simply can't
> type fast enough to overflow the buffer.  On virtual hardware you can if
> you script keyboard input, thereby breaking things, simply because ps/2
> wasn't designed for that.  Whenever the buffer is 15 bytes or 16 bytes
> or something else (with usb-kbd) doesn't matter, the underlying issue is
> always the same.  And the solution is to apply a speed limit to kbd
> input.
>

Thanks for the explaination, I got your point, by add the speed limit to kbd
input, even when I send a bulk kbd event through vnc, the kbd will have
time to
read the data. before this, vnc will fulfill the buffer until qemu have
time to to schedule a kbd_read_data call. I've tested your patch, and it
solved my problem, thank you.


>
> > This chunk of code originally uses full QUEUE_SIZE buffer, and in
> > this commit 2858ab09e6f708e3, it changes the behaviour implicitly.
>
> It's a workaround for older linux kernels.  They misbehave in case they
> find 16 chars in the ps2 buffer (this is something which simply doesn't
> happen on real hardware).
>
> cheers,
>   Gerd
>
>
>
>


-- 
Thanks,
Yang