Re: [RFC 00/10] hw/mos6522: VIA timer emulation fixes and improvements

[Finn Thain]

On Fri, 10 Sep 2021, Mark Cave-Ayland wrote:

> On 01/09/2021 09:06, Mark Cave-Ayland wrote:
> 
> > I'll have a go at some basic timer measurements using your patch to 
> > see what sort of numbers I get for the latency here. Obviously QEMU 
> > doesn't guarantee response times but over 20ms does seem high.
> 
> I was able to spend some time today looking at this and came up with 
> https://github.com/mcayland/qemu/tree/via-hacks with the aim of starting 
> with your patches to reduce the calls to the timer update functions (to 
> reduce jitter) and fixing up the places where mos6522_update_irq() isn't 
> called.
> 

What kind of guest was that? What impact does jitter have on that guest? 
Was the jitter measurement increased, decreased or unchanged by this patch 
series?

> That seemed okay, but the part I'm struggling with at the moment is 
> removing the re-assertion of the timer interrupt if the timer has 
> expired when any of the registers are read i.e.
> 
> diff --git a/hw/misc/mos6522.c b/hw/misc/mos6522.c
> index 3c33cbebde..9884d7e178 100644
> --- a/hw/misc/mos6522.c
> +++ b/hw/misc/mos6522.c
> @@ -229,16 +229,7 @@ uint64_t mos6522_read(void *opaque, hwaddr addr, unsigned
> size)
>  {
>      MOS6522State *s = opaque;
>      uint32_t val;
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> 
> -    if (now >= s->timers[0].next_irq_time) {
> -        mos6522_timer1_update(s, &s->timers[0], now);
> -        s->ifr |= T1_INT;
> -    }
> -    if (now >= s->timers[1].next_irq_time) {
> -        mos6522_timer2_update(s, &s->timers[1], now);
> -        s->ifr |= T2_INT;
> -    }
>      switch (addr) {
>      case VIA_REG_B:
>          val = s->b;
> 
> If I apply this then I see a hang in about roughly 1 in 10 boots. Poking 
> it with a debugger shows that the VIA1 timer interrupts are constantly 
> being fired as IER and IFR have the timer 1 bit set, but it is being 
> filtered out by SR being set to 0x2700 on the CPU.
> 
> The existing code above isn't correct since T1_INT should only be 
> asserted by the expiry of the timer 1 via mos6522_timer1(), so I'm 
> wondering if this is tickling a CPU bug somewhere? In what circumstances 
> could interrupts be disabled via SR but not enabled again?
> 

The code you're patching here was part of Laurent's commit cd8843ff25 
("mos6522: fix T1 and T2 timers"). You've mentioned that elsewhere in this 
thread. My response is the same as before: this patch series removes that 
code so it's moot.

Please test the patch series I sent, unmodified. If you find a problem 
with my code, please do report it here. I believe that you will see no 
hangs at all.