Re: [PATCH v5 06/15] emul/ns16x50: implement THR/RBR registers

[dmukhin@xxxxxxx]

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@xxxxxxx wrote:
> > > From: Denis Mukhin <dmukhin@xxxxxxxx> 
> > > 
> > > 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 <dmukhin@xxxxxxxx>
> > > ---
> > > 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 ?
> > 
> > 
> > > +}
>