All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Kiszka <jan.kiszka@siemens.com>
To: Pavel Dovgaluk <Pavel.Dovgaluk@ispras.ru>
Cc: 'qemu-devel' <qemu-devel@nongnu.org>, quintela@redhat.com
Subject: Re: [Qemu-devel] [PATCH] Fix serial interface vmstate
Date: Wed, 22 Jun 2011 10:12:43 +0200	[thread overview]
Message-ID: <4E01A3FB.5050706@siemens.com> (raw)
In-Reply-To: <49270.9042774097$1308723700@news.gmane.org>

On 2011-06-22 08:19, Pavel Dovgaluk wrote:
>> "Pavel Dovgaluk" <Pavel.Dovgaluk@ispras.ru> 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 <pavel.dovgaluk@gmail.com>
>>
>> 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

  parent reply	other threads:[~2011-06-22  8:13 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-06-21 12:23 [Qemu-devel] [PATCH] Fix serial interface vmstate Pavel Dovgaluk
2011-06-21 13:31 ` Juan Quintela
2011-06-22  6:19   ` Pavel Dovgaluk
     [not found]   ` <49270.9042774097$1308723700@news.gmane.org>
2011-06-22  8:12     ` Jan Kiszka [this message]
2011-06-22  8:58       ` Pavel Dovgaluk
2011-06-22  9:10         ` Jan Kiszka
2011-06-22  9:15           ` Pavel Dovgaluk
2011-06-22  9:22             ` Jan Kiszka
2011-06-22 10:13               ` Pavel Dovgaluk
2011-06-22 16:14                 ` Jan Kiszka
     [not found]       ` <4e01af43.ce4ee50a.60ee.3b47SMTPIN_ADDED@mx.google.com>
2011-06-23 10:11         ` Andreas Färber

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4E01A3FB.5050706@siemens.com \
    --to=jan.kiszka@siemens.com \
    --cc=Pavel.Dovgaluk@ispras.ru \
    --cc=qemu-devel@nongnu.org \
    --cc=quintela@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.