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

[Jeff Kubascik <jeff.kubascik@xxxxxxxxxxxxxxx>]

On 12/5/2019 3:28 PM, Julien Grall wrote:
> 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)

This makes sense now - I wasn't considering the greater than equals condition.

> We only subtract boot_count here as the timer subsystem expects a
> relative number of nanoseconds from when Xen booted.

Makes sense.

>>
>> 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!).

Assuming 100MHz timer frequency, the math works out to be about 5,850 years,
give or take. I'm assuming we don't need to worry about rollover conditions?

> Cheers,
> 
> --
> Julien Grall
> 

Sincerely,
Jeff Kubascik

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