Re: [PATCH v4 5/8] emul/vuart-ns16550: introduce NS16550-compatible UART emulator (x86)

[dmkhn@xxxxxxxxx]

On Thu, Jul 31, 2025 at 04:57:00PM -0700, Stefano Stabellini wrote:
> On Thu, 31 Jul 2025, dmkhn@xxxxxxxxx wrote:

[..]

> > +/*
> > + * Emulate 8-bit write access to NS16550 register.
> > + */
> > +static int ns16550_io_write8(
> > +    struct vuart_ns16550 *vdev, uint32_t reg, uint8_t *data)
> > +{
> > +    uint8_t *regs = vdev->regs;
> > +    uint8_t val = *data;
> > +    int rc = 0;
> > +
> > +    if ( ns16550_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
> > +        regs[NS16550_REGS_NUM + reg] = val;
> > +    else
> > +    {
> > +        switch ( reg )
> > +        {
> > +        case UART_THR:
> > +            if ( regs[UART_MCR] & UART_MCR_LOOP )
> > +            {
> > +                (void)ns16550_fifo_rx_putchar(vdev, val);
> > +                regs[UART_LSR] |= UART_LSR_OE;
> 
> Why is UART_LSR_OE set unconditionally here instead of checking if
> ns16550_fifo_rx_putchar returned -ENOSPC?

Thanks!
Yes, overrun condition should be emulated once FIFO is full.

[..]

> > +
> > +            /* Calculate changes in modem status */
> > +            if ( (msr_curr & UART_MSR_CTS) ^ (msr_next & UART_MSR_CTS) )
> > +                msr_delta |= UART_MSR_DCTS;
> > +            if ( (msr_curr & UART_MCR_RTS) ^ (msr_next & UART_MCR_RTS) )
> > +                msr_delta |= UART_MSR_DDSR;
> 
> Should we check UART_MSR_DSR instead of UART_MCR_RTS to set
> UART_MSR_DDSR ?

Good catch!
Thanks.

[..]