On Fri, Aug 29, 2025 at 01:28:41PM -0700, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, dmuk...@xen.org wrote:
> > From: Denis Mukhin <dmuk...@ford.com>
> >
> > 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 <dmuk...@ford.com>
> > ---
> > 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.
Ack.
>
>
> > +}
> > +
> > +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 used TX buffer not as a FIFO but as a large buffer for simplicity...
I have reworked that into a normal circular buffer 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
Ack.
>
>
> > + 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 ?
I moved that to the I/O handler calling ns16x50_fifo_tx_flush() instead,
because ns16x50_fifo_tx_flush() can be called during vuart destroy,
and there's no need to emulate IRQs at domain destroy.
>
>
> > +}
> > +
> > +/*
> > + * Accumulate guest OS output before sending to Xen console.
> > + */
> > +static void ns16x50_fifo_tx_putchar(struct vuart_ns16x50 *vdev, char ch)
> > +{
> > + struct xencons_interface *cons = &vdev->cons;
> > +
> > + if ( !is_console_printable(ch) )
> > + return;
> > +
> > + if ( ch != '\0' )
> > + {
> > + cons->out[cons->out_prod] = ch;
>
> should use MASK_XENCONS_IDX to access the array
Ack.
>
>
> > + cons->out_prod++;
> > + }
> > +
> > + if ( cons->out_prod == ARRAY_SIZE(cons->out) - 1 ||
> > + ch == '\n' || ch == '\0' )
> > + ns16x50_fifo_tx_flush(vdev);
> > +}
> > +
> > static inline uint8_t cf_check ns16x50_dlab_get(const struct vuart_ns16x50
> > *vdev)
> > {
> > return vdev->regs[UART_LCR] & UART_LCR_DLAB ? 1 : 0;
> > @@ -228,6 +341,16 @@ static int ns16x50_io_write8(
> > {
> > switch ( reg )
> > {
> > + case UART_THR:
> > + if ( regs[UART_MCR] & UART_MCR_LOOP )
> > + rc = ns16x50_fifo_rx_putchar(vdev, val);
> > + else
> > + ns16x50_fifo_tx_putchar(vdev, val);
>
> should UART_IIR_THR be cleared here?
Yes, thanks for the catch!
>
>
> > + rc = 0;
> > +
> > + break;
> > +
> > case UART_IER:
> > /*
> > * NB: Make sure THR interrupt is re-triggered once guest OS
> > @@ -312,6 +435,14 @@ static int ns16x50_io_read8(
> > else {
> > switch ( reg )
> > {
> > + case UART_RBR:
> > + rc = ns16x50_fifo_rx_getchar(vdev);
> > + if ( rc >= 0 )
> > + val = (uint8_t)rc;
>
> Empty fifo check?
Will do.
>
>
> > + rc = 0;
> > + break;
> > +
> > case UART_IER:
> > val = regs[UART_IER];
> > break;
> > @@ -499,6 +630,10 @@ static void cf_check ns16x50_deinit(void *arg)
> > struct vuart_ns16x50 *vdev = arg;
> >
> > ASSERT(vdev);
> > +
> > + spin_lock(&vdev->lock);
> > + ns16x50_fifo_tx_flush(vdev);
> > + spin_unlock(&vdev->lock);
> > }
> >
> > static void * cf_check ns16x50_alloc(struct domain *d, const struct
> > vuart_info *info)
> > --
> > 2.51.0
> >
