From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:41338) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZGnF-0005In-Um for qemu-devel@nongnu.org; Wed, 22 Jun 2011 02:20:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QZGnE-0005bq-KZ for qemu-devel@nongnu.org; Wed, 22 Jun 2011 02:20:05 -0400 Received: from smtp.ispras.ru ([83.149.198.202]:41876) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZGnD-0005ab-9f for qemu-devel@nongnu.org; Wed, 22 Jun 2011 02:20:04 -0400 From: "Pavel Dovgaluk" References: <001501cc300e$0b66bea0$22343be0$@Dovgaluk@ispras.ru> In-Reply-To: Date: Wed, 22 Jun 2011 10:19:03 +0400 Message-ID: <001601cc30a4$48964840$d9c2d8c0$@Dovgaluk@ispras.ru> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Language: ru Subject: Re: [Qemu-devel] [PATCH] Fix serial interface vmstate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com 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? 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