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

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

[Stefano Stabellini]

Do not unmask the emulated phys_timer when the related Xen timer
expires.
Do not inject the phys_timer interrupt if it is masked.

Stop the Xen timer if the pending bit is already set and the phys_timer
is masked.

Register a gic callback to clear the pending bit in the ctl register.

Define offset and cval as uint64_t given that they can't be negative and
they are used as uint64_t arguments.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/setup.c         |    1 +
 xen/arch/arm/vtimer.c        |   30 ++++++++++++++++++++++++------
 xen/arch/arm/vtimer.h        |    2 ++
 xen/include/asm-arm/domain.h |    4 ++--
 4 files changed, 29 insertions(+), 8 deletions(-)

diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 5680c73..f1d15bb 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -398,6 +398,7 @@ void __init start_xen(unsigned long boot_phys_offset,
     init_timer_interrupt();
 
     timer_init();
+    init_ptimer();
 
     init_idle_domain();
 
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index f4326f8..e033191 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -18,6 +18,7 @@
  */
 
 #include <xen/config.h>
+#include <xen/init.h>
 #include <xen/lib.h>
 #include <xen/timer.h>
 #include <xen/sched.h>
@@ -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);
 }
 
 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)) ||
+                 !(v->arch.phys_timer.ctl & CNTx_CTL_ENABLE) )
+                stop_timer(&v->arch.phys_timer.timer);
+            else
             {
                 set_timer(&v->arch.phys_timer.timer,
                           v->arch.phys_timer.cval + v->arch.phys_timer.offset);
             }
-            else
-                stop_timer(&v->arch.phys_timer.timer);
         }
 
         return 1;
@@ -139,7 +153,11 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, union hsr hsr)
         else
         {
             v->arch.phys_timer.cval = now + ticks_to_ns(*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)) ||
+                 !(v->arch.phys_timer.ctl & CNTx_CTL_ENABLE) )
+                stop_timer(&v->arch.phys_timer.timer);
+            else
             {
                 set_timer(&v->arch.phys_timer.timer,
                           v->arch.phys_timer.cval + v->arch.phys_timer.offset);
diff --git a/xen/arch/arm/vtimer.h b/xen/arch/arm/vtimer.h
index 43eef69..cbe0470 100644
--- a/xen/arch/arm/vtimer.h
+++ b/xen/arch/arm/vtimer.h
@@ -26,6 +26,8 @@ extern int virt_timer_save(struct vcpu *v);
 extern int virt_timer_restore(struct vcpu *v);
 extern void vcpu_timer_destroy(struct vcpu *v);
 
+int init_ptimer(void);
+
 #endif
 
 /*
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 577ad19..3e9cc37 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -75,8 +75,8 @@ struct vtimer {
         int irq;
         struct timer timer;
         uint32_t ctl;
-        s_time_t offset;
-        s_time_t cval;
+        uint64_t offset;
+        uint64_t cval;
 };
 
 struct arch_vcpu
-- 
1.7.2.5

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

[Ian Campbell]

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

>  }
>  
>  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?

> +                 !(v->arch.phys_timer.ctl & CNTx_CTL_ENABLE) )
> +                stop_timer(&v->arch.phys_timer.timer);
> +            else
>              {
>                  set_timer(&v->arch.phys_timer.timer,
>                            v->arch.phys_timer.cval + v->arch.phys_timer.offset);
>              }
> -            else
> -                stop_timer(&v->arch.phys_timer.timer);
>          }
>  
>          return 1;
> @@ -139,7 +153,11 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, union hsr hsr)
>          else
>          {
>              v->arch.phys_timer.cval = now + ticks_to_ns(*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)) ||
> +                 !(v->arch.phys_timer.ctl & CNTx_CTL_ENABLE) )
> +                stop_timer(&v->arch.phys_timer.timer);
> +            else
>              {
>                  set_timer(&v->arch.phys_timer.timer,
>                            v->arch.phys_timer.cval + v->arch.phys_timer.offset);

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

[Stefano Stabellini]

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.

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

[Ian Campbell]

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.


> 
> 
> > >  }
> > >  
> > >  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.

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.

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

I have a feeling this is only true because we incorrectly set the
pending bit in phys_timer_expired().

Ian.

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

[Stefano Stabellini]

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

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

[Ian Campbell]

On Mon, 2013-02-18 at 15:31 +0000, Stefano Stabellini wrote:
> 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,

Perhaps it is the case that:
	MASKED == 0 => PENDING represents the "live" state of the 
			interrupt
	MASKED == 1 => PENDING is latched to what ever it was when the 
			mask bit was set
?

That is consistent with the docs as I read them, but not very explicit
(since they only really specify the MASKED==1 case). This would be worth
clarifying with ARM I think.

>  in fact both Xen and Linux check for the pending
> bit in the physical interrupt handler.

Right, if the behaviour is as I described then they are not buggy to do
so without masking the interrupt.

However even with the behaviour above this patch is not implementing the
pending behaviour correctly, since it is cleared unconditionally in
phys_timer_gic_callback(). Also it should be r/o from the guest point of
view.

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

The answer may well be yes *on the Cortex A15*. That is why I would like
to see this clarified with ARM. Especially given that this is not
consistent with what the docs say, since they preclude PENDING changing
while MASK is set:
        A register write that sets IMASK to 1 latches this bit to
        reflect the state of the interrupt immediately before that
        write.

The key word is "latches" here -- although it's not clear until when I
think it is likely to be until the mask bit is cleared (another thing to
ask about I think).

It is certainly possible for the timer to fire but be masked and for it
to trigger the interrupt immediately when the mask bit is subsequently
cleared, but that does not imply that the pending bit changes at the
point where it would have fired while it was masked.

> > > > >  }
> > > > >  
> > > > >  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.

The word "latches" does (or at least could) cause it to mean that
though. 

> > 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

At the very least the documentation is unclear, or we wouldn't be having
this conversation.

Ian.

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

[Stefano Stabellini]

On Mon, 18 Feb 2013, Ian Campbell wrote:
> On Mon, 2013-02-18 at 15:31 +0000, Stefano Stabellini wrote:
> > 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,
> 
> Perhaps it is the case that:
> 	MASKED == 0 => PENDING represents the "live" state of the 
> 			interrupt
> 	MASKED == 1 => PENDING is latched to what ever it was when the 
> 			mask bit was set
> ?
> 
> That is consistent with the docs as I read them, but not very explicit
> (since they only really specify the MASKED==1 case). This would be worth
> clarifying with ARM I think.
> 
> >  in fact both Xen and Linux check for the pending
> > bit in the physical interrupt handler.
> 
> Right, if the behaviour is as I described then they are not buggy to do
> so without masking the interrupt.
> 
> However even with the behaviour above this patch is not implementing the
> pending behaviour correctly, since it is cleared unconditionally in
> phys_timer_gic_callback(). Also it should be r/o from the guest point of
> view.

You are right about being RO. See below regarding the "latch" case.


> > 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.
> 
> The answer may well be yes *on the Cortex A15*. That is why I would like
> to see this clarified with ARM. Especially given that this is not
> consistent with what the docs say, since they preclude PENDING changing
> while MASK is set:
>         A register write that sets IMASK to 1 latches this bit to
>         reflect the state of the interrupt immediately before that
>         write.
> 
> The key word is "latches" here -- although it's not clear until when I
> think it is likely to be until the mask bit is cleared (another thing to
> ask about I think).
> 
> It is certainly possible for the timer to fire but be masked and for it
> to trigger the interrupt immediately when the mask bit is subsequently
> cleared, but that does not imply that the pending bit changes at the
> point where it would have fired while it was masked.

I think they mean "latch" in electronic sense: they use an SR latch to
keep the pending bit high even if the guest EOIs the interrupt, as long
as the mask bit is 1.
In other words the pending bit cannot be reset if the mask bit is 1.

So I should prevent the guest from resetting this bit directly in all
cases and also I should not reset the bit on EOI if the mask bit is 1.

I agree that is confusing though.

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

[Ian Campbell]

On Mon, 2013-02-18 at 16:14 +0000, Stefano Stabellini wrote:
> I think they mean "latch" in electronic sense: they use an SR latch to
> keep the pending bit high even if the guest EOIs the interrupt, as long
> as the mask bit is 1.

> In other words the pending bit cannot be reset if the mask bit is 1.

"latching high" is a plausible interpretation of the docs given the
observed behaviour on the A15.

We shouldn't assume that just because it appears to behave on way on the
A15, for all we know the state of the PENDING bit while mask==0 is
explicitly implementation defined and just happens to follow the state
of the interrupt in this particular implementation (yes, this would make
Linux and Xen both buggy in their handling). After all we don't have any
alternative implementations to compare with...

So I'd still prefer to check with ARM, rather than potentially bake a
buggy "hardware" implementation into our interface.

> So I should prevent the guest from resetting this bit directly in all
> cases and also I should not reset the bit on EOI if the mask bit is 1.

You also need to do the right thing when the guest clears the mask bit,
which is going to require you to track if the interrupt has been EOId
separately from the pending bit.

> I agree that is confusing though.

Very!