Re: [Xen-devel] [PATCH v2] xen/arm: remove physical timer offset

[Julien Grall <julien@xxxxxxx>]

Hi,

On 05/12/2019 19:17, Jeff Kubascik wrote:
> On 12/3/2019 1:04 PM, Julien Grall wrote:
> > Hi,
> >
> > On 26/11/2019 21:13, Jeff Kubascik wrote:
> > > The physical timer traps apply an offset so that time starts at 0 for
> > > the guest. However, this offset is not currently applied to the physical
> > > counter. Per the ARMv8 Reference Manual (ARM DDI 0487E.a), section
> > > D11.2.4 Timers, the "Offset" between the counter and timer should be
> > > zero for a physical timer. This removes the offset to make the timer and
> > > counter consistent.
> > >
> > > Furthermore, section D11.2.4 specifies that the values in the TimerValue
> > > view of the timers are signed in standard two's complement form. When
> > > writing to the TimerValue register, it should be signed extended as
> > > described by the equation
> > >
> > >     CompareValue = (Counter[63:0] + SignExtend(TimerValue))[63:0]
> > I am a bit confused, is it a new bug introduced by the change or
> > previously existing? If the latter, then I think this should be modified
> > in a separate patch.
> This would be a previously existing bug - a quirk in the timer design that
> wasn't emulated correctly before. I can break this out into a separate patch.
It would be great if you can split it. Thank you!

[...]


> > > @@ -185,7 +184,7 @@ static bool vtimer_cntp_ctl(struct cpu_user_regs *regs, 
> > > uint32_t *r, bool read)
> > >            if ( v->arch.phys_timer.ctl & CNTx_CTL_ENABLE )
> > >            {
> > >                set_timer(&v->arch.phys_timer.timer,
> > > -                      v->arch.phys_timer.cval + 
> > > v->domain->arch.phys_timer_base.offset);
> > > +                      ticks_to_ns(v->arch.phys_timer.cval - boot_count));
> > cval may be smaller than boot_count. In that case, we will set the timer
> > to expire a very long time. This is not the expected behavior from the
> > guest.
> >
> > Instead, we should either use 0 to create the timer or call
> > phys_timer_expired directly.
> I disagree - if you set cval to a value smaller than boot_count, you are setting
> cval to a value less than the physical counter value. This would result in the
> timer having a long expiration time.
boot_count refers to when Xen began to boot, not the start of the 
physical counter. If you look at the condition to fire the timer (see 
below), then it means the timer will fire right now because the physical 
counter is past CompareValue (cval).
TimerConditionMet = (((Counter[63:0] – Offset[63:0])[63:0] - 
CompareValue[63:0]) >= 0)
We only subtract boot_count here as the timer subsystem expects a 
relative number of nanoseconds from when Xen booted.
> However, set_timer expects a signed 64 bit value in ns. The conversion of cval
> (unsigned 64 bit) from ticks to ns is going to overflow this. I'm not sure what
> would be the best way to work around this limitation. At the very least, I think
> we should print a warning message.
A warning message in emulation is definitely not the right solution. If 
a user asks something that is valid from the spec PoV then we should 
implement it correctly. The more that I don't think boot_count store 
what you expect (see above).
But we definitely can't allow the caller of ticks_to_ns() to pass a 
negative value as argument because (cval - boot_count) may be over 2^63 
for instance if the user requests a timer to be set in a million of year 
(I didn't do the math!).
Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel