| Message ID | 1464847505-27478-1-git-send-email-hongyang.yang@easystack.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 > > >
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
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 > > > >
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)
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(-)