| Message ID | 1372115067-17071-2-git-send-email-julien.grall@citrix.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 25 Jun 2013, Julien Grall wrote: > From: Julien Grall <julien.grall@linaro.org> > > 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@linaro.org> > --- > 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.
On 06/25/2013 02:16 PM, Stefano Stabellini wrote: > On Tue, 25 Jun 2013, Julien Grall wrote: >> From: Julien Grall <julien.grall@linaro.org> >> >> 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@linaro.org> >> --- >> 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. Right. I wrote this patch because it easier to forget to modify some part when non-1:1 IRQ mappings will be created :).
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@linaro.org> > >> > >> 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@linaro.org> > >> --- > >> 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.
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++;