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

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

On Mon, 18 Feb 2013, Ian Campbell wrote:
> On Mon, 2013-02-18 at 14:44 +0000, Stefano Stabellini wrote:
> > 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.
> 
> Yes. Actually, I think the setting of CNTx_CTL_PENDING here is wrong
> also, it should be set only when the guest masks the interrupt.

I don't think so. It is a fact that the pending bit is set when an
interrupt is raised, in fact both Xen and Linux check for the pending
bit in the physical interrupt handler.

So the question is: should the pending bit also be set when an interrupt
should have been raised but it wasn't because the timer was masked?
As a matter of fact the answer is yes from my experiments on the
Versatile Express Cortex A15 machine I have.



> > > >  }
> > > >  
> > > >  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).
> 
> The manual says that the pending bit (which the newer docs call
> "istatus") is the interrupt status before masking, so this statement
> doesn't really make sense in that context because "even if the timer is
> masked" doesn't really mean anything given that, since this bit is only
> really useful when the interrupt is masked.

That is not true, both Xen and Linux test for that bit when receiving an
interrupt.
Also the manual states:

"""
The status of the timer interrupt:
0 Interrupt not asserted.
1 Interrupt asserted.
This bit is read-only.
A register write that sets IMASK to 1 latches this bit to reflect the
state of the interrupt immediately before that write.
"""

the fact that a register write that sets IMASK to 1 causes an update to
the pending bit, doesn't NOT mean that the pending bit is not also
updated when an interrupt is received.


> if there are discrepancies between the hardware and the documentation
> (which is on the confusing side in this case, although the newest doc is
> better) it would probably be better to get ARM to clarify than to guess
> what the behaviour is suppose to be.

I don't think is a discrepancy