Re: [PATCH v5 05/15] emul/ns16x50: implement EIR/IIR registers

[dmukhin@xen.org]

On Fri, Aug 29, 2025 at 01:14:03PM -0700, Stefano Stabellini wrote:
> On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
> > From: Denis Mukhin <dmukhin@ford.com> 
> > 
> > Add interrupt enable register emulation (EIR) and interrupt identity reason
> 
> EIR->IER

Whooops, thanks.

> 
> > (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.
> > 
> > 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@ford.com>
> > ---
> > Changes since v4:
> > - new patch
> > ---
> >  xen/common/emul/vuart/ns16x50.c | 177 +++++++++++++++++++++++++++++++-
> >  1 file changed, 176 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/common/emul/vuart/ns16x50.c b/xen/common/emul/vuart/ns16x50.c
> > index f9f307a4ad24..20597cc36b35 100644
> > --- a/xen/common/emul/vuart/ns16x50.c
> > +++ b/xen/common/emul/vuart/ns16x50.c
> > @@ -85,9 +85,131 @@ struct vuart_ns16x50 {
> >      spinlock_t lock;                    /* Protection */
> >  };
> >  
> > +static bool ns16x50_fifo_rx_empty(const struct vuart_ns16x50 *vdev)
> > +{
> > +    const struct xencons_interface *cons = &vdev->cons;
> > +
> > +    return cons->in_prod == cons->in_cons;
> > +}
> 
> there is no ring so far so I would not add ns16x50_fifo_rx_empty for now

Ack.

> 
> 
> >  static inline uint8_t cf_check 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 vdev->regs[UART_LSR] & UART_LSR_MASK;
> > +}
> > +
> > +static bool cf_check ns16x50_iir_check_rda(const struct vuart_ns16x50 *vdev)
> > +{
> > +    return !ns16x50_fifo_rx_empty(vdev);
> > +}
> > +
> > +static bool cf_check ns16x50_iir_check_thr(const struct vuart_ns16x50 *vdev)
> > +{
> > +    return vdev->regs[NS16X50_REGS_NUM + UART_IIR] & UART_IIR_THR;
> > +}
> > +
> > +static bool cf_check ns16x50_iir_check_msi(const struct vuart_ns16x50 *vdev)
> > +{
> > +    return vdev->regs[UART_MSR] & UART_MSR_CHANGE;
> > +}
> > +
> > +/*
> > + * Get the interrupt identity reason.
> > + *
> > + * IIR is re-calculated once called, because ns16x50 always reports high
> > + * priority events first.
> > + * regs[NS16X50_REGS_NUM + UART_IIR] is used to store THR reason only.
> > + */
> > +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 },
> > +        [3] = { ns16x50_iir_check_msi, UART_IER_EMSI,   UART_IIR_MSI },
> > +    };
> > +    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;
> > +
> > +    }
> > +    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) ) /* HVM */
> > +        vector = hvm_isa_irq_assert(d, info->irq, vioapic_get_vector);
> > +    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 */
> > +        hvm_isa_irq_deassert(d, info->irq);
> > +    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_assert(vdev);
> 
> It is a bit strange that if "NOINT" is set, we raise the interrupt

Yes, that is wrong.
Thank you!

> 
> 
> > +    else
> > +        ns16x50_irq_deassert(vdev);
> > +
> > +    ns16x50_debug(vdev, "IRQ#%d IIR 0x%02x %s\n", info->irq, iir,
> > +                  (iir & UART_IIR_NOINT) ? "deassert" : "assert");
> >  }
> >  
> >  /*
> > @@ -102,6 +224,29 @@ 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 )
> > +        {
> > +        case UART_IER:
> > +            /*
> > +             * NB: Make sure THR interrupt is re-triggered once guest OS
> > +             * re-enabled ETHREI in EIR.
> 
> EIR->IER

Will fix.

> 
> 
> > +             */
> > +            if ( val & regs[UART_IER] & UART_IER_ETHREI )
> > +                regs[NS16X50_REGS_NUM + UART_IIR] |= UART_IIR_THR;
> 
> I am confused by this. Shouldn't it be :
> 
> if ( (val & UART_IER_ETHREI) && !(regs[UART_IER] & UART_IER_ETHREI) )
> 
> Meaning set UART_IIR_THR if ETHREI goes 0->1 ?

That is by design to re-toggle the UART_IIR_THR since there's no baud
rate emulation.

> 
> 
> > +            regs[UART_IER] = val & UART_IER_MASK;
> > +
> > +            break;
> > +
> > +        default:
> > +            rc = -EINVAL;
> > +            break;
> > +        }
> > +
> > +        ns16x50_irq_check(vdev);
> > +    }
> >  
> >      return rc;
> >  }
> > @@ -164,6 +309,29 @@ 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_IER:
> > +            val = regs[UART_IER];
> > +            break;
> > +
> > +        case UART_IIR: /* RO */
> > +            val = ns16x50_iir_get(vdev);
> > +
> > +            /* NB: clear IIR scratch location */
> > +            if ( val & UART_IIR_THR )
> > +                regs[NS16X50_REGS_NUM + UART_IIR] &= ~UART_IIR_THR;
> 
> Maybe add an in-code comment why it is a good idea to clear THR here

Will fix.

> 
> 
> > +
> > +            break;
> > +
> > +        default:
> > +            rc = -EINVAL;
> > +            break;
> > +        }
> > +
> > +        ns16x50_irq_check(vdev);
> > +    }
> >  
> >      *data = val;
> >  
> > @@ -314,8 +482,15 @@ static int ns16x50_init(void *arg)
> >      vdev->regs[NS16X50_REGS_NUM + UART_DLL] = divisor & 0xff;
> >      vdev->regs[NS16X50_REGS_NUM + UART_DLM] = (divisor >> 8) & 0xff;
> >  
> > +    /* ns16x50 shall assert UART_IIR_THR whenever transmitter is empty. */
> > +    vdev->regs[NS16X50_REGS_NUM + UART_IIR] = UART_IIR_THR;
> > +
> >      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
> >