Re: [PATCH 7/7] xen/arm: phys_timer fixes

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

On Fri, 15 Feb 2013, Ian Campbell wrote:
> > @@ -33,8 +34,8 @@ static void phys_timer_expired(void *data)
> >  {
> >      struct vtimer *t = data;
> >      t->ctl |= CNTx_CTL_PENDING;
> > -    t->ctl &= ~CNTx_CTL_MASK;
> > -    vgic_vcpu_inject_irq(t->v, 30, 1);
> > +    if ( !(t->ctl & CNTx_CTL_MASK) )
> > +        vgic_vcpu_inject_irq(t->v, 30, 1);
> 
> Thinking about the previous discussion about exposing the change of the
> mask bit to the guest, it just occurred to me that t->ctl.MASK need not
> actually represent the state of the underlying physical mask bit, since
> we do emulate accesses after all.

Sure. However this change still makes sense: if the guest masked the
timer (no matter how we internally represent it), we should not inject
an interrupt.


> >  }
> >  
> >  static void virt_timer_expired(void *data)
> > @@ -44,6 +45,17 @@ static void virt_timer_expired(void *data)
> >      vgic_vcpu_inject_irq(t->v, 27, 1);
> >  }
> >   
> > +static void phys_timer_gic_callback(struct vcpu *v, int irq)
> > +{
> > +    v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
> > +}
> > +
> > +int __init init_ptimer(void)
> > +{
> > +    register_gic_callback(30, phys_timer_gic_callback);
> > +    return 0;
> > +}
> > +
> >  int vcpu_vtimer_init(struct vcpu *v)
> >  {
> >      struct vtimer *t = &v->arch.phys_timer;
> > @@ -119,13 +131,15 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, union hsr hsr)
> >          {
> >              v->arch.phys_timer.ctl = *r;
> >  
> > -            if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> > +            if ( ((v->arch.phys_timer.ctl & CNTx_CTL_MASK) &&
> > +                  (v->arch.phys_timer.ctl & CNTx_CTL_PENDING)) ||
> 
> Why does PENDING matter here and below?

Doing tests on the physical hardware to better understand its behaviour,
I noticed that the pending bit gets set even if the timer is masked
(and therefore no interrupt is asserted).

However if the pending bit is already set there is no point for us to
try to set it again, so we skip setting the internal Xen timer.