| Message ID | 1395232325-19226-7-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On 03/19/2014 12:32 PM, Stefano Stabellini wrote: > This change is needed by other patches later on. It is going to make > sure that the calculation in Xen of the highest priority interrupt > currently inflight is correct and accurate and not based on stale data. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Acked-by: Julien Grall <julien.grall@linaro.org>
On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: Can this not be folded back into the patch which added this function? > This change is needed by other patches later on. It is going to make > sure that the calculation in Xen of the highest priority interrupt > currently inflight is correct and accurate and not based on stale data. Hrm, can we not do this on demand just at the point where we are about to make such a calculation? There are going to be lots of hypervisor entries which don't want to do anything at all with interrupts, aren't there? Ian.
On Fri, 21 Mar 2014, Ian Campbell wrote: > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: > > Can this not be folded back into the patch which added this function? Yes, it can. > > This change is needed by other patches later on. It is going to make > > sure that the calculation in Xen of the highest priority interrupt > > currently inflight is correct and accurate and not based on stale data. > > Hrm, can we not do this on demand just at the point where we are about > to make such a calculation? There are going to be lots of hypervisor > entries which don't want to do anything at all with interrupts, aren't > there? The alternative would be calling gic_clear_lrs at the beginning of gic_inject and gic_events_need_delivery, that is called by local_events_need_delivery*. It could be called multiple times before returning to guest. However gic_clear_lrs only iterates over the LRs in lr_mask, so even calling it multiple times shouldn't cause more work, the only slow down would be due to the spin_lock.
On Fri, 2014-03-21 at 16:34 +0000, Stefano Stabellini wrote: > On Fri, 21 Mar 2014, Ian Campbell wrote: > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: > > > > Can this not be folded back into the patch which added this function? > > Yes, it can. > > > > > This change is needed by other patches later on. It is going to make > > > sure that the calculation in Xen of the highest priority interrupt > > > currently inflight is correct and accurate and not based on stale data. > > > > Hrm, can we not do this on demand just at the point where we are about > > to make such a calculation? There are going to be lots of hypervisor > > entries which don't want to do anything at all with interrupts, aren't > > there? > > The alternative would be calling gic_clear_lrs at the beginning of > gic_inject and gic_events_need_delivery, that is called by > local_events_need_delivery*. It could be called multiple times before > returning to guest. We could probably invent some sort of "once per h/v entry" construct, but ick. What I was actually thinking of though was to do it even further up -- e.g. in the function which makes the interrupt pending in the first place. I suppose that is called too infrequently though in the absence of maintenance interrupts. > > However gic_clear_lrs only iterates over the LRs in lr_mask, so even > calling it multiple times shouldn't cause more work, the only slow down > would be due to the spin_lock.
On Fri, 21 Mar 2014, Ian Campbell wrote: > On Fri, 2014-03-21 at 16:34 +0000, Stefano Stabellini wrote: > > On Fri, 21 Mar 2014, Ian Campbell wrote: > > > On Wed, 2014-03-19 at 12:32 +0000, Stefano Stabellini wrote: > > > > > > Can this not be folded back into the patch which added this function? > > > > Yes, it can. > > > > > > > > This change is needed by other patches later on. It is going to make > > > > sure that the calculation in Xen of the highest priority interrupt > > > > currently inflight is correct and accurate and not based on stale data. > > > > > > Hrm, can we not do this on demand just at the point where we are about > > > to make such a calculation? There are going to be lots of hypervisor > > > entries which don't want to do anything at all with interrupts, aren't > > > there? > > > > The alternative would be calling gic_clear_lrs at the beginning of > > gic_inject and gic_events_need_delivery, that is called by > > local_events_need_delivery*. It could be called multiple times before > > returning to guest. > > We could probably invent some sort of "once per h/v entry" construct, > but ick. > > What I was actually thinking of though was to do it even further up -- > e.g. in the function which makes the interrupt pending in the first > place. I suppose that is called too infrequently though in the absence > of maintenance interrupts. Yes, that function would not be called often enough: in order to calculate correctly if events need delivery, we need to know the current value of GICV_PMR.Priority, that is aliased by GICH_VMCR and can be changed by the guest without traps, and the current active highest priority interrupt in the LRs. As a consequence I think that gic_events_need_delivery needs to be called at least once before returning to guest.
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index a5a4da3..3f061cf 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -67,8 +67,6 @@ static DEFINE_PER_CPU(u8, gic_cpu_id); /* Maximum cpu interface per GIC */ #define NR_GIC_CPU_IF 8 -static void gic_clear_lrs(struct vcpu *v); - static unsigned int gic_cpu_mask(const cpumask_t *cpumask) { unsigned int cpu; @@ -130,7 +128,6 @@ void gic_restore_state(struct vcpu *v) GICH[GICH_HCR] = GICH_HCR_EN; isb(); - gic_clear_lrs(v); gic_restore_pending_irqs(v); } @@ -701,14 +698,15 @@ out: return; } -static void gic_clear_lrs(struct vcpu *v) +void gic_clear_lrs(struct vcpu *v) { struct pending_irq *p; int i = 0, irq; uint32_t lr; bool_t inflight; + unsigned long flags; - ASSERT(!local_irq_is_enabled()); + spin_lock_irqsave(&v->arch.vgic.lock, flags); while ((i = find_next_bit((const long unsigned int *) &this_cpu(lr_mask), nr_lrs, i)) < nr_lrs) { @@ -744,6 +742,8 @@ static void gic_clear_lrs(struct vcpu *v) i++; } + + spin_unlock_irqrestore(&v->arch.vgic.lock, flags); } static void gic_restore_pending_irqs(struct vcpu *v) @@ -786,8 +786,6 @@ int gic_events_need_delivery(void) void gic_inject(void) { - gic_clear_lrs(current); - if ( vcpu_info(current, evtchn_upcall_pending) ) vgic_vcpu_inject_irq(current, current->domain->arch.evtchn_irq); diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 21c7b26..dd936be 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -68,6 +68,7 @@ static int debug_stack_lines = 40; integer_param("debug_stack_lines", debug_stack_lines); +static void enter_hypervisor_head(void); void __cpuinit init_traps(void) { @@ -1543,6 +1544,8 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) { union hsr hsr = { .bits = READ_SYSREG32(ESR_EL2) }; + enter_hypervisor_head(); + switch (hsr.ec) { case HSR_EC_WFI_WFE: if ( !check_conditional_instr(regs, hsr) ) @@ -1620,11 +1623,13 @@ asmlinkage void do_trap_hypervisor(struct cpu_user_regs *regs) asmlinkage void do_trap_irq(struct cpu_user_regs *regs) { + enter_hypervisor_head(); gic_interrupt(regs, 0); } asmlinkage void do_trap_fiq(struct cpu_user_regs *regs) { + enter_hypervisor_head(); gic_interrupt(regs, 1); } @@ -1642,6 +1647,11 @@ asmlinkage void leave_hypervisor_tail(void) } } +static void enter_hypervisor_head(void) +{ + gic_clear_lrs(current); +} + /* * Local variables: * mode: C diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index 4834cd6..5a9dc77 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -220,6 +220,7 @@ extern unsigned int gic_number_lines(void); /* IRQ translation function for the device tree */ int gic_irq_xlate(const u32 *intspec, unsigned int intsize, unsigned int *out_hwirq, unsigned int *out_type); +void gic_clear_lrs(struct vcpu *v); #endif /* __ASSEMBLY__ */ #endif
This change is needed by other patches later on. It is going to make sure that the calculation in Xen of the highest priority interrupt currently inflight is correct and accurate and not based on stale data. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- xen/arch/arm/gic.c | 12 +++++------- xen/arch/arm/traps.c | 10 ++++++++++ xen/include/asm-arm/gic.h | 1 + 3 files changed, 16 insertions(+), 7 deletions(-)