| Message ID | fcafae5827cc7fab7b98b9afaf73e08a@hostfission.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/hw/input/ps2.c b/hw/input/ps2.c > index 6edf046820..011290920f 100644 > --- a/hw/input/ps2.c > +++ b/hw/input/ps2.c > @@ -192,12 +192,50 @@ void ps2_queue(PS2State *s, int b) > { > PS2Queue *q = &s->queue; > > - if (q->count >= PS2_QUEUE_SIZE - 1) > + if (q->count == PS2_QUEUE_SIZE) > + { > + printf("Warning! PS2 Queue Overflow!\n"); > return; > + } Leftover debug printf? > +void ps2_raise(PS2State *s) > +{ > + s->update_irq(s->update_arg, 1); > +} > + > +void ps2_queue_raise(PS2State *s, int b) > +{ > + ps2_queue(s, b); > + s->update_irq(s->update_arg, 1); > +} I'd suggest to keep the ps2_queue() name. Makes the patch much smaller and easier to review. Factor out the code to actually queue things to a new ps2_queue_noirq() function. > +void ps2_queue_bytes(PS2State *s, const int length, ...) I'd prefer to not use vaargs here as gcc can't check the arguments then. Suggest to just have ps2_queue_{2,3,4}() helpers instead to queue multibyte messages. cheers, Gerd
On 2018-05-07 22:34, Gerd Hoffmann wrote: >> diff --git a/hw/input/ps2.c b/hw/input/ps2.c >> index 6edf046820..011290920f 100644 >> --- a/hw/input/ps2.c >> +++ b/hw/input/ps2.c >> @@ -192,12 +192,50 @@ void ps2_queue(PS2State *s, int b) >> { >> PS2Queue *q = &s->queue; >> >> - if (q->count >= PS2_QUEUE_SIZE - 1) >> + if (q->count == PS2_QUEUE_SIZE) >> + { >> + printf("Warning! PS2 Queue Overflow!\n"); >> return; >> + } > > Leftover debug printf? Correct :), I will remove it. > >> +void ps2_raise(PS2State *s) >> +{ >> + s->update_irq(s->update_arg, 1); >> +} >> + >> +void ps2_queue_raise(PS2State *s, int b) >> +{ >> + ps2_queue(s, b); >> + s->update_irq(s->update_arg, 1); >> +} > > I'd suggest to keep the ps2_queue() name. Makes the patch much smaller > and easier to review. Factor out the code to actually queue things to > a new ps2_queue_noirq() function. > >> +void ps2_queue_bytes(PS2State *s, const int length, ...) Ack. > > I'd prefer to not use vaargs here as gcc can't check the arguments > then. > Suggest to just have ps2_queue_{2,3,4}() helpers instead to queue > multibyte messages. Ack. > > cheers, > Gerd Thanks, Geoff
diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c index f17f18e51b..004ea3466d 100644 --- a/hw/input/pckbd.c +++ b/hw/input/pckbd.c @@ -216,9 +216,9 @@ static uint64_t kbd_read_status(void *opaque, hwaddr addr, static void kbd_queue(KBDState *s, int b, int aux) { if (aux) - ps2_queue(s->mouse, b); + ps2_queue_raise(s->mouse, b); else - ps2_queue(s->kbd, b); + ps2_queue_raise(s->kbd, b); } static void outport_write(KBDState *s, uint32_t val) diff --git a/hw/input/ps2.c b/hw/input/ps2.c index 6edf046820..011290920f 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -192,12 +192,50 @@ void ps2_queue(PS2State *s, int b) { PS2Queue *q = &s->queue; - if (q->count >= PS2_QUEUE_SIZE - 1) + if (q->count == PS2_QUEUE_SIZE) + { + printf("Warning! PS2 Queue Overflow!\n"); return; + } + q->data[q->wptr] = b; if (++q->wptr == PS2_QUEUE_SIZE) q->wptr = 0; q->count++; +} + +void ps2_raise(PS2State *s) +{ + s->update_irq(s->update_arg, 1); +} + +void ps2_queue_raise(PS2State *s, int b) +{ + ps2_queue(s, b); + s->update_irq(s->update_arg, 1); +} + +void ps2_queue_bytes(PS2State *s, const int length, ...) +{ + PS2Queue *q = &s->queue; + + if (PS2_QUEUE_SIZE - q->count < length) { + printf("Unable to send %d bytes, buffer full\n", length); + return; + } + + va_list args; + va_start(args, length); + + for(int i = 0; i < length; ++i) + { + q->data[q->wptr] = va_arg(args, int); + if (++q->wptr == PS2_QUEUE_SIZE) + q->wptr = 0; + q->count++; + } + + va_end(args); s->update_irq(s->update_arg, 1); } @@ -213,13 +251,13 @@ static void ps2_put_keycode(void *opaque, int keycode) if (keycode == 0xf0) { s->need_high_bit = true; } else if (s->need_high_bit) { - ps2_queue(&s->common, translate_table[keycode] | 0x80); + ps2_queue_raise(&s->common, translate_table[keycode] | 0x80); s->need_high_bit = false; } else { - ps2_queue(&s->common, translate_table[keycode]); + ps2_queue_raise(&s->common, translate_table[keycode]); } } else { - ps2_queue(&s->common, keycode); + ps2_queue_raise(&s->common, keycode); }
This fixes an issue by adding bounds checking to multi-byte packets where the PS/2 mouse data stream may become corrupted due to data being discarded when the PS/2 ringbuffer is full. Interrupts for Multi-byte responses are postponed until the final byte has been queued. These changes fix a bug where windows guests drop the mouse device entirely requring the guest to be restarted. Signed-off-by: Geoffrey McRae <geoff@hostfission.com> --- hw/input/pckbd.c | 6 +-- hw/input/ps2.c | 160 +++++++++++++++++++++++++++++++++++++------------------ 2 files changed, 110 insertions(+), 56 deletions(-) } @@ -490,72 +528,80 @@ void ps2_write_keyboard(void *opaque, int val) case -1: switch(val) { case 0x00: - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); break; case 0x05: - ps2_queue(&s->common, KBD_REPLY_RESEND); + ps2_queue_raise(&s->common, KBD_REPLY_RESEND); break; case KBD_CMD_GET_ID: - ps2_queue(&s->common, KBD_REPLY_ACK); /* We emulate a MF2 AT keyboard here */ - ps2_queue(&s->common, KBD_REPLY_ID); if (s->translate) - ps2_queue(&s->common, 0x41); + ps2_queue_bytes(&s->common, 3, + KBD_REPLY_ACK, + KBD_REPLY_ID, + 0x41); else - ps2_queue(&s->common, 0x83); + ps2_queue_bytes(&s->common, 3, + KBD_REPLY_ACK, + KBD_REPLY_ID, + 0x83); break; case KBD_CMD_ECHO: - ps2_queue(&s->common, KBD_CMD_ECHO); + ps2_queue_raise(&s->common, KBD_CMD_ECHO); break; case KBD_CMD_ENABLE: s->scan_enabled = 1; - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); break; case KBD_CMD_SCANCODE: case KBD_CMD_SET_LEDS: case KBD_CMD_SET_RATE: s->common.write_cmd = val; - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); break; case KBD_CMD_RESET_DISABLE: ps2_reset_keyboard(s); s->scan_enabled = 0; - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); break; case KBD_CMD_RESET_ENABLE: ps2_reset_keyboard(s); s->scan_enabled = 1; - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); break; case KBD_CMD_RESET: ps2_reset_keyboard(s); - ps2_queue(&s->common, KBD_REPLY_ACK); - ps2_queue(&s->common, KBD_REPLY_POR); + ps2_queue_bytes(&s->common, 2, + KBD_REPLY_ACK, + KBD_REPLY_POR); break; default: - ps2_queue(&s->common, KBD_REPLY_RESEND); + ps2_queue_raise(&s->common, KBD_REPLY_RESEND); break; } break; case KBD_CMD_SCANCODE: if (val == 0) { - ps2_queue(&s->common, KBD_REPLY_ACK); - ps2_put_keycode(s, s->scancode_set); + if (s->common.queue.count <= PS2_QUEUE_SIZE - 2) + { + ps2_queue_raise(&s->common, KBD_REPLY_ACK); + ps2_put_keycode(s, s->scancode_set); + } } else if (val >= 1 && val <= 3) { s->scancode_set = val; - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); } else { - ps2_queue(&s->common, KBD_REPLY_RESEND); + ps2_queue_raise(&s->common, KBD_REPLY_RESEND); } s->common.write_cmd = -1; break; case KBD_CMD_SET_LEDS: ps2_set_ledstate(s, val); - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); s->common.write_cmd = -1; break; case KBD_CMD_SET_RATE: - ps2_queue(&s->common, KBD_REPLY_ACK); + ps2_queue_raise(&s->common, KBD_REPLY_ACK); s->common.write_cmd = -1; break; } @@ -572,11 +618,15 @@ void ps2_keyboard_set_translation(void *opaque, int mode) s->translate = mode; } -static void ps2_mouse_send_packet(PS2MouseState *s) +static int ps2_mouse_send_packet(PS2MouseState *s) { unsigned int b; int dx1, dy1, dz1; + const int needed = 3 + (s->mouse_type - 2); + if (PS2_QUEUE_SIZE - s->common.queue.count < needed) + return 0; + dx1 = s->mouse_dx; dy1 = s->mouse_dy; dz1 = s->mouse_dz; @@ -614,11 +664,15 @@ static void ps2_mouse_send_packet(PS2MouseState *s) break; } + ps2_raise(&s->common); + trace_ps2_mouse_send_packet(s, dx1, dy1, dz1, b); /* update deltas */ s->mouse_dx -= dx1; s->mouse_dy -= dy1; s->mouse_dz -= dz1; + + return 1; } static void ps2_mouse_event(DeviceState *dev, QemuConsole *src, @@ -680,10 +734,7 @@ static void ps2_mouse_sync(DeviceState *dev) qemu_system_wakeup_request(QEMU_WAKEUP_REASON_OTHER); } if (!(s->mouse_status & MOUSE_STATUS_REMOTE)) { - while (s->common.queue.count < PS2_QUEUE_SIZE - 4) { - /* if not remote, send event. Multiple events are sent if - too big deltas */ - ps2_mouse_send_packet(s); + while(ps2_mouse_send_packet(s)) { if (s->mouse_dx == 0 && s->mouse_dy == 0 && s->mouse_dz == 0) break; } @@ -713,66 +764,68 @@ void ps2_write_mouse(void *opaque, int val) if (s->mouse_wrap) { if (val == AUX_RESET_WRAP) { s->mouse_wrap = 0; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); return; } else if (val != AUX_RESET) { - ps2_queue(&s->common, val); + ps2_queue_raise(&s->common, val); return; } } switch(val) { case AUX_SET_SCALE11: s->mouse_status &= ~MOUSE_STATUS_SCALE21; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_SET_SCALE21: s->mouse_status |= MOUSE_STATUS_SCALE21; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_SET_STREAM: s->mouse_status &= ~MOUSE_STATUS_REMOTE; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_SET_WRAP: s->mouse_wrap = 1; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_SET_REMOTE: s->mouse_status |= MOUSE_STATUS_REMOTE; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_GET_TYPE: - ps2_queue(&s->common, AUX_ACK); - ps2_queue(&s->common, s->mouse_type); + ps2_queue_bytes(&s->common, 2, + AUX_ACK, + s->mouse_type); break; case AUX_SET_RES: case AUX_SET_SAMPLE: s->common.write_cmd = val; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_GET_SCALE: - ps2_queue(&s->common, AUX_ACK); - ps2_queue(&s->common, s->mouse_status); - ps2_queue(&s->common, s->mouse_resolution); - ps2_queue(&s->common, s->mouse_sample_rate); + ps2_queue_bytes(&s->common, 4, + AUX_ACK, + s->mouse_status, + s->mouse_resolution, + s->mouse_sample_rate); break; case AUX_POLL: - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); ps2_mouse_send_packet(s); break; case AUX_ENABLE_DEV: s->mouse_status |= MOUSE_STATUS_ENABLED; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_DISABLE_DEV: s->mouse_status &= ~MOUSE_STATUS_ENABLED; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_SET_DEFAULT: s->mouse_sample_rate = 100; s->mouse_resolution = 2; s->mouse_status = 0; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); break; case AUX_RESET: s->mouse_sample_rate = 100; @@ -780,9 +833,10 @@ void ps2_write_mouse(void *opaque, int val) s->mouse_status = 0; s->mouse_type = 0; ps2_reset_queue(&s->common); - ps2_queue(&s->common, AUX_ACK); - ps2_queue(&s->common, 0xaa); - ps2_queue(&s->common, s->mouse_type); + ps2_queue_bytes(&s->common, 3, + AUX_ACK, + 0xaa, + s->mouse_type); break; default: break; @@ -816,12 +870,12 @@ void ps2_write_mouse(void *opaque, int val) s->mouse_detect_state = 0; break; } - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); s->common.write_cmd = -1; break; case AUX_SET_RES: s->mouse_resolution = val; - ps2_queue(&s->common, AUX_ACK); + ps2_queue_raise(&s->common, AUX_ACK); s->common.write_cmd = -1; break; }