Re: [PATCH v7 10/16] emul/ns16x50: implement THR register

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the patch.

On Tue, Sep 9, 2025 at 1:26 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Add THR register emulation to the I/O port handlder.
>
> Add TX FIFO management code since THR depends on TX FIFO.
>
> TX FIFOs is 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).
>
> TX FIFOs is emulated by using xencons_interface which conveniently provides
> primitives for buffer management and later can be used for inter-domain
> communication similarly to vpl011.
>
> Account for DLL == 0: in this case, disable transmitter.
>
> Add UART_IIR_THR interrupt reason handling since it depends on THR register
> access.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - added DLL == 0 case handling as per Mykola's suggestion
> - dropped UART_IIR_THR clearing in UART_IIR register emulation in 
> ns16x50_io_write8()
> - simplified UART_IIR_THR handling
> - updated ns16x50_iir_check_thr()
> ---
>  xen/common/emul/vuart/ns16x50.c | 82 ++++++++++++++++++++++++++++++++-
>  1 file changed, 81 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 250411e0a7d8..137ce08f4e1d 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -149,6 +149,66 @@ static int ns16x50_fifo_rx_putchar(struct vuart_ns16x50 
> *vdev, char c)
>      return rc;
>  }
>
> +static bool ns16x50_fifo_tx_full(const struct vuart_ns16x50 *vdev)
> +{
> +    const struct xencons_interface *cons = &vdev->cons;
> +
> +    return cons->out_prod - cons->out_cons == ARRAY_SIZE(cons->out);
> +}
> +
> +static void ns16x50_fifo_tx_reset(struct vuart_ns16x50 *vdev)
> +{
> +    struct xencons_interface *cons = &vdev->cons;
> +
> +    cons->out_cons = cons->out_prod;
> +}
> +
> +/*
> + * 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;
> +    XENCONS_RING_IDX i, n, len = cons->out_prod - cons->out_cons;
> +
> +    ASSERT(len <= ARRAY_SIZE(cons->out));
> +    if ( !len )
> +        return;
> +
> +    i = MASK_XENCONS_IDX(cons->out_cons, cons->out);
> +    n = min_t(XENCONS_RING_IDX, len, ARRAY_SIZE(cons->out) - i);
> +    if ( n )
> +        guest_printk(d, guest_prefix "%.*s", n, &cons->out[i]);
> +
> +    i = 0;
> +    n = len - n;
> +    if ( n )
> +        guest_printk(d, guest_prefix "%.*s", n, &cons->out[i]);

ns16x50_fifo_tx_flush() splits wrapped output into two guest_printk()
calls, so the log gets two prefixes for a single line:
    (d1) PART1(d1) PART2

Could we linearize the wrapped buffer and emit a single guest_printk()
(e.g. by printing both spans in one format string) to keep just one prefix?


Best regards,
Mykola


> +
> +    cons->out_cons += len;
> +}
> +
> +/*
> + * 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 ( !ns16x50_fifo_tx_full(vdev) )
> +    {
> +        cons->out[MASK_XENCONS_IDX(cons->out_prod, cons->out)] = ch;
> +        cons->out_prod++;
> +    }
> +
> +    if ( ch == '\n' || ch == '\0' || ns16x50_fifo_tx_full(vdev) )
> +        ns16x50_fifo_tx_flush(vdev);
> +}
> +
>  static bool ns16x50_is_running(const struct vuart_ns16x50 *vdev)
>  {
>      /* DLL set to 0 disables serial communication. */
> @@ -172,7 +232,7 @@ static bool cf_check ns16x50_iir_check_rda(const struct 
> vuart_ns16x50 *vdev)
>
>  static bool cf_check ns16x50_iir_check_thr(const struct vuart_ns16x50 *vdev)
>  {
> -    return false;
> +    return !ns16x50_fifo_tx_full(vdev);
>  }
>
>  static bool cf_check ns16x50_iir_check_msi(const struct vuart_ns16x50 *vdev)
> @@ -294,6 +354,22 @@ static int ns16x50_io_write8(
>      {
>          switch ( reg )
>          {
> +        case UART_THR:
> +            if ( !ns16x50_is_running(vdev) )
> +                break;
> +
> +            if ( regs[UART_MCR] & UART_MCR_LOOP )
> +            {
> +                if ( ns16x50_fifo_rx_putchar(vdev, val) )
> +                    regs[UART_LSR] |= UART_LSR_OE;
> +
> +                regs[UART_LSR] |= UART_LSR_DR;
> +            }
> +            else
> +                ns16x50_fifo_tx_putchar(vdev, val);
> +
> +            break;
> +
>          case UART_IER:
>              regs[UART_IER] = val & UART_IER_MASK;
>              break;
> @@ -646,6 +722,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
>
>