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

[Artyom Tarasenko]

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? ;-)

> 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/

>> - 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);
    }
}