Re: [PATCH v7 08/16] emul/ns16x50: implement MCR/MSR registers

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Denis,

Thank you for the patch.

On Tue, Sep 9, 2025 at 12:12 AM <dmukhin@xxxxxxx> wrote:
>
> From: Denis Mukhin <dmukhin@xxxxxxxx>
>
> Add MCR/MSR registers emulation to the I/O port handler.
>
> Add implementation of ns16x50_iir_check_msi().
>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - fixed UART_MSR_TERI handling
> ---
>  xen/common/emul/vuart/ns16x50.c | 62 ++++++++++++++++++++++++++++++++-
>  1 file changed, 61 insertions(+), 1 deletion(-)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 0831a576cd9e..fdc20124d4c9 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -107,7 +107,7 @@ static bool cf_check ns16x50_iir_check_thr(const struct 
> vuart_ns16x50 *vdev)
>
>  static bool cf_check ns16x50_iir_check_msi(const struct vuart_ns16x50 *vdev)
>  {
> -    return false;
> +    return vdev->regs[UART_MSR] & UART_MSR_CHANGE;
>  }
>
>  /*
> @@ -232,12 +232,63 @@ static int ns16x50_io_write8(
>              regs[UART_LCR] = val;
>              break;
>
> +        case UART_MCR: {

Probably the opening brace should be moved to the next line.
See CODING_STYLE:

Braces ('{' and '}') are usually placed on a line of their own, except
for:

- the do/while loop
- the opening brace in definitions of enum, struct, and union
- the opening brace in initializers
- compound literals

> +            uint8_t msr_curr, msr_next, msr_delta;
> +
> +            msr_curr = regs[UART_MSR];
> +            msr_next = 0;
> +            msr_delta = 0;
> +
> +            if ( val & UART_MCR_RSRVD0 )
> +                ns16x50_warn(vdev, "MCR: attempt to set reserved bit: %x\n",
> +                             UART_MCR_RSRVD0);
> +
> +            if ( val & UART_MCR_TCRTLR )
> +                ns16x50_warn(vdev, "MCR: not supported: %x\n",
> +                             UART_MCR_TCRTLR);
> +
> +            if ( val & UART_MCR_RSRVD1 )
> +                ns16x50_warn(vdev, "MCR: attempt to set reserved bit: %x\n",
> +                             UART_MCR_RSRVD1);
> +
> +            /* Set modem status */
> +            if ( val & UART_MCR_LOOP )
> +            {
> +                if ( val & UART_MCR_DTR )
> +                    msr_next |= UART_MSR_DSR;
> +                if ( val & UART_MCR_RTS )
> +                    msr_next |= UART_MSR_CTS;
> +                if ( val & UART_MCR_OUT1 )
> +                    msr_next |= UART_MSR_RI;
> +                if ( val & UART_MCR_OUT2 )
> +                    msr_next |= UART_MSR_DCD;
> +            }
> +            else
> +                msr_next |= UART_MSR_DCD | UART_MSR_DSR | UART_MSR_CTS;
> +
> +            /* 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_MSR_DSR) ^ (msr_next & UART_MSR_DSR) )
> +                msr_delta |= UART_MSR_DDSR;
> +            if ( !(msr_curr & UART_MSR_RI) && (msr_next & UART_MSR_RI) )
> +                msr_delta |= UART_MSR_TERI;
> +            if ( (msr_curr & UART_MSR_DCD) ^ (msr_next & UART_MSR_DCD) )
> +                msr_delta |= UART_MSR_DDCD;

It looks like this could be simplified to something like:
    msr_delta = ((regs[UART_MSR] ^ msr_next) >> 4);

> +
> +            regs[UART_MCR] = val & UART_MCR_MASK;
> +            regs[UART_MSR] = msr_next | msr_delta;

Does this description for the first four bits of MSR:
    "... input to the chip has changed state since the last time it was
    read by the CPU"

mean that we shouldn't modify bits that are already set but have not yet
been read by the CPU?

> +
> +            break;
> +        }
> +
>          /* NB: Firmware (e.g. OVMF) may rely on SCR presence. */
>          case UART_SCR:
>              regs[UART_SCR] = val;
>              break;
>
>          case UART_LSR: /* RO */
> +        case UART_MSR: /* RO */
>          default:
>              rc = -EINVAL;
>              break;
> @@ -323,11 +374,20 @@ static int ns16x50_io_read8(
>              val = regs[UART_LCR];
>              break;
>
> +        case UART_MCR:
> +            val = regs[UART_MCR];
> +            break;
> +
>          case UART_LSR:
>              val = regs[UART_LSR] | UART_LSR_THRE | UART_LSR_TEMT;
>              regs[UART_LSR] = val & ~UART_LSR_MASK;
>              break;
>
> +        case UART_MSR:
> +            val = regs[UART_MSR];
> +            regs[UART_MSR] &= ~UART_MSR_CHANGE;
> +            break;
> +
>          case UART_SCR:
>              val = regs[UART_SCR];
>              break;
> --
> 2.51.0
>
>

Best regards,
Mykola