Re: [PATCH v5 15/15] emul/ns16x50: implement IRQ emulation via vIOAPIC

[dmukhin@xen.org]

On Fri, Sep 05, 2025 at 04:01:15PM -0700, Stefano Stabellini wrote:
> On Fri, 5 Sep 2025, dmukhin@xen.org wrote:
> > On Fri, Aug 29, 2025 at 03:21:13PM -0700, Stefano Stabellini wrote:
> > > On Thu, 28 Aug 2025, dmukhin@xen.org wrote:
> > > > From: Denis Mukhin <dmukhin@ford.com> 
> > > > 
> > > > PVH domains use vIOAPIC, not vPIC and NS16550 emulates ISA IRQs which cannot
> > > > be asserted on vIOAPIC.
> > > 
> > > One option is to enable the vPIT for PVH domains when the NS16550
> > > emulator is enabled. Would that resolve the problem? That would be a
> > > simpler solution compared to adding IRQ_EMU because the IRQ_* logic is
> > > already quite complex.
> > 
> > vPIC? (PIT is timer).
> > 
> > I tried to enable vPIC for hwdom, but there's some more work to make it work
> > :-/
> > 
> > > 
> > > Alternatively...
> > [..]
> > > 
> > > > --- a/xen/arch/x86/hvm/vioapic.c
> > > > +++ b/xen/arch/x86/hvm/vioapic.c
> > > > @@ -177,6 +177,16 @@ static int vioapic_hwdom_map_gsi(unsigned int gsi, unsigned int trig,
> > > >  
> > > >      ASSERT(is_hardware_domain(currd));
> > > >  
> > > > +    /*
> > > > +     * Interrupt is claimed by one of the platform virtual devices (e.g.
> > > > +     * NS16550); do nothing.
> > > > +     */
> > > > +    write_lock(&currd->event_lock);
> > > > +    ret = domain_irq_to_emuirq(currd, gsi);
> > > > +    write_unlock(&currd->event_lock);
> > > > +    if ( ret != IRQ_UNBOUND )
> > > > +        return 0;
> > > 
> > > ..alternatively, we could have an add-hoc check here? Not very nice but
> > > at least it would be very simple.
> > > 
> > > In other words, adding vPIT is preferable in my opinion but as a second
> > > option I would consider an ad-hoc check. I would try to avoid adding
> > > IRQ_EMU -- that should be the last resort in my view.
> > 
> > In my opinion, IRQ_EMU falls into category of an ad-hoc.
> > 
> > I think vioapic_hwdom_map_gsi() should not depend on the presence of certain
> > virtual devices but rely on some global flag, e.g. via quering
> > domain_irq_to_emuirq() infra.
> 
> Hi Denis, the reason why IRQ_EMU is dangerous is that there are a bunch
> of checks emuirq != IRQ_PT and also emuirq != IRQ_MSI_EMU which I don't
> know if they would still behave correctly after the introduction of
> IRQ_EMU.

!= IRQ_PT checks (arch/x86/irq.c) are done from map_domain_emuirq_pirq()
and unmap_domain_pirq_emuirq() which I use for IRQ_EMU case.

Those checks are needed to correctly allocate an "ad-hoc flag" in a radix
"IRQ_EMU" tree, i.e. IRQ_EMU case is handled correctly.

!= IRQ_MSI_EMU check (there's only one in arch/x86/hvm/irq.c) is done from
hvm_inject_msi(), it guarantees that IRQ_EMU case is handled correctly.

> 
> I would prefer to avoid the problem if possible...