Re: [PATCH v7 06/16] emul/ns16x50: implement IER/IIR 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 interrupt enable register emulation (IER) and interrupt identity reason
> (IIR) register emulation to the I/O port handler.
>
> Also add routines for asserting/deasserting the virtual ns16x50 interrupt
> line as a dependent on IIR code. vPIC case is implemented (HVM), vIOAPIC
> case is stubbed out (for follow on PVH).
>
> Poke ns16x50_irq_check() on every I/O register access because the emulator
> does not have clock emulation anyway (e.g. for baud rate emulation)>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>
> ---
> Changes since v6:
> - removed asserts for !has_vpic() paths
> ---
>  xen/common/emul/vuart/ns16x50.c | 138 ++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
>
> diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> index 5643ef4cc01e..664d799ddaee 100644
> --- a/xen/common/emul/vuart/ns16x50.c
> +++ b/xen/common/emul/vuart/ns16x50.c
> @@ -90,6 +90,124 @@ static uint8_t ns16x50_dlab_get(const struct 
> vuart_ns16x50 *vdev)
>      return 0;
>  }
>
> +static bool cf_check ns16x50_iir_check_lsi(const struct vuart_ns16x50 *vdev)
> +{
> +    return false;
> +}
> +
> +static bool cf_check ns16x50_iir_check_rda(const struct vuart_ns16x50 *vdev)
> +{
> +    return false;
> +}
> +
> +static bool cf_check ns16x50_iir_check_thr(const struct vuart_ns16x50 *vdev)
> +{
> +    return false;
> +}
> +
> +static bool cf_check ns16x50_iir_check_msi(const struct vuart_ns16x50 *vdev)
> +{
> +    return false;
> +}
> +
> +/*
> + * Get the interrupt identity reason.
> + *
> + * IIR is re-calculated once called, because ns16x50 always reports high
> + * priority events first.
> + */
> +static uint8_t ns16x50_iir_get(const struct vuart_ns16x50 *vdev)
> +{
> +    /*
> +     * Interrupt identity reasons by priority.
> +     * NB: high priority are at lower indexes below.
> +     */
> +    static const struct {
> +        bool (*check)(const struct vuart_ns16x50 *vdev);
> +        uint8_t ier;
> +        uint8_t iir;
> +    } iir_by_prio[] = {
> +        [0] = { ns16x50_iir_check_lsi, UART_IER_ELSI,   UART_IIR_LSI },
> +        [1] = { ns16x50_iir_check_rda, UART_IER_ERDAI,  UART_IIR_RDA },
> +        [2] = { ns16x50_iir_check_thr, UART_IER_ETHREI, UART_IIR_THR },

According to the spec (PC16550D and others also state this), if the
source of the IRQ is Transmitter Holding Register Empty, then reading
from IIR clears this interrupt.

So, I think it is not safe to call ns16x50_irq_check for every I/O.
According to the documentation, reset conditions for interrupts are:

    Reads:
        UART_RBR
        UART_IIR (only for the above case)
        UART_LSR
        UART_MSR

    Writes:
        UART_THR
        UART_IER

Of course, it is also necessary to think about how to handle the setting
of bits in IIR properly.

> +        [3] = { ns16x50_iir_check_msi, UART_IER_EMSI,   UART_IIR_MSI },

Are you going to implement support for the Timeout Interrupt bit in
FIFO mode (PC16550D)?

    Bit 3: In the 16450 Mode this bit is 0. In the FIFO mode this
    bit is set along with bit 2 when a timeout interrupt is pending.
?

> +    };
> +    const uint8_t *regs = vdev->regs;
> +    uint8_t iir = 0;
> +    unsigned int i;
> +
> +    /*
> +     * NB: every interaction w/ ns16x50 registers (except DLAB=1) goes
> +     * through that call.
> +     */
> +    ASSERT(spin_is_locked(&vdev->lock));
> +
> +    for ( i = 0; i < ARRAY_SIZE(iir_by_prio); i++ )
> +    {
> +        if ( (regs[UART_IER] & iir_by_prio[i].ier) &&
> +             iir_by_prio[i].check(vdev) )
> +            break;
> +

Do we need this extra line?

> +    }
> +    if ( i == ARRAY_SIZE(iir_by_prio) )
> +        iir |= UART_IIR_NOINT;
> +    else
> +        iir |= iir_by_prio[i].iir;
> +
> +    if ( regs[UART_FCR] & UART_FCR_ENABLE )
> +        iir |= UART_IIR_FE;
> +
> +    return iir;
> +}
> +
> +static void ns16x50_irq_assert(const struct vuart_ns16x50 *vdev)
> +{
> +    struct domain *d = vdev->owner;
> +    const struct vuart_info *info = vdev->info;
> +    int vector;
> +
> +    if ( has_vpic(d) )
> +        vector = hvm_isa_irq_assert(d, info->irq, vioapic_get_vector);
> +    else if ( has_vioapic(d) )
> +        /* TODO */
> +    else
> +        ASSERT_UNREACHABLE();
> +
> +    ns16x50_debug(vdev, "IRQ#%d vector %d assert\n", info->irq, vector);
> +}
> +
> +static void ns16x50_irq_deassert(const struct vuart_ns16x50 *vdev)
> +{
> +    struct domain *d = vdev->owner;
> +    const struct vuart_info *info = vdev->info;
> +
> +    if ( has_vpic(d) )
> +        hvm_isa_irq_deassert(d, info->irq);
> +    else if ( has_vioapic(d) )
> +        /* TODO */
> +    else
> +        ASSERT_UNREACHABLE();
> +
> +    ns16x50_debug(vdev, "IRQ#%d deassert\n", info->irq);
> +}
> +
> +/*
> + * Assert/deassert virtual ns16x50 interrupt line.
> + */
> +static void ns16x50_irq_check(const struct vuart_ns16x50 *vdev)
> +{
> +    uint8_t iir = ns16x50_iir_get(vdev);
> +    const struct vuart_info *info = vdev->info;
> +
> +    if ( iir & UART_IIR_NOINT )
> +        ns16x50_irq_deassert(vdev);
> +    else
> +        ns16x50_irq_assert(vdev);
> +
> +    ns16x50_debug(vdev, "IRQ#%d IIR 0x%02x %s\n", info->irq, iir,
> +                  (iir & UART_IIR_NOINT) ? "deassert" : "assert");
> +}
> +
>  /*
>   * Emulate 8-bit write access to ns16x50 register.
>   */
> @@ -106,6 +224,10 @@ static int ns16x50_io_write8(
>      {
>          switch ( reg )
>          {
> +        case UART_IER:
> +            regs[UART_IER] = val & UART_IER_MASK;
> +            break;
> +
>          /* NB: Firmware (e.g. OVMF) may rely on SCR presence. */
>          case UART_SCR:
>              regs[UART_SCR] = val;
> @@ -115,6 +237,8 @@ static int ns16x50_io_write8(
>              rc = -EINVAL;
>              break;
>          }
> +
> +        ns16x50_irq_check(vdev);
>      }
>
>      return rc;
> @@ -182,6 +306,14 @@ static int ns16x50_io_read8(
>      {
>          switch ( reg )
>          {
> +        case UART_IER:
> +            val = regs[UART_IER];
> +            break;
> +
> +        case UART_IIR: /* RO */
> +            val = ns16x50_iir_get(vdev);
> +            break;
> +
>          case UART_SCR:
>              val = regs[UART_SCR];
>              break;
> @@ -190,6 +322,8 @@ static int ns16x50_io_read8(
>              rc = -EINVAL;
>              break;
>          }
> +
> +        ns16x50_irq_check(vdev);
>      }
>
>      *data = val;
> @@ -342,6 +476,10 @@ static int ns16x50_init(void *arg)
>
>      register_portio_handler(d, info->base_addr, info->size, 
> ns16x50_io_handle);
>
> +    spin_lock(&vdev->lock);
> +    ns16x50_irq_check(vdev);
> +    spin_unlock(&vdev->lock);
> +
>      return 0;
>  }
>
> --
> 2.51.0
>
>

Best regards,
Mykola