From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <Ian.Campbell@citrix.com>
Subject: Re: [PATCH 7/7] xen/arm: phys_timer fixes
Date: Fri, 15 Feb 2013 13:04:48 +0000
Message-ID: <1360933488.31407.35.camel@zakaz.uk.xensource.com>
References: <1360859833-16498-7-git-send-email-stefano.stabellini@eu.citrix.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1360859833-16498-7-git-send-email-stefano.stabellini@eu.citrix.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, "Tim
	(Xen.org)" <tim@xen.org>
List-Id: xen-devel@lists.xenproject.org

> @@ -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);