| Message ID | 001501cc300e$0b66bea0$22343be0$@Dovgaluk@ispras.ru |
|---|---|
| State | New |
| Headers | show |
"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? > + /* 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. > +} > + > + > static void serial_ioport_write(void *opaque, uint32_t addr, uint32_t val) > { > SerialState *s = opaque; > @@ -414,50 +470,7 @@ static void serial_ioport_write(void *opaque, uint32_t addr, uint32_t val) > } > break; > case 2: > - val = val & 0xFF; > - > - if (s->fcr == val) > - break; > - > - /* 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; > - serial_update_irq(s); > + serial_write_fcr(s, val, 0); > break; > case 3: > { > @@ -673,20 +686,38 @@ static int serial_post_load(void *opaque, int version_id) > s->fcr_vmstate = 0; > } > /* Initialize fcr via setter to perform essential side-effects */ > - serial_ioport_write(s, 0x02, s->fcr_vmstate); > + serial_write_fcr(s, s->fcr_vmstate, 1); > serial_update_parameters(s); > return 0; > } > > + > +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. Later, Juan.
> "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? And how to create them? Pavel Dovgaluk
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
> >>> 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 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
On 2011-06-22 10:58, Pavel Dovgaluk wrote: >>> 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. E.g., if the fifo is empty, you do not need to save its content. That would be one part of the condition. Go through all fields and check if they have states that could be ignored or if they could be ignored if other already saved fields have specific values. If you find any new field that must always be restored, let us discuss it. It may turn out that a substate is unrealistic, then we need to go with a new version. > >> 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? Default PC configurations contain this port. But that doesn't mean guests actively use. Jan
> >>> 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. > > E.g., if the fifo is empty, you do not need to save its content. That > would be one part of the condition. Go through all fields and check if > they have states that could be ignored or if they could be ignored if > other already saved fields have specific values. If you find any new > field that must always be restored, let us discuss it. It may turn out > that a substate is unrealistic, then we need to go with a new version. You mean, if FIFO is empty an will not be saved, we will have to clear it before loading every time? So there should be multiple subsections for every possible field? E.g. timers are saved only if they are pending, thr_ipending is saved only when it is nonzero, and so on. Do you mean that? Pavel Dovgaluk
On 2011-06-22 11:15, Pavel Dovgaluk wrote: >>>>> 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. >> >> E.g., if the fifo is empty, you do not need to save its content. That >> would be one part of the condition. Go through all fields and check if >> they have states that could be ignored or if they could be ignored if >> other already saved fields have specific values. If you find any new >> field that must always be restored, let us discuss it. It may turn out >> that a substate is unrealistic, then we need to go with a new version. > > You mean, if FIFO is empty an will not be saved, we will have to clear > it before loading every time? We (soon, patches posted) do a reset before every loadvm. We already do this when starting a new machine. That sets all states that aren't restored to defaults. In case of the fifo, they are all emptied. > So there should be multiple subsections for every possible field? > E.g. timers are saved only if they are pending, thr_ipending is saved > only when it is nonzero, and so on. Do you mean that? Nope, only a single subsection. Either we are able to avoid any new field under certain circumstances or we have to break the legacy format anyway. Jan
> On 2011-06-22 11:15, Pavel Dovgaluk wrote: > >>>>> 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. > >> > >> E.g., if the fifo is empty, you do not need to save its content. That > >> would be one part of the condition. Go through all fields and check if > >> they have states that could be ignored or if they could be ignored if > >> other already saved fields have specific values. If you find any new > >> field that must always be restored, let us discuss it. It may turn out > >> that a substate is unrealistic, then we need to go with a new version. > > > > You mean, if FIFO is empty an will not be saved, we will have to clear > > it before loading every time? > > We (soon, patches posted) do a reset before every loadvm. We already do > this when starting a new machine. That sets all states that aren't > restored to defaults. In case of the fifo, they are all emptied. > > > So there should be multiple subsections for every possible field? > > E.g. timers are saved only if they are pending, thr_ipending is saved > > only when it is nonzero, and so on. Do you mean that? > > Nope, only a single subsection. Either we are able to avoid any new > field under certain circumstances or we have to break the legacy format > anyway. Fields added by my patch are not belong to single subsection. Consider THR and TSR registers: THR: Bit 5 in the LSR, line status register can be used to check if new information must be written to THR. The value 1 indicates that the register is empty. TSR: Contains data, that should be shifted out. This register is Interconnected with transmit_timer - when timer event occurs, the data is shifted out. There are also other fields, that describe the state of the serial interface, but cannot be directly derived from THR and TSR state. So, there are the following ways, as I see it: 1. Make several subsections - one for every substate. 2. Make one subsection with very complex "needed" condition, which will be true, when any part of the subsection should be written. I think, that such complex condition will be too hard for testing. 3. Change version of the format. Pavel Dovgaluk
On 2011-06-22 12:13, Pavel Dovgaluk wrote: >> On 2011-06-22 11:15, Pavel Dovgaluk wrote: >>>>>>> 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. >>>> >>>> E.g., if the fifo is empty, you do not need to save its content. That >>>> would be one part of the condition. Go through all fields and check if >>>> they have states that could be ignored or if they could be ignored if >>>> other already saved fields have specific values. If you find any new >>>> field that must always be restored, let us discuss it. It may turn out >>>> that a substate is unrealistic, then we need to go with a new version. >>> >>> You mean, if FIFO is empty an will not be saved, we will have to clear >>> it before loading every time? >> >> We (soon, patches posted) do a reset before every loadvm. We already do >> this when starting a new machine. That sets all states that aren't >> restored to defaults. In case of the fifo, they are all emptied. >> >>> So there should be multiple subsections for every possible field? >>> E.g. timers are saved only if they are pending, thr_ipending is saved >>> only when it is nonzero, and so on. Do you mean that? >> >> Nope, only a single subsection. Either we are able to avoid any new >> field under certain circumstances or we have to break the legacy format >> anyway. > > Fields added by my patch are not belong to single subsection. > Consider THR and TSR registers: > THR: > Bit 5 in the LSR, line status register can be used to check if new > information must be written to THR. The value 1 indicates that the > register is empty. > TSR: > Contains data, that should be shifted out. This register is > Interconnected with transmit_timer - when timer event occurs, > the data is shifted out. > There are also other fields, that describe the state of the serial > interface, but cannot be directly derived from THR and TSR state. Neither thr nor tsr are set to a specific value on reset. That indicates that both depend on other states to be valid. Looks like tsr_retry > 0 would be the condition for a tsr worth saving e.g. > > So, there are the following ways, as I see it: > 1. Make several subsections - one for every substate. Again, that comes with no benefits. > 2. Make one subsection with very complex "needed" condition, > which will be true, when any part of the subsection should be > written. I think, that such complex condition will be too hard > for testing. > 3. Change version of the format. I still don't think we have found the case that requires a version change. Jan
Am 22.06.2011 um 10:58 schrieb Pavel Dovgaluk: >>>>> 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> > If the port is disabled, the state will not be saved, isn't it? See the ISA reconfigurability / PReP series for a counter-example [1]. Whether the state is saved depends on whether the isa-serial qdev device is instantiated, not on whether the port is active. Therefore a subsection will be even more useful here. Andreas [1] http://patchwork.ozlabs.org/patch/100274/
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; + + /* 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); + } +} + + static void serial_ioport_write(void *opaque, uint32_t addr, uint32_t val) { SerialState *s = opaque; @@ -414,50 +470,7 @@ static void serial_ioport_write(void *opaque, uint32_t addr, uint32_t val) } break; case 2: - val = val & 0xFF; - - if (s->fcr == val) - break; - - /* 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; - serial_update_irq(s); + serial_write_fcr(s, val, 0); break; case 3: { @@ -673,20 +686,38 @@ static int serial_post_load(void *opaque, int version_id) s->fcr_vmstate = 0; } /* Initialize fcr via setter to perform essential side-effects */ - serial_ioport_write(s, 0x02, s->fcr_vmstate); + serial_write_fcr(s, s->fcr_vmstate, 1); serial_update_parameters(s); return 0; } + +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() } };
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> --- hw/serial.c | 133 ++++++++++++++++++++++++++++++++++++++-------------------- 1 files changed, 87 insertions(+), 46 deletions(-)