From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36231) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UJ5Vl-0007DA-V8 for qemu-devel@nongnu.org; Fri, 22 Mar 2013 13:12:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UJ5Vk-0006xu-D6 for qemu-devel@nongnu.org; Fri, 22 Mar 2013 13:12:13 -0400 Received: from e23smtp08.au.ibm.com ([202.81.31.141]:44032) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UJ5Vj-0006xW-Rg for qemu-devel@nongnu.org; Fri, 22 Mar 2013 13:12:12 -0400 Received: from /spool/local by e23smtp08.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Sat, 23 Mar 2013 03:10:08 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id B95C12BB0050 for ; Sat, 23 Mar 2013 04:12:05 +1100 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r2MGx08v66125984 for ; Sat, 23 Mar 2013 03:59:01 +1100 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r2MHC4OI005332 for ; Sat, 23 Mar 2013 04:12:04 +1100 From: Anthony Liguori In-Reply-To: <514C8BCD.4020107@redhat.com> References: <87boadc2yp.fsf@codemonkey.ws> <1363883716-30289-1-git-send-email-alevy@redhat.com> <1363883716-30289-2-git-send-email-alevy@redhat.com> <87sj3o62gd.fsf@codemonkey.ws> <514C0EC3.3090202@redhat.com> <87wqszv8zr.fsf@codemonkey.ws> <514C8BCD.4020107@redhat.com> Date: Fri, 22 Mar 2013 12:11:48 -0500 Message-ID: <87wqsz2wbv.fsf@codemonkey.ws> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: Re: [Qemu-devel] [PATCH 1/2] char: add qemu_chr_be_is_fe_connected List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Hans de Goede Cc: amit.shah@redhat.com, Alon Levy , qemu-devel@nongnu.org, kraxel@redhat.com Hans de Goede writes: > Hi, > > On 03/22/2013 02:50 PM, Anthony Liguori wrote: >> Hans de Goede writes: >> >> We should have never allowed that in the first place and >> I object strongly to extending the concept without making it make sense >> for everything else. >> >>> Frontends end inside the guest usually in the form of some piece of >>> virtual hardware inside the guest. Just because the virtual hardware >>> is there does not mean the guest has a driver, >> >> Okay, let's use your example here with a standard UART. In the >> following sequence, I should receive: >> >> 1) Starts guest >> 2) When guest initializes the UART, qemu_chr_fe_open() >> 3) Reboot guest >> 4) Receive qemu_chr_fe_close() >> 5) Boot new guest without a UART driver >> 6) Nothing is received >> >> But this doesn't happen today because the only backend that cares >> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write(). > > 1) If the guest does not have an uart driver, nothing will be written > to the uart, so it wont call qemu_chr_fe_write and we won't assume > a qemu_chr_fe_open > > 2) But I agree the assuming of qemu_chr_fe_open on the first write is > a hack, it stems from before we had qemu_chr_fe_open/_close and now that > we do have we should remove it. If the qemu-char.c code did: int qemu_chr_fe_write(...) { if (!s->fe_open) { qemu_chr_fe_open(s); } ... } That would be one thing. It's a hack, but a more reasonable hack than doing this in the backend like we do today. And in fact, herein lies exactly what the problem is. There is no "s->fe_open". And if such a thing did exist, we would note that this is in fact guest state and that it needs to be taken care of during migration. > Note btw that many backends also don't handle sending CHR_EVENT_OPENED / > _CLOSED themselves, they simply use qemu_chr_generic_open and that generated > the opened event once on creation of the device. So the concept is just > as broken / not broken on the backend side. I know :-/ > We could do the same and have a qemu_fe_generic_open for frontends which > does the qemu_chr_fe_open call. You mean, in qemu_chr_fe_write()? Yes, I think not doing this bit in the backend and making it part of qemu-char would be a significant improvement and would also guide in solving this problem correctly. Also, you can get reset handling for free by unconditionally clearly s->fe_open with a reset handler. That would also simplify the virtio-serial code since it would no longer need the reset logic. >> And for me, the most logical thing is to call qemu_chr_fe_open() in >> post_load for the device. > > With the device I assume you mean the frontend? That is what we originally > suggested and submitted a patch for (for virtio-console) but Amit didn't > like it. As Avi would say, this is "derived guest state" and derived guest state has to be recomputed in post_load handlers. Regards, Anthony Liguori > > Regards, > > Hans