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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-04-26 at 16:28 +0100, Stefano Stabellini wrote:
> Introduce a domain wide vtimer initialization function to initialize
> the phys_timer and the virt_timer offsets.
> 
> Use the domain phys_timer and virt_timer offsets throughout the vtimer
> code instead of the per-vcpu offsets.
> 
> Remove the per-vcpu offsets from struct vtimer altogether.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> 
> Changes in v4:
> - introduce vcpu_domain_init;
> - inline phys_timer_base and virt_timer_base in arch_domain;
> - use phys_timer_base.offset and virt_timer_base.offset directly in
> vtimer code (remove offset field from struct vtimer).
> ---
>  xen/arch/arm/domain.c        |    3 +++
>  xen/arch/arm/vtimer.c        |   26 +++++++++++++++++---------
>  xen/arch/arm/vtimer.h        |    1 +
>  xen/include/asm-arm/domain.h |   24 +++++++++++++++---------
>  4 files changed, 36 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index f7ec979..f26222a 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -485,6 +485,9 @@ int arch_domain_create(struct domain *d, unsigned int 
> domcr_flags)
>      if ( (rc = domain_vgic_init(d)) != 0 )
>          goto fail;
>  
> +    if ( (rc = vcpu_domain_init(d)) != 0 )
> +        goto fail;
> +
>      /* Domain 0 gets a real UART not an emulated one */
>      if ( d->domain_id && (rc = domain_uart0_init(d)) != 0 )
>          goto fail;
> diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> index 393aac3..782a255 100644
> --- a/xen/arch/arm/vtimer.c
> +++ b/xen/arch/arm/vtimer.c
> @@ -44,21 +44,27 @@ static void virt_timer_expired(void *data)
>      vgic_vcpu_inject_irq(t->v, 27, 1);
>  }
>  
> +int vcpu_domain_init(struct domain *d)
> +{
> +    d->arch.phys_timer_base.offset = NOW();
> +    d->arch.virt_timer_base.offset = READ_SYSREG64(CNTVCT_EL0) +
> +                                     READ_SYSREG64(CNTVOFF_EL2);

I know you are just moving this code but I don't understand how this can
make sense.

When initialising dom0 these are, AFAICT, uninitialised. When
initialising domU these are whatever the current domain (so, dom0)
happens to have here.

Also CNTVCT == Phys Count - Virt Offset, so adding Virt offset to it
seems like an odd way to do it?

> +    return 0;
> +}
> +
>  int vcpu_vtimer_init(struct vcpu *v)
>  {
>      struct vtimer *t = &v->arch.phys_timer;
>  
>      init_timer(&t->timer, phys_timer_expired, t, v->processor);
>      t->ctl = 0;
> -    t->offset = NOW();
> -    t->cval = NOW();
> +    t->cval = 0;
>      t->irq = 30;

Why this cval change? I don't see the opposite being taken into account
so I think this is an actual semantic change unrelated to the moving of
the offset fields?

Our handling of vcpu time is IMHO pretty, well, not well understood but
other than this little change the rest of the patch seems to otherwise
fall into the "can't make anything worse" category. (I don't know how to
classify this hunk, not necessarily saying it is bad).

Ian.



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