| Message ID | 20200612155640.4101-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/vpt: fixes for vpt and enable vPIT for PVH dom0 | expand |
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@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> 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?) Jan
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@citrix.com> > > Reviewed-by: Jan Beulich <jbeulich@suse.com> > > 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.
diff --git a/xen/arch/x86/hvm/vpt.c b/xen/arch/x86/hvm/vpt.c index 62c87867c5..6a975fc668 100644 --- 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; if ( src == hvm_intsrc_pic ) return (v->domain->arch.hvm.vpic[isa_irq >> 3].irq_base
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@citrix.com> --- xen/arch/x86/hvm/vpt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)