Re: [Qemu-devel] [PULL 17/28] hw/ptimer: Perform counter wrap around if timer already expired

2016-06-24 Thread Dmitry Osipenko 
On 24.06.2016 21:37, Mark Cave-Ayland wrote:
> On 24/06/16 19:19, Dmitry Osipenko wrote:
> 
>> On 24.06.2016 19:02, Peter Maydell wrote:
>>> On 24 June 2016 at 16:58, Mark Cave-Ayland
>>>  wrote:
 On 06/06/16 15:47, Peter Maydell wrote:

> From: Dmitry Osipenko 
>
> ptimer_get_count() might be called while QEMU timer already been expired.
> In that case ptimer would return counter = 0, which might be undesirable
> in case of polled timer. Do counter wrap around for periodic timer to keep
> it distributed. In order to achieve more accurate emulation behaviour of
> certain hardware, don't perform wrap around when in icount mode and return
> counter = 0 in that case (that doesn't affect polled counter
> distribution).
>
>>>
 Whilst testing Artyom's qemu-system-sparc patch today, I noticed another
 regression which I've bisected down to the above commit.

 Booting my NetBSD/OpenBSD test images with current git master causes the
 following warning to appear on the console: "WARNING: negative runtime;
 monotonic clock has gone backwards".

 Could this be a regression or does qemu-system-sparc make an incorrect
 assumption as to how the timer should work in this scenario?
>>>
>>> I'm not sure -- Dmitry ?
>>>
>>> thanks
>>> -- PMM
>>>
>>
>> The problem could be due to the IRQ being raised after the poll of the 
>> wrapped
>> around counter in non-icount mode, so CPU "sees the future". In that case 
>> patch
>> should be reverted and reworked. During the review of the patch we decided 
>> that
>> it shouldn't be an issue. I'll take a closer look at it and try to reproduce 
>> the
>> issue.
> 
> Hi Dmitry,
> 
> Thanks for looking at this. The reproducer with NetBSD is fairly easy:
> 
> ./qemu-system-sparc -cdrom NetBSD-6.1.5-sparc.iso -boot d
> 
> Hit 1 followed by Enter for the first couple of questions, and by the time you
> get asked for the terminal settings you should find that the message has been
> emitted somewhere on the console.
> 
> 
> ATB,
> 
> Mark.
> 
> (repost due to mail client crash - apologies if this is a repeat)
> 

Hi Mark,

Thanks for the guide. I successfully reproduced the issue (it's exactly what I
supposed in my previous reply) and will send fix for it shortly. Sorry for
inconvenience.

-- 
Dmitry

Re: [Qemu-devel] [PULL 17/28] hw/ptimer: Perform counter wrap around if timer already expired

2016-06-24 Thread Dmitry Osipenko 
On 24.06.2016 19:02, Peter Maydell wrote:
> On 24 June 2016 at 16:58, Mark Cave-Ayland
>  wrote:
>> On 06/06/16 15:47, Peter Maydell wrote:
>>
>>> From: Dmitry Osipenko 
>>>
>>> ptimer_get_count() might be called while QEMU timer already been expired.
>>> In that case ptimer would return counter = 0, which might be undesirable
>>> in case of polled timer. Do counter wrap around for periodic timer to keep
>>> it distributed. In order to achieve more accurate emulation behaviour of
>>> certain hardware, don't perform wrap around when in icount mode and return
>>> counter = 0 in that case (that doesn't affect polled counter
>>> distribution).
>>>
> 
>> Whilst testing Artyom's qemu-system-sparc patch today, I noticed another
>> regression which I've bisected down to the above commit.
>>
>> Booting my NetBSD/OpenBSD test images with current git master causes the
>> following warning to appear on the console: "WARNING: negative runtime;
>> monotonic clock has gone backwards".
>>
>> Could this be a regression or does qemu-system-sparc make an incorrect
>> assumption as to how the timer should work in this scenario?
> 
> I'm not sure -- Dmitry ?
> 
> thanks
> -- PMM
> 

The problem could be due to the IRQ being raised after the poll of the wrapped
around counter in non-icount mode, so CPU "sees the future". In that case patch
should be reverted and reworked. During the review of the patch we decided that
it shouldn't be an issue. I'll take a closer look at it and try to reproduce the
issue.

-- 
Dmitry

Re: [Qemu-devel] [PULL 17/28] hw/ptimer: Perform counter wrap around if timer already expired

2016-06-24 Thread Mark Cave-Ayland 

On 24/06/16 19:19, Dmitry Osipenko wrote:


On 24.06.2016 19:02, Peter Maydell wrote:

On 24 June 2016 at 16:58, Mark Cave-Ayland
 wrote:

On 06/06/16 15:47, Peter Maydell wrote:


From: Dmitry Osipenko 

ptimer_get_count() might be called while QEMU timer already been expired.
In that case ptimer would return counter = 0, which might be undesirable
in case of polled timer. Do counter wrap around for periodic timer to keep
it distributed. In order to achieve more accurate emulation behaviour of
certain hardware, don't perform wrap around when in icount mode and return
counter = 0 in that case (that doesn't affect polled counter
distribution).




Whilst testing Artyom's qemu-system-sparc patch today, I noticed another
regression which I've bisected down to the above commit.

Booting my NetBSD/OpenBSD test images with current git master causes the
following warning to appear on the console: "WARNING: negative runtime;
monotonic clock has gone backwards".

Could this be a regression or does qemu-system-sparc make an incorrect
assumption as to how the timer should work in this scenario?


I'm not sure -- Dmitry ?

thanks
-- PMM



The problem could be due to the IRQ being raised after the poll of the wrapped
around counter in non-icount mode, so CPU "sees the future". In that case patch
should be reverted and reworked. During the review of the patch we decided that
it shouldn't be an issue. I'll take a closer look at it and try to reproduce the
issue.


Hi Dmitry,

Thanks for looking at this. The reproducer with NetBSD is fairly easy:

./qemu-system-sparc -cdrom NetBSD-6.1.5-sparc.iso -boot d

Hit 1 followed by Enter for the first couple of questions, and by the 
time you get asked for the terminal settings you should find that the 
message has been emitted somewhere on the console.



ATB,

Mark.

(repost due to mail client crash - apologies if this is a repeat)

Re: [Qemu-devel] [PULL 17/28] hw/ptimer: Perform counter wrap around if timer already expired

2016-06-24 Thread Peter Maydell 
On 24 June 2016 at 16:58, Mark Cave-Ayland
 wrote:
> On 06/06/16 15:47, Peter Maydell wrote:
>
>> From: Dmitry Osipenko 
>>
>> ptimer_get_count() might be called while QEMU timer already been expired.
>> In that case ptimer would return counter = 0, which might be undesirable
>> in case of polled timer. Do counter wrap around for periodic timer to keep
>> it distributed. In order to achieve more accurate emulation behaviour of
>> certain hardware, don't perform wrap around when in icount mode and return
>> counter = 0 in that case (that doesn't affect polled counter
>> distribution).
>>

> Whilst testing Artyom's qemu-system-sparc patch today, I noticed another
> regression which I've bisected down to the above commit.
>
> Booting my NetBSD/OpenBSD test images with current git master causes the
> following warning to appear on the console: "WARNING: negative runtime;
> monotonic clock has gone backwards".
>
> Could this be a regression or does qemu-system-sparc make an incorrect
> assumption as to how the timer should work in this scenario?

I'm not sure -- Dmitry ?

thanks
-- PMM

Re: [Qemu-devel] [PULL 17/28] hw/ptimer: Perform counter wrap around if timer already expired

2016-06-24 Thread Mark Cave-Ayland 

On 06/06/16 15:47, Peter Maydell wrote:


From: Dmitry Osipenko 

ptimer_get_count() might be called while QEMU timer already been expired.
In that case ptimer would return counter = 0, which might be undesirable
in case of polled timer. Do counter wrap around for periodic timer to keep
it distributed. In order to achieve more accurate emulation behaviour of
certain hardware, don't perform wrap around when in icount mode and return
counter = 0 in that case (that doesn't affect polled counter distribution).

Signed-off-by: Dmitry Osipenko 
Reviewed-by: Peter Crosthwaite 
Message-id: 
4ce381c7d24d85d165ff251d2875d16a4b6a5c04.1464367869.git.dig...@gmail.com
Signed-off-by: Peter Maydell 
---
 hw/core/ptimer.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 16d7dd5..7e6fc2d 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -84,14 +84,16 @@ static void ptimer_tick(void *opaque)

 uint64_t ptimer_get_count(ptimer_state *s)
 {
-int64_t now;
 uint64_t counter;

 if (s->enabled) {
-now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+int64_t next = s->next_event;
+bool expired = (now - next >= 0);
+bool oneshot = (s->enabled == 2);
+
 /* Figure out the current counter value.  */
-if (now - s->next_event > 0
-|| s->period == 0) {
+if (s->period == 0 || (expired && (oneshot || use_icount))) {
 /* Prevent timer underflowing if it should already have
triggered.  */
 counter = 0;
@@ -103,7 +105,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
 uint32_t period_frac = s->period_frac;
 uint64_t period = s->period;

-if ((s->enabled == 1) && !use_icount && (s->delta * period < 
1)) {
+if (!oneshot && (s->delta * period < 1) && !use_icount) {
 period = 1 / s->delta;
 period_frac = 0;
 }
@@ -118,7 +120,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
backwards.
 */

-rem = s->next_event - now;
+rem = expired ? now - next : next - now;
 div = period;

 clz1 = clz64(rem);
@@ -138,6 +140,11 @@ uint64_t ptimer_get_count(ptimer_state *s)
 div += 1;
 }
 counter = rem / div;
+
+if (expired && counter != 0) {
+/* Wrap around periodic counter.  */
+counter = s->limit - (counter - 1) % s->limit;
+}
 }
 } else {
 counter = s->delta;



Hi Peter,

Whilst testing Artyom's qemu-system-sparc patch today, I noticed another 
regression which I've bisected down to the above commit.


Booting my NetBSD/OpenBSD test images with current git master causes the 
following warning to appear on the console: "WARNING: negative runtime; 
monotonic clock has gone backwards".


Could this be a regression or does qemu-system-sparc make an incorrect 
assumption as to how the timer should work in this scenario?



ATB,

Mark.

[Qemu-devel] [PULL 17/28] hw/ptimer: Perform counter wrap around if timer already expired

2016-06-06 Thread Peter Maydell 
From: Dmitry Osipenko 

ptimer_get_count() might be called while QEMU timer already been expired.
In that case ptimer would return counter = 0, which might be undesirable
in case of polled timer. Do counter wrap around for periodic timer to keep
it distributed. In order to achieve more accurate emulation behaviour of
certain hardware, don't perform wrap around when in icount mode and return
counter = 0 in that case (that doesn't affect polled counter distribution).

Signed-off-by: Dmitry Osipenko 
Reviewed-by: Peter Crosthwaite 
Message-id: 
4ce381c7d24d85d165ff251d2875d16a4b6a5c04.1464367869.git.dig...@gmail.com
Signed-off-by: Peter Maydell 
---
 hw/core/ptimer.c | 19 +--
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/core/ptimer.c b/hw/core/ptimer.c
index 16d7dd5..7e6fc2d 100644
--- a/hw/core/ptimer.c
+++ b/hw/core/ptimer.c
@@ -84,14 +84,16 @@ static void ptimer_tick(void *opaque)
 
 uint64_t ptimer_get_count(ptimer_state *s)
 {
-int64_t now;
 uint64_t counter;
 
 if (s->enabled) {
-now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
+int64_t next = s->next_event;
+bool expired = (now - next >= 0);
+bool oneshot = (s->enabled == 2);
+
 /* Figure out the current counter value.  */
-if (now - s->next_event > 0
-|| s->period == 0) {
+if (s->period == 0 || (expired && (oneshot || use_icount))) {
 /* Prevent timer underflowing if it should already have
triggered.  */
 counter = 0;
@@ -103,7 +105,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
 uint32_t period_frac = s->period_frac;
 uint64_t period = s->period;
 
-if ((s->enabled == 1) && !use_icount && (s->delta * period < 
1)) {
+if (!oneshot && (s->delta * period < 1) && !use_icount) {
 period = 1 / s->delta;
 period_frac = 0;
 }
@@ -118,7 +120,7 @@ uint64_t ptimer_get_count(ptimer_state *s)
backwards.
 */
 
-rem = s->next_event - now;
+rem = expired ? now - next : next - now;
 div = period;
 
 clz1 = clz64(rem);
@@ -138,6 +140,11 @@ uint64_t ptimer_get_count(ptimer_state *s)
 div += 1;
 }
 counter = rem / div;
+
+if (expired && counter != 0) {
+/* Wrap around periodic counter.  */
+counter = s->limit - (counter - 1) % s->limit;
+}
 }
 } else {
 counter = s->delta;
-- 
1.9.1