Re: [Qemu-devel] [PATCH] Fix serial interface vmstate

[Jan Kiszka]

On 2011-06-22 08:19, Pavel Dovgaluk wrote:
>> "Pavel Dovgaluk" <address@hidden> wrote:
>>>   This patch fixes save/restore of serial interface's state.
>>>   It includes changing of fcr setter function (it now does not invoke
>>> an interrupt while loading vmstate), and saving/restoring all
>>> fields that describe the state of serial interface (including timers).
>>>
>>> Signed-off-by: Pavel Dovgalyuk <address@hidden>
>>
>> I think we can do this with a new subsection.
>>
>>> ---
>>>  hw/serial.c |  133 
>>> ++++++++++++++++++++++++++++++++++++++--------------------
>>>  1 files changed, 87 insertions(+), 46 deletions(-)
>>>
>>> diff --git a/hw/serial.c b/hw/serial.c
>>> index 0ee61dd..936e048 100644
>>> --- a/hw/serial.c
>>> +++ b/hw/serial.c
>>> @@ -362,6 +362,62 @@ static void serial_xmit(void *opaque)
>>>  }
>>>
>>>
>>> +/* Setter for FCR.
>>> +   is_load flag means, that value is set while loading VM state
>>> +   and interrupt should not be invoked */
>>> +static void serial_write_fcr(void *opaque, uint32_t val, int is_load)
>>> +{
>>> +    SerialState *s = opaque;
>>> +
>>> +    val = val & 0xFF;
>>> +
>>> +    if (s->fcr == val)
>>> +        return;
>>
>> This looks like a test.  if this is true, we don't need to restore the
>> other values, so we shouldbe safe, right?
> 
>   Yes, that's right.
>   Actually, this code is moved from serial_ioport_write into separate
> function. All checks and branches were not changed, only irq invocation was
> made conditional.
> 
>>> +    /* Did the enable/disable flag change? If so, make sure FIFOs get 
>>> flushed */
>>> +    if ((val ^ s->fcr) & UART_FCR_FE)
>>> +        val |= UART_FCR_XFR | UART_FCR_RFR;
>>> +
>>> +    /* FIFO clear */
>>> +
>>> +    if (val & UART_FCR_RFR) {
>>> +        qemu_del_timer(s->fifo_timeout_timer);
>>> +        s->timeout_ipending=0;
>>> +        fifo_clear(s,RECV_FIFO);
>>> +    }
>>> +
>>> +    if (val & UART_FCR_XFR) {
>>> +        fifo_clear(s,XMIT_FIFO);
>>> +    }
>>> +
>>> +    if (val & UART_FCR_FE) {
>>> +        s->iir |= UART_IIR_FE;
>>> +        /* Set RECV_FIFO trigger Level */
>>> +        switch (val & 0xC0) {
>>> +        case UART_FCR_ITL_1:
>>> +            s->recv_fifo.itl = 1;
>>> +            break;
>>> +        case UART_FCR_ITL_2:
>>> +            s->recv_fifo.itl = 4;
>>> +            break;
>>> +        case UART_FCR_ITL_3:
>>> +            s->recv_fifo.itl = 8;
>>> +            break;
>>> +        case UART_FCR_ITL_4:
>>> +            s->recv_fifo.itl = 14;
>>> +            break;
>>> +        }
>>> +    } else
>>> +        s->iir &= ~UART_IIR_FE;
>>> +
>>> +    /* Set fcr - or at least the bits in it that are supposed to "stick" */
>>> +    s->fcr = val & 0xC9;
>>> +    if (!is_load) {
>>> +        serial_update_irq(s);
>>> +    }
>>
>> we can put the serial_update_irq() at caller site.  Function is only
>> called twice, on place need to call serial_update_irq() and the other not.
> 
>   Yes, you right. I didn't think about it.
> 
>>> +static const VMStateDescription vmstate_fifo = {
>>> +    .name = "serial FIFO",
>>> +    .version_id = 1,
>>> +    .minimum_version_id = 1,
>>> +    .fields      = (VMStateField []) {
>>> +        VMSTATE_BUFFER(data, SerialFIFO),
>>> +        VMSTATE_UINT8(count, SerialFIFO),
>>> +        VMSTATE_UINT8(itl, SerialFIFO),
>>> +        VMSTATE_UINT8(tail, SerialFIFO),
>>> +        VMSTATE_UINT8(head, SerialFIFO),
>>> +        VMSTATE_END_OF_LIST()
>>> +    }
>>> +};
>>> +
>>> +
>>>  static const VMStateDescription vmstate_serial = {
>>>      .name = "serial",
>>> -    .version_id = 3,
>>> +    .version_id = 4,
>>>      .minimum_version_id = 2,
>>>      .pre_save = serial_pre_save,
>>>      .post_load = serial_post_load,
>>>      .fields      = (VMStateField []) {
>>>          VMSTATE_UINT16_V(divider, SerialState, 2),
>>>          VMSTATE_UINT8(rbr, SerialState),
>>> +        VMSTATE_UINT8_V(thr, SerialState, 4),
>>> +        VMSTATE_UINT8_V(tsr, SerialState, 4),
>>>          VMSTATE_UINT8(ier, SerialState),
>>>          VMSTATE_UINT8(iir, SerialState),
>>>          VMSTATE_UINT8(lcr, SerialState),
>>> @@ -695,6 +726,16 @@ static const VMStateDescription vmstate_serial = {
>>>          VMSTATE_UINT8(msr, SerialState),
>>>          VMSTATE_UINT8(scr, SerialState),
>>>          VMSTATE_UINT8_V(fcr_vmstate, SerialState, 3),
>>> +        VMSTATE_INT32_V(thr_ipending, SerialState, 4),
>>> +        VMSTATE_INT32_V(last_break_enable, SerialState, 4),
>>> +        VMSTATE_INT32_V(tsr_retry, SerialState, 4),
>>> +        VMSTATE_STRUCT(recv_fifo, SerialState, 4, vmstate_fifo, 
>>> SerialFIFO),
>>> +        VMSTATE_STRUCT(xmit_fifo, SerialState, 4, vmstate_fifo, 
>>> SerialFIFO),
>>> +        VMSTATE_TIMER_V(fifo_timeout_timer, SerialState, 4),
>>> +        VMSTATE_INT32_V(timeout_ipending, SerialState, 4),
>>> +        VMSTATE_TIMER_V(transmit_timer, SerialState, 4),
>>> +        VMSTATE_INT32_V(poll_msl, SerialState, 4),
>>> +        VMSTATE_TIMER_V(modem_status_poll, SerialState, 4),
>>>          VMSTATE_END_OF_LIST()
>>>      }
>>>  };
>>
>> Anyways, I think that it is better to split the change in two patches.
>> One that refactor the common code in another function.  And the other
>> that adds the VMSTATE bits, I can add the subsection part if you want.
> 
>   What is the purpose of subsections?

To skip the new fields whenever possible. That would allow to continue
saving a vmstate on a new version of qemu and then restoring it on an
older one.

So you have to implement a handler that checks the serial state on
savevm whether any of the new fields contains a state that requires to
be saved. Of any of them do, we have to throw that time-traveling over
board and create the subsection. If not, we can continue to write the
old state. That might be the case here if the guest does not use the
serial port or if the port is idle at the time of saving.

>   And how to create them?

See e.g. [1] with [2] (to fix my copy&paste mistake).

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/74679
[2] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/74685

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux