From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:48788) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZJHn-0006yo-ME for qemu-devel@nongnu.org; Wed, 22 Jun 2011 04:59:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1QZJHl-0001Jz-Ox for qemu-devel@nongnu.org; Wed, 22 Jun 2011 04:59:47 -0400 Received: from smtp.ispras.ru ([83.149.198.202]:42402) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1QZJHl-0001Jo-2k for qemu-devel@nongnu.org; Wed, 22 Jun 2011 04:59:45 -0400 From: "Pavel Dovgaluk" References: <001501cc300e$0b66bea0$22343be0$@Dovgaluk@ispras.ru> <49270.9042774097$1308723700@news.gmane.org> <4E01A3FB.5050706@siemens.com> In-Reply-To: <4E01A3FB.5050706@siemens.com> Date: Wed, 22 Jun 2011 12:58:49 +0400 Message-ID: <003c01cc30ba$9a3b7560$ceb26020$@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: 'Jan Kiszka' Cc: 'qemu-devel' , quintela@redhat.com > >>> 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 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