Re: [Xen-devel] [PATCH 1/5] xen/arm: Physical IRQ is not always equal to virtual IRQ

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2013-06-25 at 16:21 +0100, Julien Grall wrote:
> On 06/25/2013 02:16 PM, Stefano Stabellini wrote:
> 
> > On Tue, 25 Jun 2013, Julien Grall wrote:
> >> From: Julien Grall <julien.grall@xxxxxxxxxx>
> >>
> >> When Xen needs to EOI a physical IRQ, we must use the IRQ number
> >> in irq_desc instead of the virtual IRQ.
> >>
> >> Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
> >> ---
> >>  xen/arch/arm/gic.c |    7 ++++---
> >>  1 file changed, 4 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> >> index 177560e..0fee3f2 100644
> >> --- a/xen/arch/arm/gic.c
> >> +++ b/xen/arch/arm/gic.c
> >> @@ -810,7 +810,7 @@ static void gic_irq_eoi(void *info)
> >>  
> >>  static void maintenance_interrupt(int irq, void *dev_id, struct 
> >> cpu_user_regs *regs)
> >>  {
> >> -    int i = 0, virq;
> >> +    int i = 0, virq, pirq;
> >>      uint32_t lr;
> >>      struct vcpu *v = current;
> >>      uint64_t eisr = GICH[GICH_EISR0] | (((uint64_t) GICH[GICH_EISR1]) << 
> >> 32);
> >> @@ -846,6 +846,7 @@ static void maintenance_interrupt(int irq, void 
> >> *dev_id, struct cpu_user_regs *r
> >>              /* Assume only one pcpu needs to EOI the irq */
> >>              cpu = p->desc->arch.eoi_cpu;
> >>              eoi = 1;
> >> +            pirq = p->desc->irq;
> >>          }
> >>          list_del_init(&p->inflight);
> >>          spin_unlock_irq(&v->arch.vgic.lock);
> >> @@ -854,10 +855,10 @@ static void maintenance_interrupt(int irq, void 
> >> *dev_id, struct cpu_user_regs *r
> >>              /* this is not racy because we can't receive another irq of 
> >> the
> >>               * same type until we EOI it.  */
> >>              if ( cpu == smp_processor_id() )
> >> -                gic_irq_eoi((void*)(uintptr_t)virq);
> >> +                gic_irq_eoi((void*)(uintptr_t)pirq);
> >>              else
> >>                  on_selected_cpus(cpumask_of(cpu),
> >> -                                 gic_irq_eoi, (void*)(uintptr_t)virq, 0);
> >> +                                 gic_irq_eoi, (void*)(uintptr_t)pirq, 0);
> >>          }
> > 
> > I think that virq and pirq are guaranteed to always be the same, at
> > least at the moment. Look at vgic_vcpu_inject_irq: it takes just one irq
> > parameter, that is both the physical and the virtual irq number.
> 
> > Unless we change the vgic_vcpu_inject_irq interface to allow virq !=
> > pirq, I don't think this patch makes much sense.

But what is the downside?

> Right. I wrote this patch because it easier to forget to modify some
> part when non-1:1 IRQ mappings will be created :).

I'd be tempted to make this change on that basis, it is correct both
before and after any change to vgic_vcpu_inject_irq and doesn't appear
to be expensive or anything. Not to mention that it is semantically
correct.


Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel