Xen devel - Jan 2013 - [PATCH 1/2] xen/arm: support the ARM generic virtual timer

Save and restore the virtual timer registers during the context switch.
At save time initialize an internal Xen timer to make sure that Xen
schedules the guest vcpu at the time of the next virtual timer
interrupt.
Receive the virtual timer interrupt into the hypervisor and inject it
into the running guest.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/arch/arm/domain.c        |   13 ++----
 xen/arch/arm/gic.c           |    4 +-
 xen/arch/arm/time.c          |   14 ++++++-
 xen/arch/arm/vtimer.c        |   91 +++++++++++++++++++++++++++++++-----------
 xen/arch/arm/vtimer.h        |    2 +
 xen/include/asm-arm/domain.h |   23 ++++++-----
 xen/include/asm-arm/time.h   |    3 +
 7 files changed, 105 insertions(+), 45 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index c5292c7..193a931 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -51,9 +51,7 @@ static void ctxt_switch_from(struct vcpu *p)
     p->arch.tpidrprw = READ_CP32(TPIDRPRW);
 
     /* Arch timer */
-    p->arch.cntvoff = READ_CP64(CNTVOFF);
-    p->arch.cntv_cval = READ_CP64(CNTV_CVAL);
-    p->arch.cntv_ctl = READ_CP32(CNTV_CTL);
+    virt_timer_save(p);
 
     /* XXX only save these if ThumbEE e.g. ID_PFR0.THUMB_EE_SUPPORT */
     p->arch.teecr = READ_CP32(TEECR);
@@ -134,11 +132,6 @@ static void ctxt_switch_to(struct vcpu *n)
     WRITE_CP32(n->arch.mair1, MAIR1);
     isb();
 
-    /* Arch timer */
-    WRITE_CP64(n->arch.cntvoff, CNTVOFF);
-    WRITE_CP64(n->arch.cntv_cval, CNTV_CVAL);
-    WRITE_CP32(n->arch.cntv_ctl, CNTV_CTL);
-
     /* Control Registers */
     WRITE_CP32(n->arch.actlr, ACTLR);
     WRITE_CP32(n->arch.sctlr, SCTLR);
@@ -165,6 +158,10 @@ static void ctxt_switch_to(struct vcpu *n)
 
     WRITE_CP32(hcr, HCR);
     isb();
+
+    /* This is could trigger an hardware interrupt from the virtual
+     * timer. The interrupt needs to be injected into the guest. */
+    virt_timer_restore(n);
 }
 
 static void schedule_tail(struct vcpu *prev)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 81188f0..8c00a1c 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -373,7 +373,9 @@ void gic_route_ppis(void)
     gic_route_irq(25, 1, 1u << smp_processor_id(), 0xa0);
     /* Hypervisor Timer */
     gic_route_irq(26, 1, 1u << smp_processor_id(), 0xa0);
-    /* Timer */
+    /* Virtual Timer */
+    gic_route_irq(27, 1, 1u << smp_processor_id(), 0xa0);
+    /* Physical Timer */
     gic_route_irq(30, 1, 1u << smp_processor_id(), 0xa0);
 }
 
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index b6d7015..9515b42 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -24,8 +24,11 @@
 #include <xen/lib.h>
 #include <xen/mm.h>
 #include <xen/softirq.h>
+#include <xen/sched.h>
 #include <xen/time.h>
 #include <asm/system.h>
+#include <asm/time.h>
+#include "gic.h"
 
 /*
  * Unfortunately the hypervisor timer interrupt appears to be buggy in
@@ -34,10 +37,11 @@
  */
 #define USE_HYP_TIMER 1
 
+uint64_t __read_mostly boot_count;
+
 /* For fine-grained timekeeping, we use the ARM "Generic Timer", a
  * register-mapped time source in the SoC. */
 static uint32_t __read_mostly cntfrq;      /* Ticks per second */
-static uint64_t __read_mostly boot_count;  /* Counter value at boot time */
 
 /*static inline*/ s_time_t ticks_to_ns(uint64_t ticks)
 {
@@ -153,6 +157,13 @@ static void timer_interrupt(int irq, void *dev_id, struct
cpu_user_regs *regs)
     }
 }
 
+static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
+{
+    current->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
+    WRITE_CP32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL);
+    vgic_vcpu_inject_irq(current, irq, 1);
+}
+
 /* Set up the timer interrupt on this CPU */
 void __cpuinit init_timer_interrupt(void)
 {
@@ -172,6 +183,7 @@ void __cpuinit init_timer_interrupt(void)
 
     /* XXX Need to find this IRQ number from devicetree? */
     request_irq(26, timer_interrupt, 0, "hyptimer", NULL);
+    request_irq(27, vtimer_interrupt, 0, "virtimer", NULL);
     request_irq(30, timer_interrupt, 0, "phytimer", NULL);
 }
 
diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index 490b021..598fd87 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -21,27 +21,70 @@
 #include <xen/lib.h>
 #include <xen/timer.h>
 #include <xen/sched.h>
+#include <asm/irq.h>
+#include <asm/time.h>
 #include "gic.h"
 
 extern s_time_t ticks_to_ns(uint64_t ticks);
 extern uint64_t ns_to_ticks(s_time_t ns);
 
-static void vtimer_expired(void *data)
+static void phys_timer_expired(void *data)
 {
-    struct vcpu *v = data;
-    v->arch.vtimer.ctl |= CNTx_CTL_PENDING;
-    v->arch.vtimer.ctl &= ~CNTx_CTL_MASK;
-    vgic_vcpu_inject_irq(v, 30, 1);
+    struct vtimer *t = data;
+    t->ctl |= CNTx_CTL_PENDING;
+    t->ctl &= ~CNTx_CTL_MASK;
+    vgic_vcpu_inject_irq(t->v, 30, 1);
+}
+
+static void virt_timer_expired(void *data)
+{
+    struct vtimer *t = data;
+    vcpu_wake(t->v);
 }
 
 int vcpu_vtimer_init(struct vcpu *v)
 {
-    init_timer(&v->arch.vtimer.timer,
-               vtimer_expired, v,
-               smp_processor_id());
-    v->arch.vtimer.ctl = 0;
-    v->arch.vtimer.offset = NOW();
-    v->arch.vtimer.cval = NOW();
+    struct vtimer *t = &v->arch.phys_timer;
+
+    init_timer(&t->timer, phys_timer_expired, t, smp_processor_id());
+    t->ctl = 0;
+    t->offset = NOW();
+    t->cval = NOW();
+    t->irq = 30;
+    t->v = v;
+
+    t = &v->arch.virt_timer;
+    init_timer(&t->timer, virt_timer_expired, t, smp_processor_id());
+    t->ctl = 0;
+    t->offset = READ_CP64(CNTVCT) + READ_CP64(CNTVOFF);
+    t->cval = 0;
+    t->irq = 27;
+    t->v = v;
+
+    return 0;
+}
+
+int virt_timer_save(struct vcpu *v)
+{
+    v->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
+    WRITE_CP32(v->arch.virt_timer.ctl & ~CNTx_CTL_ENABLE, CNTV_CTL);
+    v->arch.virt_timer.cval = READ_CP64(CNTV_CVAL);
+    if ( v->arch.virt_timer.ctl & CNTx_CTL_ENABLE )
+    {
+        set_timer(&v->arch.virt_timer.timer,
ticks_to_ns(v->arch.virt_timer.cval +
+                  v->arch.virt_timer.offset - boot_count));
+    }
+    return 0;
+}
+
+int virt_timer_restore(struct vcpu *v)
+{
+    stop_timer(&v->arch.virt_timer.timer);
+
+    WRITE_CP32(v->arch.virt_timer.ctl & ~CNTx_CTL_ENABLE, CNTV_CTL);
+    WRITE_CP64(v->arch.virt_timer.offset, CNTVOFF);
+    WRITE_CP64(v->arch.virt_timer.cval, CNTV_CVAL);
+    WRITE_CP32(v->arch.virt_timer.ctl, CNTV_CTL);
     return 0;
 }
 
@@ -57,36 +100,36 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs,
union hsr hsr)
     case HSR_CPREG32(CNTP_CTL):
         if ( cp32.read )
         {
-            *r = v->arch.vtimer.ctl;
+            *r = v->arch.phys_timer.ctl;
         }
         else
         {
-            v->arch.vtimer.ctl = *r;
+            v->arch.phys_timer.ctl = *r;
 
-            if ( v->arch.vtimer.ctl & CNTx_CTL_ENABLE )
+            if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
             {
-                set_timer(&v->arch.vtimer.timer,
-                          v->arch.vtimer.cval + v->arch.vtimer.offset);
+                set_timer(&v->arch.phys_timer.timer,
+                          v->arch.phys_timer.cval +
v->arch.phys_timer.offset);
             }
             else
-                stop_timer(&v->arch.vtimer.timer);
+                stop_timer(&v->arch.phys_timer.timer);
         }
 
         return 1;
 
     case HSR_CPREG32(CNTP_TVAL):
-        now = NOW() - v->arch.vtimer.offset;
+        now = NOW() - v->arch.phys_timer.offset;
         if ( cp32.read )
         {
-            *r = (uint32_t)(ns_to_ticks(v->arch.vtimer.cval - now) &
0xffffffffull);
+            *r = (uint32_t)(ns_to_ticks(v->arch.phys_timer.cval - now) &
0xffffffffull);
         }
         else
         {
-            v->arch.vtimer.cval = now + ticks_to_ns(*r);
-            if ( v->arch.vtimer.ctl & CNTx_CTL_ENABLE )
+            v->arch.phys_timer.cval = now + ticks_to_ns(*r);
+            if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
             {
-                set_timer(&v->arch.vtimer.timer,
-                          v->arch.vtimer.cval + v->arch.vtimer.offset);
+                set_timer(&v->arch.phys_timer.timer,
+                          v->arch.phys_timer.cval +
v->arch.phys_timer.offset);
             }
         }
 
@@ -111,7 +154,7 @@ static int vtimer_emulate_64(struct cpu_user_regs *regs,
union hsr hsr)
     case HSR_CPREG64(CNTPCT):
         if ( cp64.read )
         {
-            now = NOW() - v->arch.vtimer.offset;
+            now = NOW() - v->arch.phys_timer.offset;
             ticks = ns_to_ticks(now);
             *r1 = (uint32_t)(ticks & 0xffffffff);
             *r2 = (uint32_t)(ticks >> 32);
diff --git a/xen/arch/arm/vtimer.h b/xen/arch/arm/vtimer.h
index d87bb25..faebd68 100644
--- a/xen/arch/arm/vtimer.h
+++ b/xen/arch/arm/vtimer.h
@@ -22,6 +22,8 @@
 
 extern int vcpu_vtimer_init(struct vcpu *v);
 extern int vtimer_emulate(struct cpu_user_regs *regs, union hsr hsr);
+extern int virt_timer_save(struct vcpu *v);
+extern int virt_timer_restore(struct vcpu *v);
 
 #endif
 
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 114a8f6..577ad19 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -3,6 +3,7 @@
 
 #include <xen/config.h>
 #include <xen/cache.h>
+#include <xen/sched.h>
 #include <asm/page.h>
 #include <asm/p2m.h>
 #include <public/hvm/params.h>
@@ -69,6 +70,15 @@ struct arch_domain
 
 }  __cacheline_aligned;
 
+struct vtimer {
+        struct vcpu *v;
+        int irq;
+        struct timer timer;
+        uint32_t ctl;
+        s_time_t offset;
+        s_time_t cval;
+};
+
 struct arch_vcpu
 {
     struct {
@@ -118,11 +128,6 @@ struct arch_vcpu
     uint32_t teecr, teehbr;
     uint32_t joscr, jmcr;
 
-    /* Arch timers */
-    uint64_t cntvoff;
-    uint64_t cntv_cval;
-    uint32_t cntv_ctl;
-
     /* CP 15 */
     uint32_t csselr;
 
@@ -157,12 +162,8 @@ struct arch_vcpu
         spinlock_t lock;
     } vgic;
 
-    struct {
-        struct timer timer;
-        uint32_t ctl;
-        s_time_t offset;
-        s_time_t cval;
-    } vtimer;
+    struct vtimer phys_timer;
+    struct vtimer virt_timer;
 }  __cacheline_aligned;
 
 void vcpu_show_execution_state(struct vcpu *);
diff --git a/xen/include/asm-arm/time.h b/xen/include/asm-arm/time.h
index 8cc9e78..0d4a052 100644
--- a/xen/include/asm-arm/time.h
+++ b/xen/include/asm-arm/time.h
@@ -15,6 +15,9 @@ struct tm wallclock_time(void);
 /* Set up the timer interrupt on this CPU */
 extern void __cpuinit init_timer_interrupt(void);
 
+/* Counter value at boot time */
+extern uint64_t boot_count;
+
 #endif /* __ARM_TIME_H__ */
 /*
  * Local variables:
-- 
1.7.2.5

At 16:17 +0000 on 09 Jan (1357748277), Stefano Stabellini
wrote:
> +static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs
*regs)
> +{
> +    current->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
> +    WRITE_CP32(current->arch.virt_timer.ctl | CNTx_CTL_MASK, CNTV_CTL);
This is masking the vtimer interrupt in a way that''s visible to the
currently running guest.  Is that going to confuse the guest?

When we talked about this before I had imagined that the masking would
happen in the GIC, where the guest doesn''t see it. 
> +    vgic_vcpu_inject_irq(current, irq, 1);
> +}
> +
>  /* Set up the timer interrupt on this CPU */
>  void __cpuinit init_timer_interrupt(void)
>  {
> @@ -172,6 +183,7 @@ void __cpuinit init_timer_interrupt(void)
>  
>      /* XXX Need to find this IRQ number from devicetree? */
>      request_irq(26, timer_interrupt, 0, "hyptimer", NULL);
> +    request_irq(27, vtimer_interrupt, 0, "virtimer", NULL);
>      request_irq(30, timer_interrupt, 0, "phytimer", NULL);
>  }
>  
> +static void virt_timer_expired(void *data)
> +{
> +    struct vtimer *t = data;
> +    vcpu_wake(t->v);
Shouldn''t this also inject the irq?  Otherwise when an unscheduled
guest''s timer goes off, we take two interrupts - one for the hyp timer
to call this function and then another immediately after
virt_timer_restore() which causes us to inject the vtimer irq. 
If we injected the irq here (and arranged to mask the vtimer irq) we''d
avoid a second interrupt.

Tim.

On Thu, 24 Jan 2013, Tim Deegan wrote:
> At 16:17 +0000 on 09 Jan (1357748277), Stefano Stabellini wrote:
> > +static void vtimer_interrupt(int irq, void *dev_id, struct
cpu_user_regs *regs)
> > +{
> > +    current->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
> > +    WRITE_CP32(current->arch.virt_timer.ctl | CNTx_CTL_MASK,
CNTV_CTL);
> 
> This is masking the vtimer interrupt in a way that''s visible to
the
> currently running guest.  Is that going to confuse the guest?
>
> When we talked about this before I had imagined that the masking would
> happen in the GIC, where the guest doesn''t see it. 
I know it is not ideal but it is safe, it is not creating any problems to
Linux and it is even the same thing that KVM does).
More importantly I just couldn''t get the GIC strategy to work correctly
(actually it work decently well on the physical Versatile Express but it
hangs the emulator, this tells me that it is not the way it was intended
to be used by the ARM people).

> > +    vgic_vcpu_inject_irq(current, irq, 1);
> > +}
> > +
> >  /* Set up the timer interrupt on this CPU */
> >  void __cpuinit init_timer_interrupt(void)
> >  {
> > @@ -172,6 +183,7 @@ void __cpuinit init_timer_interrupt(void)
> >  
> >      /* XXX Need to find this IRQ number from devicetree? */
> >      request_irq(26, timer_interrupt, 0, "hyptimer", NULL);
> > +    request_irq(27, vtimer_interrupt, 0, "virtimer", NULL);
> >      request_irq(30, timer_interrupt, 0, "phytimer", NULL);
> >  }
> >  
> > +static void virt_timer_expired(void *data)
> > +{
> > +    struct vtimer *t = data;
> > +    vcpu_wake(t->v);
> 
> Shouldn''t this also inject the irq?  Otherwise when an unscheduled
> guest''s timer goes off, we take two interrupts - one for the hyp
timer
> to call this function and then another immediately after
> virt_timer_restore() which causes us to inject the vtimer irq. 
> If we injected the irq here (and arranged to mask the vtimer irq)
we''d
> avoid a second interrupt.
This might be a good idea, I''ll give it a try.

At 11:46 +0000 on 14 Feb (1360842373), Stefano Stabellini
wrote:
> On Thu, 24 Jan 2013, Tim Deegan wrote:
> > At 16:17 +0000 on 09 Jan (1357748277), Stefano Stabellini wrote:
> > > +static void vtimer_interrupt(int irq, void *dev_id, struct
cpu_user_regs *regs)
> > > +{
> > > +    current->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
> > > +    WRITE_CP32(current->arch.virt_timer.ctl | CNTx_CTL_MASK,
CNTV_CTL);
> > 
> > This is masking the vtimer interrupt in a way that''s visible
to the
> > currently running guest.  Is that going to confuse the guest?
> >
> > When we talked about this before I had imagined that the masking would
> > happen in the GIC, where the guest doesn''t see it. 
> 
> I know it is not ideal but it is safe, it is not creating any problems to
> Linux and it is even the same thing that KVM does).
Grrr.  So it''s safe unless you run something other than linux, or KVM
devs change their minds...

Do we have an interface document to describe the PV machine that we
provide to a guest?  If so, this divergence from the spec should be
described there.  If not, we really should. :)

Tim.

On Thu, 14 Feb 2013, Tim Deegan wrote:
> At 11:46 +0000 on 14 Feb (1360842373), Stefano Stabellini wrote:
> > On Thu, 24 Jan 2013, Tim Deegan wrote:
> > > At 16:17 +0000 on 09 Jan (1357748277), Stefano Stabellini wrote:
> > > > +static void vtimer_interrupt(int irq, void *dev_id, struct
cpu_user_regs *regs)
> > > > +{
> > > > +    current->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
> > > > +    WRITE_CP32(current->arch.virt_timer.ctl |
CNTx_CTL_MASK, CNTV_CTL);
> > > 
> > > This is masking the vtimer interrupt in a way that''s
visible to the
> > > currently running guest.  Is that going to confuse the guest?
> > >
> > > When we talked about this before I had imagined that the masking
would
> > > happen in the GIC, where the guest doesn''t see it. 
> > 
> > I know it is not ideal but it is safe, it is not creating any problems
to
> > Linux and it is even the same thing that KVM does).
> 
> Grrr.  So it''s safe unless you run something other than linux, or
KVM
> devs change their minds...
> 
> Do we have an interface document to describe the PV machine that we
> provide to a guest?  If so, this divergence from the spec should be
> described there.  If not, we really should. :)
That''s a good idea

On Thu, 2013-02-14 at 11:46 +0000, Stefano Stabellini
wrote:
> On Thu, 24 Jan 2013, Tim Deegan wrote:
> > At 16:17 +0000 on 09 Jan (1357748277), Stefano Stabellini wrote:
> > > +static void vtimer_interrupt(int irq, void *dev_id, struct
cpu_user_regs *regs)
> > > +{
> > > +    current->arch.virt_timer.ctl = READ_CP32(CNTV_CTL);
> > > +    WRITE_CP32(current->arch.virt_timer.ctl | CNTx_CTL_MASK,
CNTV_CTL);
> > 
> > This is masking the vtimer interrupt in a way that''s visible
to the
> > currently running guest.  Is that going to confuse the guest?
> >
> > When we talked about this before I had imagined that the masking would
> > happen in the GIC, where the guest doesn''t see it. 
> 
> I know it is not ideal but it is safe, it is not creating any problems to
> Linux and it is even the same thing that KVM does).
> More importantly I just couldn''t get the GIC strategy to work
correctly
> (actually it work decently well on the physical Versatile Express but it
> hangs the emulator, this tells me that it is not the way it was intended
> to be used by the ARM people).
I think we should ask ARM about this, it could just be a model bug. I''m
sure that they would be interested in cases where the virtualness leaks
out of the hardware like this.

Ian.
> 
> 
> > > +    vgic_vcpu_inject_irq(current, irq, 1);
> > > +}
> > > +
> > >  /* Set up the timer interrupt on this CPU */
> > >  void __cpuinit init_timer_interrupt(void)
> > >  {
> > > @@ -172,6 +183,7 @@ void __cpuinit init_timer_interrupt(void)
> > >  
> > >      /* XXX Need to find this IRQ number from devicetree? */
> > >      request_irq(26, timer_interrupt, 0, "hyptimer",
NULL);
> > > +    request_irq(27, vtimer_interrupt, 0, "virtimer",
NULL);
> > >      request_irq(30, timer_interrupt, 0, "phytimer",
NULL);
> > >  }
> > >  
> > > +static void virt_timer_expired(void *data)
> > > +{
> > > +    struct vtimer *t = data;
> > > +    vcpu_wake(t->v);
> > 
> > Shouldn''t this also inject the irq?  Otherwise when an
unscheduled
> > guest''s timer goes off, we take two interrupts - one for the
hyp timer
> > to call this function and then another immediately after
> > virt_timer_restore() which causes us to inject the vtimer irq. 
> > If we injected the irq here (and arranged to mask the vtimer irq)
we''d
> > avoid a second interrupt.
> 
> This might be a good idea, I''ll give it a try.