Re: [Qemu-devel] [PATCH 08/10] pckbd: adding new fields to vmstate

[Paolo Bonzini <pbonzini@redhat.com>]

Il 09/09/2014 15:07, Juan Quintela ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>>
>> This patch adds outport to VMState to allow correct saving and restoring
>> the state of PC keyboard controller.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Acked-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/input/pckbd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
>> index 2ab8c87..2b0cd3d 100644
>> --- a/hw/input/pckbd.c
>> +++ b/hw/input/pckbd.c
>> @@ -131,6 +131,7 @@ typedef struct KBDState {
>>      uint8_t status;
>>      uint8_t mode;
>>      uint8_t outport;
>> +    bool outport_present;
> 
> I don't like this one, but ....
> 
> 
>>      /* Bitmask of devices with data available.  */
>>      uint8_t pending;
>>      void *kbd;
>> @@ -367,18 +368,68 @@ static void kbd_reset(void *opaque)
>>      s->mode = KBD_MODE_KBD_INT | KBD_MODE_MOUSE_INT;
>>      s->status = KBD_STAT_CMD | KBD_STAT_UNLOCKED;
>>      s->outport = KBD_OUT_RESET | KBD_OUT_A20;
>> +    s->outport_present = false;
>> +}
>> +
>> +static uint8_t kbd_outport_default(KBDState *s)
>> +{
>> +    return KBD_OUT_RESET | KBD_OUT_A20
>> +           | (s->status & KBD_STAT_OBF ? KBD_OUT_OBF : 0)
>> +           | (s->status & KBD_STAT_MOUSE_OBF ? KBD_OUT_MOUSE_OBF : 0);
>> +}
>> +
>> +static int kbd_outport_post_load(void *opaque, int version_id)
>> +{
>> +    KBDState *s = opaque;
>> +    s->outport_present = true;
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_kbd_outport = {
>> +    .name = "pckbd_outport",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .post_load = kbd_outport_post_load,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_UINT8(outport, KBDState),
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static bool kbd_outport_needed(void *opaque)
>> +{
>> +    KBDState *s = opaque;
>> +    return s->outport != kbd_outport_default(s);
>> +}
>> +
>> +static int kbd_post_load(void *opaque, int version_id)
>> +{
>> +    KBDState *s = opaque;
> 
> Only solution that I thought of is putting here:
> 
> 
>      s->outport |=
>                 | (s->status & KBD_STAT_OBF ? KBD_OUT_OBF : 0)
>                 | (s->status & KBD_STAT_MOUSE_OBF ? KBD_OUT_MOUSE_OBF : 0);
> 
> 
> But I am not sure if that bits can be off if status bits are on.

Yes, they can---the outport can be written by the guest (see
outport_write).  That was my first thought as well. :)

Paolo

> Thinking about it, why it is that it is not necessary to have on
> postload something like that?
> 
> PD: no, I don't claim to understand how the pc keyboard work ...
> 
> Later, Juan.
> 
>