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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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


> +}
> +
> +/*
> + * 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 ?


> +}
> +
> +/*
> + * 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


> +        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?


> +            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?


> +            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
>