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.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
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;
+ if ( *r & CNTx_CTL_ENABLE )
+ *r |= v->arch.phys_timer.ctl & CNTx_CTL_PENDING;
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;
set_timer(&v->arch.phys_timer.timer,
v->arch.phys_timer.cval +
v->arch.phys_timer.offset);
}
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index fecf43b..5c4c0ca 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -79,8 +79,8 @@ struct vtimer {
int irq;
struct timer timer;
uint32_t ctl;
- s_time_t offset;
- s_time_t cval;
+ uint64_t offset;
+ uint64_t cval;
};
struct arch_vcpu
--
1.7.2.5
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? 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?> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > --- > 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; > + 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.> 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; > set_timer(&v->arch.phys_timer.timer, > v->arch.phys_timer.cval + v->arch.phys_timer.offset); > } > diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h > index fecf43b..5c4c0ca 100644 > --- a/xen/include/asm-arm/domain.h > +++ b/xen/include/asm-arm/domain.h > @@ -79,8 +79,8 @@ struct vtimer { > int irq; > struct timer timer; > uint32_t ctl; > - s_time_t offset; > - s_time_t cval; > + uint64_t offset; > + uint64_t cval; > }; > > struct arch_vcpu
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@eu.citrix.com> > > --- > > 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