| Message ID | 20210807121202.6294-3-vr_qemu@t-online.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] ps2: use the whole ps2 buffer but keep queue size | expand |
Hi, > +static bool ps2_keyboard_cqueue_needed(void *opaque) > +{ > + PS2KbdState *s = opaque; > + > + return s->common.queue.cwptr != -1; /* the queue is mostly empty */ > +} > + > +static const VMStateDescription vmstate_ps2_keyboard_cqueue = { > + .name = "ps2kbd/command_reply_queue", > + .needed = ps2_keyboard_cqueue_needed, > + .fields = (VMStateField[]) { > + VMSTATE_INT32(common.queue.cwptr, PS2KbdState), > + VMSTATE_END_OF_LIST() > + } > +}; Not going to work (the existing vmstate_ps2_keyboard_need_high_bit has the same problem btw). Chicken and egg problem on the receiving side: How you properly evaluate ps2_keyboard_cqueue_needed() without having common.queue.cwptr received yet? With "cqueue not needed" being the common case migration will work nicely in most cases nevertheless, but when the source machine actually sends cqueue state things will break ... Looking at data sent via vmstate works but ordering is important. You could check write_cmd because that is sent in the migration data stream before ps2_keyboard_cqueue_needed() will be evaluated (just an random example for the ordering, probably wouldn't help much in this case). There is some redundancy in the data stream (wptr + rptr would be enough, but we also send count). Maybe that can be used somehow. Of course the tricky part is to not confuse old qemu versions on the receiving end. If we can't find something we can add a property simliar to the one for the extended keyboard state. take care, Gerd
> Hi, > >> +static bool ps2_keyboard_cqueue_needed(void *opaque) >> +{ >> + PS2KbdState *s = opaque; >> + >> + return s->common.queue.cwptr != -1; /* the queue is mostly empty */ >> +} >> + >> +static const VMStateDescription vmstate_ps2_keyboard_cqueue = { >> + .name = "ps2kbd/command_reply_queue", >> + .needed = ps2_keyboard_cqueue_needed, >> + .fields = (VMStateField[]) { >> + VMSTATE_INT32(common.queue.cwptr, PS2KbdState), >> + VMSTATE_END_OF_LIST() >> + } >> +}; > Not going to work (the existing vmstate_ps2_keyboard_need_high_bit has > the same problem btw). Chicken and egg problem on the receiving side: > How you properly evaluate ps2_keyboard_cqueue_needed() without having > common.queue.cwptr received yet? > > With "cqueue not needed" being the common case migration will work > nicely in most cases nevertheless, but when the source machine actually > sends cqueue state things will break ... Hi Gerd, this part actually works. .needed is only evaluated on the sending side. For the receiving side subsections are optional. Migration doesn't fail if a subsection isn't loaded. Before I sent this patch series one of the migration tests was a migration from 6.0.92 to 6.0.92 with one byte in the command reply queue and 3 bytes in the scancode queue. The migration didn't fail. Migration will fail in the rare case when a new QEMU sends the command_reply_queue subsection to an old QEMU version. > Looking at data sent via vmstate works but ordering is important. You > could check write_cmd because that is sent in the migration data stream > before ps2_keyboard_cqueue_needed() will be evaluated (just an random > example for the ordering, probably wouldn't help much in this case). > > There is some redundancy in the data stream (wptr + rptr would be > enough, but we also send count). Maybe that can be used somehow. > Of course the tricky part is to not confuse old qemu versions on > the receiving end. > > If we can't find something we can add a property simliar to the one > for the extended keyboard state. What is the best way to add such a compat property? The ps2 keyboard isn't a qdev device. I can't just add a property to the device class. Do I have to add a property to the i8042 and the pl050 device and propagate the property value with the ps2_kbd_init() call to the PS2KbdState? With best regards, Volker > take care, > Gerd >
Hi, > this part actually works. .needed is only evaluated on the sending side. For > the receiving side subsections are optional. Migration doesn't fail if a > subsection isn't loaded. Before I sent this patch series one of the > migration tests was a migration from 6.0.92 to 6.0.92 with one byte in the > command reply queue and 3 bytes in the scancode queue. The migration didn't > fail. Hmm, ok. If you actually tested it you are probably right. My memory tells me ->needed() is evaluated on both sending and receiving side as the migration data stream does not carry the information whenever a subsection is present or not. But maybe my memories are wrong, or things have changed, I don't follow migration changes that closely. > > If we can't find something we can add a property simliar to the one > > for the extended keyboard state. > > What is the best way to add such a compat property? The ps2 keyboard isn't a > qdev device. I can't just add a property to the device class. Do I have to > add a property to the i8042 and the pl050 device and propagate the property > value with the ps2_kbd_init() call to the PS2KbdState? Yes, I think so. But double-check the migration thing first, if your approach works that is the easier way of course. take care, Gerd
> Hi, > >> this part actually works. .needed is only evaluated on the sending side. For >> the receiving side subsections are optional. Migration doesn't fail if a >> subsection isn't loaded. Before I sent this patch series one of the >> migration tests was a migration from 6.0.92 to 6.0.92 with one byte in the >> command reply queue and 3 bytes in the scancode queue. The migration didn't >> fail. > Hmm, ok. If you actually tested it you are probably right. My memory > tells me ->needed() is evaluated on both sending and receiving side as > the migration data stream does not carry the information whenever a > subsection is present or not. But maybe my memories are wrong, or > things have changed, I don't follow migration changes that closely. > >>> If we can't find something we can add a property simliar to the one >>> for the extended keyboard state. >> What is the best way to add such a compat property? The ps2 keyboard isn't a >> qdev device. I can't just add a property to the device class. Do I have to >> add a property to the i8042 and the pl050 device and propagate the property >> value with the ps2_kbd_init() call to the PS2KbdState? > Yes, I think so. But double-check the migration thing first, if your > approach works that is the easier way of course. > > take care, > Gerd > Hi Gerd, this are the results of 5 migrations. I added a trace statement to function ps2_common_post_load() in my qemu-master. The first trace line is for ps2kbd, the second for ps2mouse. #1 Migrate qemu 6.0.92 to qemu 6.0.92 2 scancodes in ps2 keyboard queue ps2_common_post_load: count 2, ccount 0 ps2_common_post_load: count 0, ccount 0 migration OK ./scripts/analyze-migration.py -f /var/tmp/qemu-state -d desc | less { "name": "ps2kbd", "instance_id": 0, "vmsd_name": "ps2kbd", "version": 3, "fields": [ { "name": "common", "type": "struct", "struct": { "vmsd_name": "PS2 Common State", "version": 3, "fields": [ { "name": "write_cmd", "type": "int32", "size": 4 }, { "name": "queue.rptr", "type": "int32", "size": 4 }, { "name": "queue.wptr", "type": "int32", "size": 4 }, { "name": "queue.count", "type": "int32", "size": 4 }, { "name": "queue.data", "type": "buffer", "size": 256 } ] }, "size": 272 }, #2 Migrate qemu 6.0.92 to qemu 6.0.92 1 command reply and 2 scancodes in ps2 keyboard queue ps2_common_post_load: count 3, ccount 1 ps2_common_post_load: count 0, ccount 0 migration OK ./scripts/analyze-migration.py -f /var/tmp/qemu-state -d desc | less { "name": "ps2kbd", "instance_id": 0, "vmsd_name": "ps2kbd", "version": 3, "fields": [ { "name": "common", "type": "struct", "struct": { "vmsd_name": "PS2 Common State", "version": 3, "fields": [ { "name": "write_cmd", "type": "int32", "size": 4 }, { "name": "queue.rptr", "type": "int32", "size": 4 }, { "name": "queue.wptr", "type": "int32", "size": 4 }, { "name": "queue.count", "type": "int32", "size": 4 }, { "name": "queue.data", "type": "buffer", "size": 256 } ] }, "size": 272 }, { "name": "scan_enabled", "type": "int32", "size": 4 }, { "name": "translate", "type": "int32", "size": 4 }, { "name": "scancode_set", "type": "int32", "size": 4 } ], "subsections": [ { "vmsd_name": "ps2kbd/command_reply_queue", "version": 0, "fields": [ { "name": "common.queue.cwptr", "type": "int32", "size": 4 } ] } ] }, #3 Migrate qemu 5.2.0 to qemu 6.0.92 4 scancodes in ps2 keyboard queue ps2_common_post_load: count 4, ccount 0 ps2_common_post_load: count 0, ccount 0 Migration OK #4 Migrate qemu 6.0.92 to qemu 5.2.0 2 scancodes in ps2 keyboard queue Migration OK #5 Migrate qemu 6.0.92 to qemu 5.2.0 1 command reply and 2 scancodes in ps2 keyboard queue qemu-system-x86_64: error while loading state for instance 0x0 of device 'ps2kbd' qemu-system-x86_64: load of migration failed: No such file or directory Migration FAILED With best regards, Volker
diff --git a/hw/input/ps2.c b/hw/input/ps2.c index 8c06fd7fb4..9376a8f4ce 100644 --- a/hw/input/ps2.c +++ b/hw/input/ps2.c @@ -80,6 +80,7 @@ */ #define PS2_BUFFER_SIZE 256 #define PS2_QUEUE_SIZE 16 /* Queue size required by PS/2 protocol */ +#define PS2_QUEUE_HEADROOM 8 /* Queue size for keyboard command replies */ /* Bits for 'modifiers' field in PS2KbdState */ #define MOD_CTRL_L (1 << 0) @@ -985,17 +986,27 @@ static void ps2_common_reset(PS2State *s) static void ps2_common_post_load(PS2State *s) { PS2Queue *q = &s->queue; + int ccount = 0; - /* set the useful data buffer queue size <= PS2_QUEUE_SIZE */ - if (q->count < 0) { - q->count = 0; - } else if (q->count > PS2_QUEUE_SIZE) { - q->count = PS2_QUEUE_SIZE; + /* limit the number of queued command replies to PS2_QUEUE_HEADROOM */ + if (q->cwptr != -1) { + ccount = (q->cwptr - q->rptr) & (PS2_BUFFER_SIZE - 1); + if (ccount > PS2_QUEUE_HEADROOM) { + ccount = PS2_QUEUE_HEADROOM; + } + } + + /* limit the scancode queue size to PS2_QUEUE_SIZE */ + if (q->count < ccount) { + q->count = ccount; + } else if (q->count > ccount + PS2_QUEUE_SIZE) { + q->count = ccount + PS2_QUEUE_SIZE; } - /* sanitize rptr and recalculate wptr */ + /* sanitize rptr and recalculate wptr and cwptr */ q->rptr = q->rptr & (PS2_BUFFER_SIZE - 1); q->wptr = (q->rptr + q->count) & (PS2_BUFFER_SIZE - 1); + q->cwptr = ccount ? (q->rptr + ccount) & (PS2_BUFFER_SIZE - 1) : -1; } static void ps2_kbd_reset(void *opaque) @@ -1086,6 +1097,22 @@ static const VMStateDescription vmstate_ps2_keyboard_need_high_bit = { } }; +static bool ps2_keyboard_cqueue_needed(void *opaque) +{ + PS2KbdState *s = opaque; + + return s->common.queue.cwptr != -1; /* the queue is mostly empty */ +} + +static const VMStateDescription vmstate_ps2_keyboard_cqueue = { + .name = "ps2kbd/command_reply_queue", + .needed = ps2_keyboard_cqueue_needed, + .fields = (VMStateField[]) { + VMSTATE_INT32(common.queue.cwptr, PS2KbdState), + VMSTATE_END_OF_LIST() + } +}; + static int ps2_kbd_post_load(void* opaque, int version_id) { PS2KbdState *s = (PS2KbdState*)opaque; @@ -1114,6 +1141,7 @@ static const VMStateDescription vmstate_ps2_keyboard = { .subsections = (const VMStateDescription*[]) { &vmstate_ps2_keyboard_ledstate, &vmstate_ps2_keyboard_need_high_bit, + &vmstate_ps2_keyboard_cqueue, NULL } };
Add migration support for the PS/2 keyboard command reply queue. Signed-off-by: Volker Rümelin <vr_qemu@t-online.de> --- hw/input/ps2.c | 40 ++++++++++++++++++++++++++++++++++------ 1 file changed, 34 insertions(+), 6 deletions(-)