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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 29 Apr 2013, Ian Campbell wrote:
> 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.

I don't think so: init_xen_time() is called before building dom0, so NOW()
should be returning proper values here.


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

The offset is what we use to convert DomU time to Xen time and vice
versa.
Initializing phys_timer_base.offset to NOW() means that domU's physical
timer starts counting from NOW in terms of Xen time.

Initializing virt_timer_base.offset to CNTVCT_EL0 + CNTVOFF_EL2
accomplishes the same thing for vtimer:

offset = CNTVCT_EL0 + CNTVOFF_EL2
       = Phys Count - CNTVOFF_EL2 + CNTVOFF_EL2
       = Phys Count

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

I think the reason for it is that in the old document it wasn't
explicitly stated that Phys Count is actually identical to CNTPCT.

 
> > +    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?
>

cval is the compare value, I don't think it matters what it is
initialized to. I couldn't think of a reason why it should be
initialized to NOW(), so I changed to 0.
I think it shouldn't make any differences, but you are right, this is a
semantic change, I will remove it from this patch.


> 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).

Right, it should be only code movement plus same initialization on both
vcpus.

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