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 Tue, 30 Apr 2013, Ian Campbell wrote:
> On Mon, 2013-04-29 at 17:14 +0100, Stefano Stabellini wrote:
> > 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.
> 
> I meant the reads of CNTVCT_EL0 and CNTVOFF_EL2, especially the second
> one which is reset to an unknown value and we don't set it.
> 
> > > 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.
> 
> So we could just read CNTPCT but don't because the docs at the time
> didn't suggest we could?

Right, I think so.


> By setting offset == phys-count we are making it so that virtual time
> for the domain appears to start from approx. 0, is that right?

Yes

> But then
> it ticks along at the same rate as phys time with no accounting for
> stolen or lost time etc? TBH I'm not even sure what stolen/lost time
> would be like for a clock which is supposed to be consistent across all
> VCPUs, or maybe that restriction is only for physical and hypervisor
> timers.

Right, no accounting. I don't know how the lost time accounting would
look like either.

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