Re: [Xen-devel] [PATCH v2] xen/arm: phys_timer fixes

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

Worth mentioning for the peanut gallery that we have a new copy of the
ARM ARM which has none of the previous ambiguities!

On Wed, 2013-02-20 at 15:52 +0000, Stefano Stabellini wrote:
> Do not unmask the emulated phys_timer when the related Xen timer
> expires.
> Do not inject the phys_timer interrupt if it is masked.
> 
> Do not let the user set CNTx_CTL_PENDING directly.
> 
> Set CNTx_CTL_PENDING when the phys_timer expires and clear it when the
> phys_timer is disabled or the compare value is changed.
> 
> Define offset and cval as uint64_t given that they can't be negative and
> they are used as uint64_t arguments.
> 
> 
> Changes in v2:
> - do not let the user set CNTx_CTL_PENDING directly;
> - set CNTx_CTL_PENDING when the phys_timer expires and clear it when the
> phys_timer is disabled or the compare value is changed.

I don't see the clear on disable in the patch?

WRT "when the compare value is changed", should it depend on what it is
changed to? IOW if it is in the past should the timer appear to have
fired already?

> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  xen/arch/arm/vtimer.c        |    8 ++++++--
>  xen/include/asm-arm/domain.h |    4 ++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index f4326f8..13b8267 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -33,8 +33,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)
> @@ -117,6 +117,9 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, 
> union hsr hsr)
>          }
>          else
>          {
> +            *r &= ~CNTx_CTL_PENDING;
> +            if ( *r & CNTx_CTL_ENABLE )
> +                *r |= v->arch.phys_timer.ctl & CNTx_CTL_PENDING;

This will end up changing whichever register the guest used for the
write, which isn't part of the expected behaviour.

>              v->arch.phys_timer.ctl = *r;
>  
>              if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> @@ -141,6 +144,7 @@ static int vtimer_emulate_32(struct cpu_user_regs *regs, 
> union hsr hsr)
>              v->arch.phys_timer.cval = now + ticks_to_ns(*r);
>              if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
>              {
> +                v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
>                  set_timer(&v->arch.phys_timer.timer,
>                            v->arch.phys_timer.cval + 
> v->arch.phys_timer.offset);
>              }
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index fecf43b..5c4c0ca 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -79,8 +79,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



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel