| Message ID | 20170415192930.1443-1-bobby.prani@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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.
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
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
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
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(+)