[Qemu-devel] Re: [PATCH] sparc32 irq clearing (guest Solaris performance+NetBSD) fix

[Blue Swirl]

On Sun, Nov 15, 2009 at 1:15 AM, Artyom Tarasenko
<address@hidden> wrote:
> 2009/11/14 Blue Swirl <address@hidden>:
>> On Sat, Nov 14, 2009 at 3:03 AM, Artyom Tarasenko
>> <address@hidden> wrote:
>>> According to NCR89C105 documentation
>>> http://www.ibiblio.org/pub/historic-linux/early-ports/Sparc/NCR/NCR89C105.txt
>>>
>>> Interrupts are cleared by disabling and then re-enabling them.
>>> This patch implements the specified behaviour. The most visible effects:
>>
>> I think the current version also implements this behaviour. The
>> difference is that now we clear on disable, with your version, the
>> interrupts are cleared when re-enabling them.
>
> Doesn't this imply that the current version does not implement this
> ("Interrupts are cleared by disabling and then re-enabling them") behavior? 
> ;-)

The specification only says that the sequence disable-enable clears
interrupts, but not which of these is true:
- clearing happens in the moment of disabling (and interrupts after
that are not cleared, current version)
- clearing happens  in the moment of re-enabling (your version, sort of)
- clearing happens in both cases (lose interrupts)

It's also interesting to think what happens between the interrupt
controller and the devices. Clearing an interrupt at controller level
does not clear the interrupt condition at the device. Aren't the
interrupts level triggered on Sparc, so the interrupt is still posted?
Is the interrupt actually masked by clearing until the level is
deactivated?

Or maybe the controller latches the interrupt so that even after the
device releases the interrupt line, interrupt is still active towards
the CPU. Then the clearing would make sense. This does not match
current code.

>> With the current version, the interrupts which arrived after getting
>> cleared during disable but before re-enabling become visible after
>> re-enabling. It looks like esp driver in Aurora 1.0, 2.0 and 2.1
>> depend on this.
>
> Hm. It certainly makes sense. But I don't see how does it agree with
> the NCR89C105 docs. Can it be the documentation is not precise?
>
>>> - NetBSD 1.3.3 - 1.5.3 boots successfully
>>
>> I didn't find those even on ftp.netbsd.org, the first I have is 1.6.
>> Thus I could not test if any of the changes I did had any positive
>> effect except if some test failed.
>
> Have you looked in the NetBSD-archive at ftp.netbsd.org?
> ftp://ftp.netbsd.org/pub/NetBSD/NetBSD-archive/

Yes, but there are no ISOs.

>>> - Solaris 2.5.1 - 7 boots ~1500 times faster (~20 seconds instead of ~8 
>>> hours)
>>>
>>> Signed-off-by: Artyom Tarasenko <address@hidden>
>>> ---
>>> diff --git a/hw/slavio_intctl.c b/hw/slavio_intctl.c
>>> index 9680392..779c661 100644
>>> --- a/hw/slavio_intctl.c
>>> +++ b/hw/slavio_intctl.c
>>> @@ -177,19 +177,19 @@ static void slavio_intctlm_mem_writel(void
>>> *opaque, target_phys_addr_t addr,
>>>     saddr = addr >> 2;
>>>     DPRINTF("write system reg 0x" TARGET_FMT_plx " = %x\n", addr, val);
>>>     switch (saddr) {
>>> -    case 2: // clear (enable)
>>> +    case 2: // clear (enable, clear formerly disabled pending)
>>>         // Force clear unused bits
>>>         val &= MASTER_IRQ_MASK;
>>> +        s->intregm_pending &= (s->intregm_disabled & val);
>>
>> This looks buggy, the AND operation will clear a lot of bits unrelated
>> to val. I think you are missing a ~ here. I tried a few combinations,
>> but none of them passed my tests.
>
> My bad. It had to be s->intregm_pending &= ~(s->intregm_disabled &
> val) . But unfortunately it doesn't work.
>
>>>         s->intregm_disabled &= ~val;
>>>         DPRINTF("Enabled master irq mask %x, curmask %x\n", val,
>>>                 s->intregm_disabled);
>>>         slavio_check_interrupts(s, 1);
>>>         break;
>>> -    case 3: // set (disable, clear pending)
>>> +    case 3: // set (disable, do not clear pending)
>>>         // Force clear unused bits
>>>         val &= MASTER_IRQ_MASK;
>>>         s->intregm_disabled |= val;
>>> -        s->intregm_pending &= ~val;
>>
>> Here
>> s->intregm_pending &= ~s->intregm_disabled;
>> would also pass my tests. Does that change anything?
>
> No, doesn't work for me either.
>
> Also I put some additional printfs into sun4m.c and see that
> interrupts are both set while being already set, and reset while not being 
> set.
> Looks like a bug?
>
> static void cpu_set_irq(void *opaque, int irq, int level)
> {
>    CPUState *env = opaque;
>
>    if (level) {
>        DPRINTF("Raise CPU IRQ %d\n", irq);
>        env->halted = 0;
>        if(env->pil_in & (1 << irq)) {printf("already set: %d\n",irq);return;}
>        env->pil_in |= 1 << irq;
>        cpu_check_irqs(env);
>    } else {
>        DPRINTF("Lower CPU IRQ %d\n", irq);
>        if(~(env->pil_in & (1 << irq))) {printf("already low:
> %d\n",irq);return;}
>        env->pil_in &= ~(1 << irq);
>        cpu_check_irqs(env);
>    }
> }
>

It should be harmless, though it may suck performance.