[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] Fix serial interface vmstate
From: |
Pavel Dovgaluk |
Subject: |
Re: [Qemu-devel] [PATCH] Fix serial interface vmstate |
Date: |
Wed, 22 Jun 2011 12:58:49 +0400 |
> >>> 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
I thought about splitting. First change is not for refactoring,
it is also a bugfix of non-deterministic loading of serial interface state.
Both part of my patch relate to the same problem - non-deterministic load.
> >> 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.
Do you have an idea how to implement "needed" function for my case?
Because I think, these fields should always be saved and loaded, because
they are related to the main state of the interface, not the kind of
optional substate.
> 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.
If the port is disabled, the state will not be saved, isn't it?
Pavel Dovgaluk
- [Qemu-devel] [PATCH] Fix serial interface vmstate, Pavel Dovgaluk, 2011/06/21
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Juan Quintela, 2011/06/21
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Pavel Dovgaluk, 2011/06/22
- Message not available
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Jan Kiszka, 2011/06/22
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate,
Pavel Dovgaluk <=
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Jan Kiszka, 2011/06/22
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Pavel Dovgaluk, 2011/06/22
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Jan Kiszka, 2011/06/22
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Pavel Dovgaluk, 2011/06/22
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Jan Kiszka, 2011/06/22
- Message not available
- Re: [Qemu-devel] [PATCH] Fix serial interface vmstate, Andreas Färber, 2011/06/23