Re: [Xen-devel] [PATCH v3 13/13] xen/arm: initialize virt_timer and phys_timer with the same values on all vcpus

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-04-24 at 20:07 +0100, Stefano Stabellini wrote:
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  xen/arch/arm/vtimer.c        |   18 ++++++++++++++----
>  xen/include/asm-arm/domain.h |   26 +++++++++++++++++---------
>  2 files changed, 31 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 2444851..49858e7 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -46,20 +46,30 @@ static void virt_timer_expired(void *data)
>  
>  int vcpu_vtimer_init(struct vcpu *v)
>  {
> +    struct vtimer_base *b = &v->domain->arch.phys_timer_base;
>      struct vtimer *t = &v->arch.phys_timer;
>  
> +    if ( !b->offset )
> +        b->offset = NOW();
> +    if ( !b->cval )
> +        b->cval = NOW();

Initialisation of domain global state should be done in
domain_vtimer_init (which you may need to add) IMHO.

>      init_timer(&t->timer, phys_timer_expired, t, v->processor);
>      t->ctl = 0;
> -    t->offset = NOW();
> -    t->cval = NOW();
> +    t->offset = b->offset;
>
> +    t->cval = b->cval;
>      t->irq = 30;
>      t->v = v;
>  
> +    b = &v->domain->arch.virt_timer_base;
> +    if ( !b->offset )
> +        b->offset = READ_SYSREG64(CNTVCT_EL0) + READ_SYSREG64(CNTVOFF_EL2);
> +    if ( !b->cval )
> +        b->cval = 0;
>      t = &v->arch.virt_timer;
>      init_timer(&t->timer, virt_timer_expired, t, v->processor);
>      t->ctl = 0;
> -    t->offset = READ_SYSREG64(CNTVCT_EL0) + READ_SYSREG64(CNTVOFF_EL2);
> -    t->cval = 0;
> +    t->offset = b->offset;
> +    t->cval = b->cval;

Are you sure this is as simple as this?

At the very least I'm surprised that we don't need to consult b->offset
anywhere other than at init time, if vcpus are supposed to see a uniform
view of time.

Or is it the case that what actually needs to happen is that t->offset
goes away and b->offset is used everywhere?


>      t->irq = 27;
>      t->v = v;
>  
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index 3fa266c2..d001802 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -47,6 +47,20 @@ enum domain_type {
>  #define is_pv64_domain(d) (0)
>  #endif
>  
> +struct vtimer {
> +        struct vcpu *v;
> +        int irq;
> +        struct timer timer;
> +        uint32_t ctl;
> +        uint64_t offset;
> +        uint64_t cval;
> +};
> +
> +struct vtimer_base {
> +        uint64_t offset;
> +        uint64_t cval;
> +};

It would be consistent with what is there to inline this into
arch_domain.

> +
>  struct arch_domain
>  {
>  #ifdef CONFIG_ARM_64
> @@ -61,6 +75,9 @@ struct arch_domain
>      uint32_t vpidr;
>      register_t vmpidr;
>  
> +    struct vtimer_base phys_timer_base;
> +    struct vtimer_base virt_timer_base;
> +
>      struct {
>          /*
>           * Covers access to other members of this struct _except_ for
> @@ -91,15 +108,6 @@ struct arch_domain
>  
>  }  __cacheline_aligned;
>  
> -struct vtimer {
> -        struct vcpu *v;
> -        int irq;
> -        struct timer timer;
> -        uint32_t ctl;
> -        uint64_t offset;
> -        uint64_t cval;
> -};
> -
>  struct arch_vcpu
>  {
>      struct {



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