From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52561) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeJxa-0006mE-Bv for qemu-devel@nongnu.org; Fri, 11 Mar 2016 05:06:24 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aeJxU-000255-1y for qemu-devel@nongnu.org; Fri, 11 Mar 2016 05:06:18 -0500 Received: from mail-wm0-x242.google.com ([2a00:1450:400c:c09::242]:35715) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aeJxT-00024m-NC for qemu-devel@nongnu.org; Fri, 11 Mar 2016 05:06:11 -0500 Received: by mail-wm0-x242.google.com with SMTP id n205so1442344wmf.2 for ; Fri, 11 Mar 2016 02:06:11 -0800 (PST) Sender: Paolo Bonzini References: <20160310115551.4812.55431.stgit@PASHA-ISP> <20160310115557.4812.55284.stgit@PASHA-ISP> <56E1676D.9000703@redhat.com> <000c01d17b5e$01123dd0$0336b970$@ru> From: Paolo Bonzini Message-ID: <56E2988E.4080401@redhat.com> Date: Fri, 11 Mar 2016 11:06:06 +0100 MIME-Version: 1.0 In-Reply-To: <000c01d17b5e$01123dd0$0336b970$@ru> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 1/5] replay: character devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Dovgalyuk , 'Pavel Dovgalyuk' , qemu-devel@nongnu.org Cc: edgar.iglesias@xilinx.com, peter.maydell@linaro.org, igor.rubinov@gmail.com, mark.burton@greensocs.com, real@ispras.ru, batuzovk@ispras.ru, maria.klimushenkova@ispras.ru, stefanha@redhat.com, kwolf@redhat.com, hines@cert.org, alex.bennee@linaro.org, fred.konrad@greensocs.com On 11/03/2016 07:19, Pavel Dovgalyuk wrote: >> From: Paolo Bonzini [mailto:pbonzini@redhat.com] >> On 10/03/2016 12:55, Pavel Dovgalyuk wrote: >>> gdbstub which also acts as a backend is not recorded to allow controlling >>> the replaying through gdb. >> >> Perhaps the monitor too? > > Right. I'll check that it works. > >> Overall the patch is nice and can definitely go in 2.6, but there are a >> couple changes to do... >> >>> @@ -245,6 +246,9 @@ int qemu_chr_fe_write(CharDriverState *s, const uint8_t *buf, int len) >>> qemu_chr_fe_write_log(s, buf, ret); >>> } >>> >>> + if (s->replay) { >>> + replay_data_int(&ret); >>> + } >> >> I think this is wrong. The logic should be >> >> if (replaying) { >> read event(&ret); >> assert(ret <= len); >> len = ret; >> } >> >> qemu_mutex_lock(&s->chr_write_lock); >> ret = s->chr_write(s, buf, len); >> >> if (ret > 0) { >> qemu_chr_fe_write_log(s, buf, ret); >> } >> qemu_mutex_unlock(&s->chr_write_lock); >> >> if (recording) { >> write event(ret); >> } >> >>> qemu_mutex_unlock(&s->chr_write_lock); >>> return ret; > > In this case return value in record and replay modes may differ > and the behavior of caller won't be deterministic. > E.g., > > static gboolean cadence_uart_xmit(GIOChannel *chan, GIOCondition cond, > void *opaque) > { > ... > ret = qemu_chr_fe_write(s->chr, s->tx_fifo, s->tx_count); > s->tx_count -= ret; > memmove(s->tx_fifo, s->tx_fifo + ret, s->tx_count); > ... > } What you are doing is actually worse. Say you are writing 20 bytes, and at recording time the chardev could only write 10. At replay time, you will write 20 but perhaps the chardev (which is an external program, remember) this time could write 15. Now you have written 15 characters, but you tell the device model that you have written 10. The result is that you'll write the 11th to 15th characters twice. Likewise you could lose characters if the chardev cannot satisfy the write at replay time. With my version the latter is still possible, but duplicated characters are not. So perhaps: if (replaying) { read event(&ret); assert(ret <= len); ret = qemu_chr_fe_write_all(s, buf, ret); return ret; } qemu_mutex_lock(&s->chr_write_lock); ret = s->chr_write(s, buf, len); if (ret > 0) { qemu_chr_fe_write_log(s, buf, ret); } qemu_mutex_unlock(&s->chr_write_lock); if (recording) { write event(ret); } return ret; Paolo