From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 2B6BACA0FED for ; Fri, 5 Sep 2025 22:29:52 +0000 (UTC) Received: from list by lists.xenproject.org with outflank-mailman.1112507.1460735 (Exim 4.92) (envelope-from ) id 1uuew0-000731-IJ; Fri, 05 Sep 2025 22:29:32 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 1112507.1460735; Fri, 05 Sep 2025 22:29:32 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1uuew0-00072u-FG; Fri, 05 Sep 2025 22:29:32 +0000 Received: by outflank-mailman (input) for mailman id 1112507; Fri, 05 Sep 2025 22:29:31 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1uuevz-00072o-DF for xen-devel@lists.xenproject.org; Fri, 05 Sep 2025 22:29:31 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.96) (envelope-from ) id 1uuevy-0088zC-00; Fri, 05 Sep 2025 22:29:30 +0000 Received: from [19.12.91.86] (helo=localhost) by xenbits.xenproject.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from ) id 1uuevx-0002CS-2z; Fri, 05 Sep 2025 22:29:30 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID :Subject:Cc:To:Date:From; bh=REYTDTUYkn/978evtZltzG59+rC8/AC8/F3zDIATo+k=; b= ZbFnijt97/ro+jb9tbUBXoF4jA5ycemRKVaFgw0RwA3lzumF8z4mOQxUqTi6RXx/Z9NghzOIXvv9G Z9wpJuuRAjaKeMmIRc58VrDxaSbWfs5cHHdVvbQwlRAQzHYPfauF5sx5VIp3rQ83wK4Yfds2Amu3N NoKqOj2Yebf7APsEs=; From: dmukhin@xen.org Date: Fri, 5 Sep 2025 15:29:29 -0700 To: Stefano Stabellini Cc: xen-devel@lists.xenproject.org, andrew.cooper3@citrix.com, anthony.perard@vates.tech, jbeulich@suse.com, julien@xen.org, michal.orzel@amd.com, roger.pau@citrix.com, dmukhin@ford.com Subject: Re: [PATCH v5 06/15] emul/ns16x50: implement THR/RBR registers Message-ID: References: <20250828235409.2835815-1-dmukhin@ford.com> <20250828235409.2835815-7-dmukhin@ford.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: On Fri, Aug 29, 2025 at 01:34:39PM -0700, Stefano Stabellini wrote: > On Fri, 29 Aug 2025, Stefano Stabellini wrote: > > On Thu, 28 Aug 2025, dmukhin@xen.org wrote: > > > From: Denis Mukhin > > > > > > Add RBR/THR registers emulation to the I/O port handlder. > > > > > > Also, add RX/TX FIFO management code since RBR depends on RX FIFO and > > > THR depends on TX FIFO. > > > > > > FIFOs are not emulated as per UART specs for simplicity (not need to emulate > > > baud rate). Emulator does not emulate NS8250 (no FIFO), NS16550a (16 bytes) or > > > NS16750 (64 bytes). > > > > > > FIFOs are emulated by means of using xencons_interface which conveniently > > > provides primitives for buffer management and later can be used for > > > inter-domain communication similarly to vpl011. > > > > > > Signed-off-by: Denis Mukhin > > > --- > > > Changes since v4: > > > - new patch > > > --- > > > xen/common/emul/vuart/ns16x50.c | 135 ++++++++++++++++++++++++++++++++ > > > 1 file changed, 135 insertions(+) > > > > > > diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c > > > index 20597cc36b35..efb2f4c6441c 100644 > > > --- a/xen/common/emul/vuart/ns16x50.c > > > +++ b/xen/common/emul/vuart/ns16x50.c > > > @@ -92,6 +92,119 @@ static bool ns16x50_fifo_rx_empty(const struct vuart_ns16x50 *vdev) > > > return cons->in_prod == cons->in_cons; > > > } > > > > > > +static bool ns16x50_fifo_rx_full(const struct vuart_ns16x50 *vdev) > > > +{ > > > + const struct xencons_interface *cons = &vdev->cons; > > > + > > > + return cons->in_prod - cons->in_cons == ARRAY_SIZE(cons->in); > > > +} > > > + > > > +static void ns16x50_fifo_rx_reset(struct vuart_ns16x50 *vdev) > > > +{ > > > + struct xencons_interface *cons = &vdev->cons; > > > + > > > + cons->in_cons = cons->in_prod; > > > +} > > > + > > > +static int ns16x50_fifo_rx_getchar(struct vuart_ns16x50 *vdev) > > > +{ > > > + struct xencons_interface *cons = &vdev->cons; > > > + int rc; > > > + > > > + if ( ns16x50_fifo_rx_empty(vdev) ) > > > + { > > > + ns16x50_debug(vdev, "RX FIFO empty\n"); > > > + rc = -ENODATA; > > > + } > > > + else > > > + { > > > + rc = cons->in[MASK_XENCONS_IDX(cons->in_cons, cons->in)]; > > > + cons->in_cons++; > > > + } > > > + > > > + return rc; > > > > The signed integer to char conversion here is not great from a MISRA > > perspective. I think it would be better to keep rc as success/failure > > return value and take the read char as a pointer parameter. > > > > > > > +} > > > + > > > +static int ns16x50_fifo_rx_putchar(struct vuart_ns16x50 *vdev, char c) > > > +{ > > > + struct xencons_interface *cons = &vdev->cons; > > > + int rc; > > > + > > > + /* > > > + * FIFO-less 8250/16450 UARTs: newly arrived word overwrites the contents > > > + * of the THR. > > > + */ > > > + if ( ns16x50_fifo_rx_full(vdev) ) > > > + { > > > + ns16x50_debug(vdev, "RX FIFO full; resetting\n"); > > > + ns16x50_fifo_rx_reset(vdev); > > > + rc = -ENOSPC; > > > + } > > > + else > > > + rc = 0; > > > + > > > + cons->in[MASK_XENCONS_IDX(cons->in_prod, cons->in)] = c; > > > + cons->in_prod++; > > > + > > > + return rc; > > > +} > > > + > > > +static bool ns16x50_fifo_tx_empty(const struct vuart_ns16x50 *vdev) > > > +{ > > > + const struct xencons_interface *cons = &vdev->cons; > > > + > > > + return cons->out_prod == cons->out_cons; > > > +} > > > + > > > +static void ns16x50_fifo_tx_reset(struct vuart_ns16x50 *vdev) > > > +{ > > > + struct xencons_interface *cons = &vdev->cons; > > > + > > > + cons->out_prod = 0; > > > + ASSERT(cons->out_cons == cons->out_prod); > > > > I am not sure about this.. why resetting the out_prod index? In theory > > it could keep increasing until wrap around and still go forward thanks > > to the mask > > I get it now .. it is because it is called from ns16x50_fifo_tx_flush > right after printing to the real console. > > I think it is better to simply do: > > cons->out_cons = cons->out_prod; > > which effectively clears out the whole buffer. It is dangerous to do: That was OK to use TX buffer in such awkward manner because of the assertions and buffer size checks. I reworked that part in v6. > > cons->out_prod = 0; > > without also doing: > > cons->out_cons = 0; > > Also, if ns16x50_fifo_tx_flush is the only caller of > ns16x50_fifo_tx_reset, I would open code the implementation directly > inside ns16x50_fifo_tx_flush to make it more obvious. The will be another callsite for ns16x50_fifo_tx_reset() - FCR (in v6). > > > > > +} > > > + > > > +/* > > > + * Flush cached output to Xen console. > > > + */ > > > +static void ns16x50_fifo_tx_flush(struct vuart_ns16x50 *vdev) > > > +{ > > > + struct xencons_interface *cons = &vdev->cons; > > > + struct domain *d = vdev->owner; > > > + > > > + if ( ns16x50_fifo_tx_empty(vdev) ) > > > + return; > > > + > > > + UART_IIR_THR ASSERT(cons->out_prod < ARRAY_SIZE(cons->out)); > > > + cons->out[cons->out_prod] = '\0'; > > > > should use MASK_XENCONS_IDX to access the array > > > > > > > + cons->out_prod++; > > > + > > > + guest_printk(d, guest_prefix "%s", cons->out); > > > + > > > + ns16x50_fifo_tx_reset(vdev); > > > > set UART_IIR_THR and call ns16x50_irq_check ? > > > > > > > +} >