[PATCH] xen: arm: vtimer fixes for arm64

[PATCH] xen: arm: vtimer fixes for arm64

[Ian Campbell]

The code was writing back the register, even for writes and didn't implement
CNTPCT at all.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/vtimer.c |   51 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 36 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
index f323453..dcda2b2 100644
--- a/xen/arch/arm/vtimer.c
+++ b/xen/arch/arm/vtimer.c
@@ -165,6 +165,27 @@ static void vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
     }
 }
 
+static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
+{
+    struct vcpu *v = current;
+    uint64_t ticks;
+    s_time_t now;
+
+    if ( read )
+    {
+        now = NOW() - v->domain->arch.phys_timer_base.offset;
+        ticks = ns_to_ticks(now);
+        *r = ticks;
+        return 1;
+    }
+    else
+    {
+        gdprintk(XENLOG_DEBUG, "READ from R/O CNTPCT\n");
+        return 0;
+    }
+}
+
+
 static int vtimer_emulate_cp32(struct cpu_user_regs *regs, union hsr hsr)
 {
     struct hsr_cp32 cp32 = hsr.cp32;
@@ -187,29 +208,23 @@ static int vtimer_emulate_cp32(struct cpu_user_regs *regs, union hsr hsr)
 
 static int vtimer_emulate_cp64(struct cpu_user_regs *regs, union hsr hsr)
 {
-    struct vcpu *v = current;
     struct hsr_cp64 cp64 = hsr.cp64;
     uint32_t *r1 = (uint32_t *)select_user_reg(regs, cp64.reg1);
     uint32_t *r2 = (uint32_t *)select_user_reg(regs, cp64.reg2);
-    uint64_t ticks;
-    s_time_t now;
+    uint64_t x;
 
     switch ( hsr.bits & HSR_CP64_REGS_MASK )
     {
     case HSR_CPREG64(CNTPCT):
+        if (!vtimer_cntpct(regs, &x, cp64.read))
+            return 0;
+
         if ( cp64.read )
         {
-            now = NOW() - v->domain->arch.phys_timer_base.offset;
-            ticks = ns_to_ticks(now);
-            *r1 = (uint32_t)(ticks & 0xffffffff);
-            *r2 = (uint32_t)(ticks >> 32);
-            return 1;
-        }
-        else
-        {
-            printk("READ from R/O CNTPCT\n");
-            return 0;
+            *r1 = (uint32_t)(x & 0xffffffff);
+            *r2 = (uint32_t)(x >> 32);
         }
+        return 1;
 
     default:
         return 0;
@@ -227,12 +242,18 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
     {
     case CNTP_CTL_EL0:
         vtimer_cntp_ctl(regs, &r, sysreg.read);
-        *x = r;
+        if ( sysreg.read )
+            *x = r;
         return 1;
     case CNTP_TVAL_EL0:
         vtimer_cntp_tval(regs, &r, sysreg.read);
-        *x = r;
+        if ( sysreg.read )
+            *x = r;
         return 1;
+
+    case HSR_CPREG64(CNTPCT):
+        return vtimer_cntpct(regs, x, sysreg.read);
+
     default:
         return 0;
     }
-- 
1.7.10.4

Re: [PATCH] xen: arm: vtimer fixes for arm64

[Julien Grall]

On 12/05/2013 12:58 PM, Ian Campbell wrote:
> The code was writing back the register, even for writes and didn't implement
> CNTPCT at all.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>   xen/arch/arm/vtimer.c |   51 ++++++++++++++++++++++++++++++++++---------------
>   1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index f323453..dcda2b2 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -165,6 +165,27 @@ static void vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
>       }
>   }
>
> +static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
> +{
> +    struct vcpu *v = current;
> +    uint64_t ticks;
> +    s_time_t now;
> +
> +    if ( read )
> +    {
> +        now = NOW() - v->domain->arch.phys_timer_base.offset;
> +        ticks = ns_to_ticks(now);
> +        *r = ticks;
> +        return 1;
> +    }
> +    else
> +    {
> +        gdprintk(XENLOG_DEBUG, "READ from R/O CNTPCT\n");

s/READ/WRITE/ ?

Except this minor typo:

Acked-by: Julien Grall <julien.grall@linaro.org>

-- 
Julien Grall

Re: [PATCH] xen: arm: vtimer fixes for arm64

[Ian Campbell]

On Fri, 2013-12-06 at 16:30 +0000, Julien Grall wrote:
> 
> On 12/05/2013 12:58 PM, Ian Campbell wrote:
> > The code was writing back the register, even for writes and didn't implement
> > CNTPCT at all.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >   xen/arch/arm/vtimer.c |   51 ++++++++++++++++++++++++++++++++++---------------
> >   1 file changed, 36 insertions(+), 15 deletions(-)
> >
> > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > index f323453..dcda2b2 100644
> > --- a/xen/arch/arm/vtimer.c
> > +++ b/xen/arch/arm/vtimer.c
> > @@ -165,6 +165,27 @@ static void vtimer_cntp_tval(struct cpu_user_regs *regs, uint32_t *r, int read)
> >       }
> >   }
> >
> > +static int vtimer_cntpct(struct cpu_user_regs *regs, uint64_t *r, int read)
> > +{
> > +    struct vcpu *v = current;
> > +    uint64_t ticks;
> > +    s_time_t now;
> > +
> > +    if ( read )
> > +    {
> > +        now = NOW() - v->domain->arch.phys_timer_base.offset;
> > +        ticks = ns_to_ticks(now);
> > +        *r = ticks;
> > +        return 1;
> > +    }
> > +    else
> > +    {
> > +        gdprintk(XENLOG_DEBUG, "READ from R/O CNTPCT\n");
> 
> s/READ/WRITE/ ?

Yeah, copied blindly from the existing wrong code!

> Except this minor typo:
> 
> Acked-by: Julien Grall <julien.grall@linaro.org>

Thanks.

Re: [PATCH] xen: arm: vtimer fixes for arm64

[Stefano Stabellini]

On Thu, 5 Dec 2013, Ian Campbell wrote:
> @@ -227,12 +242,18 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
>      {
>      case CNTP_CTL_EL0:
>          vtimer_cntp_ctl(regs, &r, sysreg.read);
> -        *x = r;
> +        if ( sysreg.read )
> +            *x = r;
>          return 1;
>      case CNTP_TVAL_EL0:
>          vtimer_cntp_tval(regs, &r, sysreg.read);
> -        *x = r;
> +        if ( sysreg.read )
> +            *x = r;
>          return 1;

Are you sure that sysreg.read == 1 means write?
>From xen/arch/arm/traps.c:do_sysreg:

        printk("%s %d, %d, c%d, c%d, %d %s x%d @ 0x%"PRIregister"\n",
               sysreg.read ? "mrs" : "msr",

sysreg.read == 1 -> mrs that is a read from special register.

Re: [PATCH] xen: arm: vtimer fixes for arm64

[Ian Campbell]

On Fri, 2013-12-06 at 17:38 +0000, Stefano Stabellini wrote:
> On Thu, 5 Dec 2013, Ian Campbell wrote:
> > @@ -227,12 +242,18 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
> >      {
> >      case CNTP_CTL_EL0:
> >          vtimer_cntp_ctl(regs, &r, sysreg.read);
> > -        *x = r;
> > +        if ( sysreg.read )
> > +            *x = r;
> >          return 1;
> >      case CNTP_TVAL_EL0:
> >          vtimer_cntp_tval(regs, &r, sysreg.read);
> > -        *x = r;
> > +        if ( sysreg.read )
> > +            *x = r;
> >          return 1;
> 
> Are you sure that sysreg.read == 1 means write?

On the contrary, it means read.

Here x points to the guest reg that is the target of the read, and r is
the new value which the emulation has produced.

So *x = r is implementing a read from the guest's PoV.

Ian.

Re: [PATCH] xen: arm: vtimer fixes for arm64

[Stefano Stabellini]

On Fri, 6 Dec 2013, Ian Campbell wrote:
> On Fri, 2013-12-06 at 17:38 +0000, Stefano Stabellini wrote:
> > On Thu, 5 Dec 2013, Ian Campbell wrote:
> > > @@ -227,12 +242,18 @@ static int vtimer_emulate_sysreg(struct cpu_user_regs *regs, union hsr hsr)
> > >      {
> > >      case CNTP_CTL_EL0:
> > >          vtimer_cntp_ctl(regs, &r, sysreg.read);
> > > -        *x = r;
> > > +        if ( sysreg.read )
> > > +            *x = r;
> > >          return 1;
> > >      case CNTP_TVAL_EL0:
> > >          vtimer_cntp_tval(regs, &r, sysreg.read);
> > > -        *x = r;
> > > +        if ( sysreg.read )
> > > +            *x = r;
> > >          return 1;
> > 
> > Are you sure that sysreg.read == 1 means write?
> 
> On the contrary, it means read.
> 
> Here x points to the guest reg that is the target of the read, and r is
> the new value which the emulation has produced.
> 
> So *x = r is implementing a read from the guest's PoV.

Ops, that's right.

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>