[Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Pranith Kumar]

Tested and confirmed that the stretch i386 debian qcow2 image on a
raspberry pi 2 works.

Fixes: LP#: 893208 <https://bugs.launchpad.net/qemu/+bug/893208/>
Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
---
 include/qemu/timer.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index e1742f2f3d..14c9558da4 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -1015,6 +1015,16 @@ static inline int64_t cpu_get_host_ticks(void)
     return cur - ofs;
 }
 
+#elif defined(__arm__) || defined(__aarch64__)
+
+/* ARM does not have a user-space readble cycle counter available.
+ * This is a compromise to get monotonically increasing time.
+ */
+static inline int64_t cpu_get_host_ticks(void)
+{
+    return get_clock();
+}
+
 #else
 /* The host CPU doesn't have an easily accessible cycle counter.
    Just return a monotonically increasing value.  This will be
-- 
2.11.0

Re: [Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Peter Maydell]

On 15 April 2017 at 20:29, Pranith Kumar <bobby.prani@gmail.com> wrote:
> Tested and confirmed that the stretch i386 debian qcow2 image on a
> raspberry pi 2 works.
>
> Fixes: LP#: 893208 <https://bugs.launchpad.net/qemu/+bug/893208/>
> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
> ---
>  include/qemu/timer.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index e1742f2f3d..14c9558da4 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -1015,6 +1015,16 @@ static inline int64_t cpu_get_host_ticks(void)
>      return cur - ofs;
>  }
>
> +#elif defined(__arm__) || defined(__aarch64__)
> +
> +/* ARM does not have a user-space readble cycle counter available.
> + * This is a compromise to get monotonically increasing time.
> + */
> +static inline int64_t cpu_get_host_ticks(void)
> +{
> +    return get_clock();
> +}

This doesn't look like it should be ARM-specific. Is it
better than the current default implementation? If so,
why not make this the default implementation?

> +
>  #else
>  /* The host CPU doesn't have an easily accessible cycle counter.
>     Just return a monotonically increasing value.  This will be
> --
> 2.11.0

The comment here says that our default is already a monotonically
increasing implementation -- is it wrong, or is there some other
advantage of your version?

thanks
-- PMM

Re: [Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Pranith Kumar]

On Mon, Apr 17, 2017 at 2:42 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 15 April 2017 at 20:29, Pranith Kumar <bobby.prani@gmail.com> wrote:
>> Tested and confirmed that the stretch i386 debian qcow2 image on a
>> raspberry pi 2 works.
>>
>> Fixes: LP#: 893208 <https://bugs.launchpad.net/qemu/+bug/893208/>
>> Signed-off-by: Pranith Kumar <bobby.prani@gmail.com>
>> ---
>>  include/qemu/timer.h | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
>> index e1742f2f3d..14c9558da4 100644
>> --- a/include/qemu/timer.h
>> +++ b/include/qemu/timer.h
>> @@ -1015,6 +1015,16 @@ static inline int64_t cpu_get_host_ticks(void)
>>      return cur - ofs;
>>  }
>>
>> +#elif defined(__arm__) || defined(__aarch64__)
>> +
>> +/* ARM does not have a user-space readble cycle counter available.
>> + * This is a compromise to get monotonically increasing time.
>> + */
>> +static inline int64_t cpu_get_host_ticks(void)
>> +{
>> +    return get_clock();
>> +}
>
> This doesn't look like it should be ARM-specific. Is it
> better than the current default implementation? If so,
> why not make this the default implementation?
>

I think we can do that...

>> +
>>  #else
>>  /* The host CPU doesn't have an easily accessible cycle counter.
>>     Just return a monotonically increasing value.  This will be
>
> The comment here says that our default is already a monotonically
> increasing implementation -- is it wrong, or is there some other
> advantage of your version?

Comment #6 in the bug report by Laszlo Ersek explains why.
Incrementing by 1 for approximately 55 ms is what is causing the
divide by zero in grub.

-- 
Pranith

Re: [Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Paolo Bonzini]

On 17/04/2017 20:55, Pranith Kumar wrote:
>>> +/* ARM does not have a user-space readble cycle counter available.
>>> + * This is a compromise to get monotonically increasing time.
>>> + */
>>> +static inline int64_t cpu_get_host_ticks(void)
>>> +{
>>> +    return get_clock();
>>> +}
>> This doesn't look like it should be ARM-specific. Is it
>> better than the current default implementation? If so,
>> why not make this the default implementation?
>
> I think we can do that...

Yes, it is always better for emulation accuracy.  It may be much slower,
depending on your OS (especially if get_clock requires a
user->kernel->user transition), but the current code is quite broken.

Paolo

>>> +
>>>  #else
>>>  /* The host CPU doesn't have an easily accessible cycle counter.
>>>     Just return a monotonically increasing value.  This will be
>> The comment here says that our default is already a monotonically
>> increasing implementation -- is it wrong, or is there some other
>> advantage of your version?
> Comment #6 in the bug report by Laszlo Ersek explains why.
> Incrementing by 1 for approximately 55 ms is what is causing the
> divide by zero in grub.

Re: [Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Pranith Kumar]

On Tue, Apr 18, 2017 at 5:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 17/04/2017 20:55, Pranith Kumar wrote:
>>>> +/* ARM does not have a user-space readble cycle counter available.
>>>> + * This is a compromise to get monotonically increasing time.
>>>> + */
>>>> +static inline int64_t cpu_get_host_ticks(void)
>>>> +{
>>>> +    return get_clock();
>>>> +}
>>> This doesn't look like it should be ARM-specific. Is it
>>> better than the current default implementation? If so,
>>> why not make this the default implementation?
>>
>> I think we can do that...
>
> Yes, it is always better for emulation accuracy.  It may be much slower,
> depending on your OS (especially if get_clock requires a
> user->kernel->user transition), but the current code is quite broken.
>

OK, I sent an updated patch using get_clock() for all other cases.

--
Pranith

Re: [Qemu-devel] [PATCH] timer.h: Provide monotonic time for ARM guests

[Peter Maydell]

On 18 April 2017 at 20:19, Pranith Kumar <bobby.prani@gmail.com> wrote:
> On Tue, Apr 18, 2017 at 5:56 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 17/04/2017 20:55, Pranith Kumar wrote:
>>>>> +/* ARM does not have a user-space readble cycle counter available.
>>>>> + * This is a compromise to get monotonically increasing time.
>>>>> + */
>>>>> +static inline int64_t cpu_get_host_ticks(void)
>>>>> +{
>>>>> +    return get_clock();
>>>>> +}
>>>> This doesn't look like it should be ARM-specific. Is it
>>>> better than the current default implementation? If so,
>>>> why not make this the default implementation?
>>>
>>> I think we can do that...
>>
>> Yes, it is always better for emulation accuracy.  It may be much slower,
>> depending on your OS (especially if get_clock requires a
>> user->kernel->user transition), but the current code is quite broken.
>>
>
> OK, I sent an updated patch using get_clock() for all other cases.

Thanks. As it happens I just checked against what configure
supports for host CPU architectures, and ARM is the only one
without a special-purpose cpu_get_host_ticks() (except perhaps
elderly MIPS chips).

thanks
-- PMM