On Mon, 2015-03-02 at 18:42 +0000, Stefano Stabellini wrote: > On Tue, 10 Feb 2015, Ian Campbell wrote: > > Stefano, > > > > do you have any comments on the viability of the general approach here > > before I go off and start addressing the review comments? > > I think that the general approach is OK > > > > Cheers, > > Ian. > > > > On Mon, 2015-01-26 at 15:55 +0000, Ian Campbell wrote: > > > ... instead of artificially masking the timer interrupt in the timer > > > state and relying on the guest to unmask (which it isn't required to > > > do per the h/w spec, although Linux does) > > > > > > To do this introduce the concept of routing a PPI to the currently > > > running VCPU. For such interrupts irq_get_domain returns NULL. > > > > > > Then we make use of the GICD I[SC]ACTIVER registers to save and > > > restore the active state of the interrupt, which prevents the nested > > > invocations which the current masking works around. > > > > > > For edge triggered interrupts we also need to context switch the > > > pending bit via I[SC]PENDR. Note that for level triggered interrupts > > > SPENDR sets a latch which is only cleared by ICPENDR (and not by h/w > > > state changes), therefore we do not want to context switch the pending > > > state for level PPIs -- instead we rely on the context switch of the > > > peripheral to restore the correct level. > > > > > > RFC Only: > > > - Not implemented for GIC v3 yet. > > > - Lightly tested with hackbench on systems with level and edge > > > triggered vtimer PPI. > > > - Is irq_get_domain == NULL to indicate route-to-current-vcpu the > > > best idea? Any alternatives? > > > > > > Signed-off-by: Ian Campbell <ian.campb...@citrix.com> > > > --- > > > xen/arch/arm/gic-v2.c | 84 > > > ++++++++++++++++++++++++++++++++++++++++++ > > > xen/arch/arm/gic.c | 32 +++++++++++++--- > > > xen/arch/arm/irq.c | 48 ++++++++++++++++++++++-- > > > xen/arch/arm/time.c | 26 +------------ > > > xen/arch/arm/vtimer.c | 24 ++++++++++-- > > > xen/include/asm-arm/domain.h | 11 ++++++ > > > xen/include/asm-arm/gic.h | 14 +++++++ > > > xen/include/asm-arm/irq.h | 1 + > > > 8 files changed, 204 insertions(+), 36 deletions(-) > > > > > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > > > index 31fb81a..9074aca 100644 > > > --- a/xen/arch/arm/gic-v2.c > > > +++ b/xen/arch/arm/gic-v2.c > > > @@ -156,6 +156,45 @@ static void gicv2_save_state(struct vcpu *v) > > > writel_gich(0, GICH_HCR); > > > } > > > > > > +static void gicv2_save_and_mask_hwppi(struct vcpu *v, const unsigned > > > virq, > > Shouldn't the argument be a physical irq? Maybe irq_desc?
I think it needs to be a virq in the namespace of the given v, since it may not be 1:1 and we would want to look it up to get the correct p. > > > > > + struct hwppi_state *s) > > > +{ > > > + struct pending_irq *p = irq_to_pending(v, virq); which we do here. I think such lookups are normally done within the (v)gic code rather than the callers, aren't they? > > > int gic_irq_xlate(const u32 *intspec, unsigned int intsize, > > > diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c > > > index ebdf19a..93c38ff 100644 > > > --- a/xen/arch/arm/irq.c > > > +++ b/xen/arch/arm/irq.c > > > @@ -202,6 +202,19 @@ void do_IRQ(struct cpu_user_regs *regs, unsigned int > > > irq, int is_fiq) > > > goto out; > > > } > > > > > > + if ( irq == current->arch.virt_timer.irq ) > > > + { > > > + ASSERT(!irq_get_domain(desc)); > > > + > > > + desc->handler->end(desc); > > > + > > > + set_bit(_IRQ_INPROGRESS, &desc->status); > > > + desc->arch.eoi_cpu = smp_processor_id(); > > > + > > > + vgic_vcpu_inject_irq(current, irq); > > > + goto out_no_end; > > > + } > > I don't think we should special case virt_timer.irq here. I would try to > reuse the normal _IRQ_GUEST path or make this if generic for any PPIs > routed to guests. Yes, I think I thought I had done that after making this quick hack the first time around but obviously I must have forgotten! > > > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c > > > index 848e2c6..d1f21a1 100644 > > > --- a/xen/arch/arm/vtimer.c > > > +++ b/xen/arch/arm/vtimer.c > > > @@ -47,9 +47,14 @@ static void phys_timer_expired(void *data) > > > static void virt_timer_expired(void *data) > > > { > > > struct vtimer *t = data; > > > - t->ctl |= CNTx_CTL_MASK; > > > - vgic_vcpu_inject_irq(t->v, t->irq); > > > - perfc_incr(vtimer_virt_inject); > > > + t->ctl |= CNTx_CTL_PENDING; > > > + if ( !(t->ctl & CNTx_CTL_MASK) ) > > > + { > > > + /* An edge triggered interrupt should now be pending. */ > > > + t->ppi_state.pending = true; > > > + vcpu_unblock(t->v); > > I was going to say that this is trouble because > local_events_need_delivery wouldn't recognize that we actually have an > event pending. But it doesn't matter because local_events_need_delivery > only works with the current vcpu and if this code trigger then the vcpu > that needs to receive the event cannot be current. So I think that is OK > but for clarity it might be better to add a check or a comment in > local_events_need_delivery_nomask anyway. i.e. a BUG_ON(t->v == current) + a comment to the affect that the timer must never expire for the current vcpu? > > > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > > > index 90ab9c3..dd50e2c 100644 > > > --- a/xen/include/asm-arm/domain.h > > > +++ b/xen/include/asm-arm/domain.h > > > @@ -34,12 +34,23 @@ enum domain_type { > > > extern int dom0_11_mapping; > > > #define is_domain_direct_mapped(d) ((d) == hardware_domain && > > > dom0_11_mapping) > > > > > > +struct hwppi_state { > > > + /* h/w state */ > > > + unsigned long enabled:1; > > > + unsigned long pending:1; > > > + unsigned long active:1; > > > + > > > + /* Xen s/w state */ > > > + unsigned long inprogress:1; > > > +}; > > Using a uint32_t bitmask would be more in line the rest of the code > style, but it is just a matter of taste. You mean "uint32_t inprogress:1" for each? Or #define ENAVBLED 1 #define PENDING 2 etc and test_set_bit stuff? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel