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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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

Right, that would be nicer


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

That would be another (probably better) way of doing it: the only important
thing is that the start time on both vcpus is the same.


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

OK

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