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

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

As I say, our whole concept of virtual time is a little bit confused,
partly I think because we don't really know what the right answer is
supposed to be.

Your comment about the old docs made me go look at the new ARM --
there's a bunch of new register in there from the looks of things, 

Ian.


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