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

[Stefano Stabellini <stefano.stabellini@xxxxxxx>]

On Fri, 5 Sep 2025, dmukhin@xxxxxxx wrote:
> On Fri, Aug 29, 2025 at 03:21:13PM -0700, Stefano Stabellini wrote:
> > On Thu, 28 Aug 2025, dmukhin@xxxxxxx wrote:
> > > From: Denis Mukhin <dmukhin@xxxxxxxx> 
> > > 
> > > 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.

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