Re: [PATCH v6 05/15] emul/ns16x50: implement SCR register

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

On Sat, Sep 6, 2025 at 2:27 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Add SCR register emulation to the I/O port handler.
> Firmware (e.g. OVMF) may use SCR during the guest OS boot.
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v5:
> - moved earlier in the series to simplify I/O handler population in
>   the follow on patches
> - Link to v5: 
> https://lore.kernel.org/xen-devel/20250828235409.2835815-11-dmukhin@xxxxxxxx/
> ---
>  xen/common/emul/vuart/ns16x50.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 7f479a5be4a2..51ec85e57627 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -103,6 +103,20 @@ static int ns16x50_io_write8(
>
>      if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
>          regs[NS16X50_REGS_NUM + reg] = val;
> +    else
> +    {
> +        switch ( reg )
> +        {
> +        /* NB: Firmware (e.g. OVMF) may rely on SCR presence. */
> +        case UART_SCR:
> +            regs[UART_SCR] = val;
> +            break;
> +
> +        default:
> +            rc = -EINVAL;

In the previous commit, when ns16x50_dlab_get() was zero and UART_DLL
or UART_DLM was accessed, the function returned 0. With this commit,
the behavior changes: now an -EINVAL error is returned for both DLL
and DLM when ns16x50_dlab_get() is zero.

Should this be fixed in the previous commit, or is this change
intentional in this one? Note that for 16-bit accesses you already
return an error when ns16x50_dlab_get() is zero, so the behavior is
inconsistent for 8-bit accesses to DLL/DLM.

> +            break;
> +        }
> +    }
>
>      return rc;
>  }
> @@ -165,6 +179,19 @@ static int ns16x50_io_read8(
>
>      if ( ns16x50_dlab_get(vdev) && (reg == UART_DLL || reg == UART_DLM) )
>          val = regs[NS16X50_REGS_NUM + reg];
> +    else
> +    {
> +        switch ( reg )
> +        {
> +        case UART_SCR:
> +            val = regs[UART_SCR];
> +            break;
> +
> +        default:
> +            rc = -EINVAL;
> +            break;
> +        }
> +    }
>
>      *data = val;
>
> --
> 2.51.0
>
>

Best regards,
Mykola