Re: [PATCH v2 30/35] x86/hvm: add helpers for raising guest IRQs

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 8:18 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:42:00PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > Added convenience wrappers for asserting/de-asserting interrupts in the
> > hardware emulation code.
> >
> > That will be used for PCI-based NS8250 emulator.
>
>
> Strictly speaking the ns8250 uart should only generate ISA interrupts
> as I understand it, as it only uses IRQs 3 and 4? IOW from that code
> you should only need to use hvm_isa_irq_assert().

Correct, current code uses IRQ#3 and IRQ#4 only.
I dropped the patch from the series for now.

>
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
> > ---
> > xen/arch/x86/hvm/irq.c | 24 ++++++++++++++++++++++++
> > xen/arch/x86/include/asm/hvm/irq.h | 3 +++
> > 2 files changed, 27 insertions(+)
> >
> > diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
> > index 
> > 1eab44defca4c82ec35769617c66c380cc07d1b6..9e3a50d21dcf281c1015116094e47795c51ed5d0
> >  100644
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -242,6 +242,30 @@ void hvm_isa_irq_deassert(
> > spin_unlock(&d->arch.hvm.irq_lock);
> > }
> >
> > +void hvm_irq_raise(struct domain *d, unsigned int irq)
> > +{
> > + if ( irq < NR_ISAIRQS )
> > + {
> > + hvm_isa_irq_assert(d, irq, NULL);
> > + }
> > + else
> > + {
> > + hvm_gsi_assert(d, irq);
> > + }
> > +}
> > +
> > +void hvm_irq_lower(struct domain *d, unsigned int irq)
>
>
> It would be better to use the assert/deassert nomenclature, like it's
> used for the functions that are called.
>
> > +{
> > + if ( irq < NR_ISAIRQS )
> > + {
> > + hvm_isa_irq_deassert(d, irq);
> > + }
> > + else
> > + {
> > + hvm_gsi_deassert(d, irq);
> > + }
> > +}
>
>
> The parameter to thins function is kind of fuzzy, as I understand it,
> if the parameter is < NR_ISAIRQS it's an ISA IRQ, while if it's >=
>
> NR_ISAIRQS it's a GSI?

Yes, agree, mixing two address spaces is at least confusing.
I dropped the patch from the series.

>
> It would also be helpul to mention that hvm_isa_irq_deassert() will
> already do the ISA IRQ -> GSI conversion in case there are any source
>
> overrides.
>
> Thanks, Roger.