[PATCH] mos6522: update counters when timer interrupts are off

[PATCH] mos6522: update counters when timer interrupts are off

[Laurent Vivier]

Even if the interrupts are off, counters must be updated because
they are running anyway and kernel can try to read them
(it's the case with g3beige kernel).

Reported-by: Andrew Randrianasulu <randrianasulu@gmail.com>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/misc/mos6522.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
index aa3bfe1afd..cecf0be59e 100644
--- a/hw/misc/mos6522.c
+++ b/hw/misc/mos6522.c
@@ -113,6 +113,10 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
     int64_t d, next_time;
     unsigned int counter;
 
+    if (ti->frequency == 0) {
+        return INT64_MAX;
+    }
+
     /* current counter value */
     d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
                  ti->frequency, NANOSECONDS_PER_SECOND);
@@ -149,10 +153,10 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
     if (!ti->timer) {
         return;
     }
+    ti->next_irq_time = get_next_irq_time(s, ti, current_time);
     if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
         timer_del(ti->timer);
     } else {
-        ti->next_irq_time = get_next_irq_time(s, ti, current_time);
         timer_mod(ti->timer, ti->next_irq_time);
     }
 }
@@ -163,10 +167,10 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
     if (!ti->timer) {
         return;
     }
+    ti->next_irq_time = get_next_irq_time(s, ti, current_time);
     if ((s->ier & T2_INT) == 0) {
         timer_del(ti->timer);
     } else {
-        ti->next_irq_time = get_next_irq_time(s, ti, current_time);
         timer_mod(ti->timer, ti->next_irq_time);
     }
 }
-- 
2.21.0

Re: [PATCH] mos6522: update counters when timer interrupts are off

[Philippe Mathieu-Daudé]

On 11/25/19 3:14 PM, Laurent Vivier wrote:
> Even if the interrupts are off, counters must be updated because
> they are running anyway and kernel can try to read them
> (it's the case with g3beige kernel).
> 
> Reported-by: Andrew Randrianasulu <randrianasulu@gmail.com>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/misc/mos6522.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index aa3bfe1afd..cecf0be59e 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -113,6 +113,10 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
>       int64_t d, next_time;
>       unsigned int counter;
>   

Can you add a comment here such "Clock disabled. This is the longest 
time before expiration" or better?

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> +    if (ti->frequency == 0) {
> +        return INT64_MAX;
> +    }
> +
>       /* current counter value */
>       d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
>                    ti->frequency, NANOSECONDS_PER_SECOND);
> @@ -149,10 +153,10 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
>       if (!ti->timer) {
>           return;
>       }
> +    ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>       if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
>           timer_del(ti->timer);
>       } else {
> -        ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>           timer_mod(ti->timer, ti->next_irq_time);
>       }
>   }
> @@ -163,10 +167,10 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
>       if (!ti->timer) {
>           return;
>       }
> +    ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>       if ((s->ier & T2_INT) == 0) {
>           timer_del(ti->timer);
>       } else {
> -        ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>           timer_mod(ti->timer, ti->next_irq_time);
>       }
>   }
> 

Re: [PATCH] mos6522: update counters when timer interrupts are off

[Laurent Vivier]

Le 25/11/2019 à 15:37, Philippe Mathieu-Daudé a écrit :
> On 11/25/19 3:14 PM, Laurent Vivier wrote:
>> Even if the interrupts are off, counters must be updated because
>> they are running anyway and kernel can try to read them
>> (it's the case with g3beige kernel).
>>
>> Reported-by: Andrew Randrianasulu <randrianasulu@gmail.com>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/misc/mos6522.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>> index aa3bfe1afd..cecf0be59e 100644
>> --- a/hw/misc/mos6522.c
>> +++ b/hw/misc/mos6522.c
>> @@ -113,6 +113,10 @@ static int64_t get_next_irq_time(MOS6522State *s,
>> MOS6522Timer *ti,
>>       int64_t d, next_time;
>>       unsigned int counter;
>>   
> 
> Can you add a comment here such "Clock disabled. This is the longest
> time before expiration" or better?
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> 
>> +    if (ti->frequency == 0) {
>> +        return INT64_MAX;
>> +    }
>> +


In fact this is here for a deeper problem:

frequency is not correctly initialized on reset.

ti->frequency are initialized by cuda/pmu/mac_via after the parent reset
(mos6522) but the parent reset calls set_counter() that uses
ti->frequency to set the counters. The mos6522 reset initialize the
ti->frequency from s->frequency but s->frequency is never initialized.

It was hidden before because the timers were not updated if the
interrupts are disabled, and now they are always updated.

I didn't want to add a such complicated comment in the code and I will
try to fix the problem later.

Thanks,
Laurent

Re: [PATCH] mos6522: update counters when timer interrupts are off

[Philippe Mathieu-Daudé]

On 11/25/19 3:56 PM, Laurent Vivier wrote:
> Le 25/11/2019 à 15:37, Philippe Mathieu-Daudé a écrit :
>> On 11/25/19 3:14 PM, Laurent Vivier wrote:
>>> Even if the interrupts are off, counters must be updated because
>>> they are running anyway and kernel can try to read them
>>> (it's the case with g3beige kernel).
>>>
>>> Reported-by: Andrew Randrianasulu <randrianasulu@gmail.com>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>    hw/misc/mos6522.c | 8 ++++++--
>>>    1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
>>> index aa3bfe1afd..cecf0be59e 100644
>>> --- a/hw/misc/mos6522.c
>>> +++ b/hw/misc/mos6522.c
>>> @@ -113,6 +113,10 @@ static int64_t get_next_irq_time(MOS6522State *s,
>>> MOS6522Timer *ti,
>>>        int64_t d, next_time;
>>>        unsigned int counter;
>>>    
>>
>> Can you add a comment here such "Clock disabled. This is the longest
>> time before expiration" or better?
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>
>>> +    if (ti->frequency == 0) {
>>> +        return INT64_MAX;
>>> +    }
>>> +
> 
> 
> In fact this is here for a deeper problem:
> 
> frequency is not correctly initialized on reset.
> 
> ti->frequency are initialized by cuda/pmu/mac_via after the parent reset
> (mos6522) but the parent reset calls set_counter() that uses
> ti->frequency to set the counters. The mos6522 reset initialize the
> ti->frequency from s->frequency but s->frequency is never initialized.

Ah, I see.

"How machines behave after a soft reset" is something I'd like to get 
tested more thoroughly (with Avocado eventually).

> It was hidden before because the timers were not updated if the
> interrupts are disabled, and now they are always updated.
> 
> I didn't want to add a such complicated comment in the code and I will
> try to fix the problem later.

Good :)

Re: [PATCH] mos6522: update counters when timer interrupts are off

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2171 bytes --]

On Mon, Nov 25, 2019 at 03:14:14PM +0100, Laurent Vivier wrote:
> Even if the interrupts are off, counters must be updated because
> they are running anyway and kernel can try to read them
> (it's the case with g3beige kernel).
> 
> Reported-by: Andrew Randrianasulu <randrianasulu@gmail.com>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Applied to ppc-for-4.2, thanks.

> ---
>  hw/misc/mos6522.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index aa3bfe1afd..cecf0be59e 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -113,6 +113,10 @@ static int64_t get_next_irq_time(MOS6522State *s, MOS6522Timer *ti,
>      int64_t d, next_time;
>      unsigned int counter;
>  
> +    if (ti->frequency == 0) {
> +        return INT64_MAX;
> +    }
> +
>      /* current counter value */
>      d = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) - ti->load_time,
>                   ti->frequency, NANOSECONDS_PER_SECOND);
> @@ -149,10 +153,10 @@ static void mos6522_timer1_update(MOS6522State *s, MOS6522Timer *ti,
>      if (!ti->timer) {
>          return;
>      }
> +    ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>      if ((s->ier & T1_INT) == 0 || (s->acr & T1MODE) != T1MODE_CONT) {
>          timer_del(ti->timer);
>      } else {
> -        ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>          timer_mod(ti->timer, ti->next_irq_time);
>      }
>  }
> @@ -163,10 +167,10 @@ static void mos6522_timer2_update(MOS6522State *s, MOS6522Timer *ti,
>      if (!ti->timer) {
>          return;
>      }
> +    ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>      if ((s->ier & T2_INT) == 0) {
>          timer_del(ti->timer);
>      } else {
> -        ti->next_irq_time = get_next_irq_time(s, ti, current_time);
>          timer_mod(ti->timer, ti->next_irq_time);
>      }
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]