Re: [PATCH v7 07/16] emul/ns16x50: implement LCR/LSR registers

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the patch.

On Tue, Sep 9, 2025 at 1:16 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Add LCR/LSR registers implementation to the I/O port handler.
>
> Add implementation of ns16x50_dlab_get() and ns16x50_iir_check_lsi().
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - n/a
> ---
>  xen/common/emul/vuart/ns16x50.c | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 664d799ddaee..0831a576cd9e 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -87,12 +87,12 @@ struct vuart_ns16x50 {
>
>  static uint8_t ns16x50_dlab_get(const struct vuart_ns16x50 *vdev)
>  {
> -    return 0;
> +    return vdev->regs[UART_LCR] & UART_LCR_DLAB ? 1 : 0;
>  }
>
>  static bool cf_check ns16x50_iir_check_lsi(const struct vuart_ns16x50 *vdev)
>  {
> -    return false;
> +    return vdev->regs[UART_LSR] & UART_LSR_MASK;
>  }
>
>  static bool cf_check ns16x50_iir_check_rda(const struct vuart_ns16x50 *vdev)
> @@ -228,11 +228,16 @@ static int ns16x50_io_write8(
>              regs[UART_IER] = val & UART_IER_MASK;
>              break;
>
> +        case UART_LCR:
> +            regs[UART_LCR] = val;

I understand that this register is mostly used to control the hardware
behavior, but I think we should pay attention to Bit 7 in this register:

    Bit 7: Divisor Latch Access Bit (DLAB). It must be set high (logic 1)
    to access the Divisor Latches of the Baud Generator during a read or
    write operation. It must be set low (logic 0) to access the Receiver
    Buffer, the Transmitter Holding Register, or the Interrupt Enable
    Register.

I know that you are checking it before accessing DLAB registers, but what
about accesses to RBR, THR, and IER?

> +            break;
> +
>          /* NB: Firmware (e.g. OVMF) may rely on SCR presence. */
>          case UART_SCR:
>              regs[UART_SCR] = val;
>              break;
>
> +        case UART_LSR: /* RO */

Is there a reason we cannot simply leave this unchanged?

>          default:
>              rc = -EINVAL;
>              break;
> @@ -314,6 +319,15 @@ static int ns16x50_io_read8(
>              val = ns16x50_iir_get(vdev);
>              break;
>
> +        case UART_LCR:
> +            val = regs[UART_LCR];
> +            break;
> +
> +        case UART_LSR:
> +            val = regs[UART_LSR] | UART_LSR_THRE | UART_LSR_TEMT;

Why are UART_LSR_THRE and UART_LSR_TEMT set unconditionally?
What about UART_LSR_DR?

> +            regs[UART_LSR] = val & ~UART_LSR_MASK;

Maybe it would be good to mention why we are not resetting
UART_LSR_PE and UART_LSR_FE on read.

> +            break;
> +
>          case UART_SCR:
>              val = regs[UART_SCR];
>              break;
> --
> 2.51.0
>
>

Best regards,
Mykola