From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:35304) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZ14K-0007U2-Om for qemu-devel@nongnu.org; Tue, 21 Jun 2011 09:32:46 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QZ14I-0001yM-GT for qemu-devel@nongnu.org; Tue, 21 Jun 2011 09:32:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32110) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZ14I-0001y3-3f for qemu-devel@nongnu.org; Tue, 21 Jun 2011 09:32:38 -0400 From: Juan Quintela In-Reply-To: <001501cc300e$0b66bea0$22343be0$@Dovgaluk@ispras.ru> (Pavel Dovgaluk's message of "Tue, 21 Jun 2011 16:23:36 +0400") References: <001501cc300e$0b66bea0$22343be0$@Dovgaluk@ispras.ru> Date: Tue, 21 Jun 2011 15:31:03 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH] Fix serial interface vmstate Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgaluk Cc: 'qemu-devel' "Pavel Dovgaluk" 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 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.