Re: [PATCH for-4.14 5/8] x86/hvm: only translate ISA interrupts to GSIs in virtual timers

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Jun 18, 2020 at 04:47:57PM +0200, Jan Beulich wrote:
> On 12.06.2020 17:56, Roger Pau Monne wrote:
> > Only call hvm_isa_irq_to_gsi for ISA interrupts, interrupts
> > originating from an IO APIC pin already use a GSI and don't need to be
> > translated.
> > 
> > I haven't observed any issues from this, but I think it's better to
> > use it correctly.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> However, ...
> 
> > --- a/xen/arch/x86/hvm/vpt.c
> > +++ b/xen/arch/x86/hvm/vpt.c
> > @@ -86,7 +86,7 @@ static int pt_irq_vector(struct periodic_time *pt, enum 
> > hvm_intsrc src)
> >          return pt->irq;
> >  
> >      isa_irq = pt->irq;
> > -    gsi = hvm_isa_irq_to_gsi(isa_irq);
> > +    gsi = pt->source == PTSRC_isa ? hvm_isa_irq_to_gsi(isa_irq) : pt->irq;
> 
> ... would you mind taking the opportunity and moving this ...
> 
> >      if ( src == hvm_intsrc_pic )
> >          return (v->domain->arch.hvm.vpic[isa_irq >> 3].irq_base
> 
> ... below here, perhaps even past the ASSERT() that follows?
> 
> (I have to admit that I find the two kinds of "source" indicators
> - the "src" function parameter and "pt->source" confusing. Aren't
> they supposed to match up?)

They are supposed to match when the injected interrupt is the timer
one, if there's a highest priority interrupt that gets injected
instead of the timer one they don't match.

AFAICT the function it's trying to get the vector that would match the
timer using the 'src' interrupt source. TBH I think this is way more
complex than needed, but I don't plan to deal with it right now.

Thanks, Roger.