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

[dmukhin@xxxxxxx]

On Sun, Sep 07, 2025 at 12:24:24AM +0300, Mykola Kvach wrote:
> 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.

I agree, it makes sense to move the switch() block with default register
handling to the previous DLL/DLM commit; will update.

Thanks!