mos6522: update counters when timer interrupts are off

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

Posted by Laurent Vivier 5 years, 11 months ago

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

Posted by Philippe Mathieu-Daudé 5 years, 11 months ago

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

Posted by Laurent Vivier 5 years, 11 months ago

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

Posted by Philippe Mathieu-Daudé 5 years, 11 months ago

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

Posted by David Gibson 5 years, 11 months ago

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