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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 3 Mar 2015, Ian Campbell wrote:
> 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@xxxxxxxxxx>
> > > > ---
> > > >  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?

I think it would be best just to add a comment in
local_events_need_delivery to remember these events are not listed
there.


> > > > 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?
 
Something like status in xen/include/asm-arm/vgic.h:struct pending_irq

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