Re: [Xen-devel] [PATCH v2] xen/arm: phys_timer fixes

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 20 Feb 2013, Ian Campbell wrote:
> Worth mentioning for the peanut gallery that we have a new copy of the
> ARM ARM which has none of the previous ambiguities!
> 
> On Wed, 2013-02-20 at 15:52 +0000, Stefano Stabellini wrote:
> > Do not unmask the emulated phys_timer when the related Xen timer
> > expires.
> > Do not inject the phys_timer interrupt if it is masked.
> > 
> > Do not let the user set CNTx_CTL_PENDING directly.
> > 
> > Set CNTx_CTL_PENDING when the phys_timer expires and clear it when the
> > phys_timer is disabled or the compare value is changed.
> > 
> > Define offset and cval as uint64_t given that they can't be negative and
> > they are used as uint64_t arguments.
> > 
> > 
> > Changes in v2:
> > - do not let the user set CNTx_CTL_PENDING directly;
> > - set CNTx_CTL_PENDING when the phys_timer expires and clear it when the
> > phys_timer is disabled or the compare value is changed.
> 
> I don't see the clear on disable in the patch?

I'll mark below where I am doing the bit clear


> WRT "when the compare value is changed", should it depend on what it is
> changed to? IOW if it is in the past should the timer appear to have
> fired already?

No, because if it is set in the past the internal hypervisor timer will
fire off immediately, and we set again the pending bit in the timer handler.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >  xen/arch/arm/vtimer.c        |    8 ++++++--
> >  xen/include/asm-arm/domain.h |    4 ++--
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vtimer.c b/xen/arch/arm/vtimer.c
> > index f4326f8..13b8267 100644
> > --- a/xen/arch/arm/vtimer.c
> > +++ b/xen/arch/arm/vtimer.c
> > @@ -33,8 +33,8 @@ static void phys_timer_expired(void *data)
> >  {
> >      struct vtimer *t = data;
> >      t->ctl |= CNTx_CTL_PENDING;
> > -    t->ctl &= ~CNTx_CTL_MASK;
> > -    vgic_vcpu_inject_irq(t->v, 30, 1);
> > +    if ( !(t->ctl & CNTx_CTL_MASK) )
> > +        vgic_vcpu_inject_irq(t->v, 30, 1);
> >  }
> >  
> >  static void virt_timer_expired(void *data)
> > @@ -117,6 +117,9 @@ static int vtimer_emulate_32(struct cpu_user_regs 
> > *regs, union hsr hsr)
> >          }
> >          else
> >          {
> > +            *r &= ~CNTx_CTL_PENDING;
                        ^ clear

> > +            if ( *r & CNTx_CTL_ENABLE )
> > +                *r |= v->arch.phys_timer.ctl & CNTx_CTL_PENDING;
> 
> This will end up changing whichever register the guest used for the
> write, which isn't part of the expected behaviour.

I'll need to make a local copy of that register


> >              v->arch.phys_timer.ctl = *r;
> >  
> >              if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> > @@ -141,6 +144,7 @@ static int vtimer_emulate_32(struct cpu_user_regs 
> > *regs, union hsr hsr)
> >              v->arch.phys_timer.cval = now + ticks_to_ns(*r);
> >              if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> >              {
> > +                v->arch.phys_timer.ctl &= ~CNTx_CTL_PENDING;
                                                ^ clear

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