Re: [PATCH RFC] xen: arm: context switch vtimer PPI state.

[Ian Campbell <ian.campbell@citrix.com>]

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.campbell@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.