| Message ID | 20191125141414.5015-1-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
| Series | mos6522: update counters when timer interrupts are off | expand |
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); > } > } >
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
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 :)
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); > } > }
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); } }
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(-)