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

[dmukhin@xxxxxxxx]

Hi Mykola,

Thanks for detailed review for v7 series, much appreciated!

Sorry, it's been a while for this and other series...
I am picking that up.

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

Good idea, will do.

--
Denis