From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:55413) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwC8j-0002a6-3F for qemu-devel@nongnu.org; Tue, 31 Jul 2012 09:05:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SwC8f-00058o-0N for qemu-devel@nongnu.org; Tue, 31 Jul 2012 09:05:33 -0400 Received: from e06smtp18.uk.ibm.com ([195.75.94.114]:55250) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SwC8e-00058W-O5 for qemu-devel@nongnu.org; Tue, 31 Jul 2012 09:05:28 -0400 Received: from /spool/local by e06smtp18.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 31 Jul 2012 14:05:27 +0100 Received: from d06av09.portsmouth.uk.ibm.com (d06av09.portsmouth.uk.ibm.com [9.149.37.250]) by d06nrmr1307.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q6VD5NVi2109636 for ; Tue, 31 Jul 2012 14:05:23 +0100 Received: from d06av09.portsmouth.uk.ibm.com (loopback [127.0.0.1]) by d06av09.portsmouth.uk.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q6VD5M7Y022775 for ; Tue, 31 Jul 2012 07:05:23 -0600 Message-ID: <5017D812.5090403@de.ibm.com> Date: Tue, 31 Jul 2012 15:05:22 +0200 From: Christian Borntraeger MIME-Version: 1.0 References: <1343292959-18308-1-git-send-email-borntraeger@de.ibm.com> <5717F05F-62A3-47DE-8961-12031F1713B5@suse.de> In-Reply-To: <5717F05F-62A3-47DE-8961-12031F1713B5@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 06/07] s390: sclp ascii console support List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: Blue Swirl , Heinz Graalfs , qemu-devel , Jens Freimann , afaerber@suse.de On 30/07/12 16:02, Alexander Graf wrote: >> + qemu_irq sclp_read_vt220; > > I'm sure this one wants a name that indicates it's an irq line ;) ok. > >> +} SCLPConsole; >> + >> +/* character layer call-back functions */ >> + >> +/* Return number of bytes that fit into iov buffer */ >> +static int chr_can_read(void *opaque) >> +{ >> + int can_read; >> + SCLPConsole *scon = opaque; >> + >> + qemu_mutex_lock(&scon->event.lock); > > Explicit locks now? IIRC Heinz introduced the locks since we have multiple threads working on the same kind of buffers (the cpu threads and the main thread). We could not verify that the main thread has the big qemu lock in all cases. Furthermore, this makes the code already thread safe if we get rid of the big qemu lock somewhen. But I agree, we have to double check why there are two different kind of locks. [...] >> + /* if new data do not fit into current buffer */ >> + if (scon->iov_data_len + size > SIZE_BUFFER_VT220) { >> + /* character layer sent more than allowed */ >> + qemu_mutex_unlock(&scon->event.lock); >> + return; > > So we drop the bytes it did send? Isn't there usually some can_read function from the char layer that we can indicate to that we have enough space? > > If so, then this should be an assert(), right? Probably yes. Will double check. [..] >> + SCLPConsole *cons = DO_UPCAST(SCLPConsole, event, event); >> + >> + /* first byte is hex 0 saying an ascii string follows */ >> + *buf++ = '\0'; >> + avail--; >> + /* if all data fit into provided SCLP buffer */ >> + if (avail >= cons->iov_sclp_rest) { >> + /* copy character byte-stream to SCLP buffer */ >> + memcpy(buf, cons->iov_sclp, cons->iov_sclp_rest); >> + *size = cons->iov_sclp_rest + 1; >> + cons->iov_sclp = cons->iov; >> + cons->iov_bs = cons->iov; >> + cons->iov_data_len = 0; >> + cons->iov_sclp_rest = 0; >> + event->event_pending = false; >> + /* data provided and no more data pending */ >> + } else { >> + /* if provided buffer is too small, just copy part */ >> + memcpy(buf, cons->iov_sclp, avail); >> + *size = avail + 1; >> + cons->iov_sclp_rest -= avail; >> + cons->iov_sclp += avail; >> + /* more data pending */ >> + } >> +} > > I'm wondering whether we actually need this indirection from > > chr layer -> buffer -> sclp buffer. > > Why can't we just do > > chr layer -> sclp buffer? > The sclp interface is a bit different here. On an input event, the hw generated an service interrupt with the event pending bit set. Then the guest kernel does a read event data sclp call with input buffer. The input data has to copied into that and then returned via an additional interrupt. Without the buffering our can_read function could only return 0 as size since there is yet no buffer. Makes sense? Christian