Re: [Qemu-devel] [PATCH 06/23] PPC: Fix IPI support in MPIC

[Alexander Graf]

Hi :)

On 22.07.2011, at 16:08, Elie Richa wrote:

> Hello.
> 
> I have been working on SMP support for the MPC8641D processor, so I have also 
> worked on completing the SMP implementation of MPIC. I've been meaning to 
> post a patch, but you beat me to it :)

Ah, very nice! It's great to see more people interested in this.

> I compared your implementation to mine, and it boils down to the same, except 
> that I had overlooked the problem of IPIs when multiple CPUs are targeted.

That was the really hard part. Took me 2 days to figure out how to route those 
properly internally :(.

> I did some IPI tests with your code and it works fine generally, but some 
> problems came up. It appears that the first time I trigger an IPI, CPU 0 
> always receives the IPI even if it wasn't targeted. The problem stops 
> appearing after the first IPI.
> 
> It turns out that all IDEs are initialized to 0x1, which is forcing CPU 0 to 
> receive the first IPI. IPI related IDEs should therefore be initialized to 0 
> in mpic_reset.

Nice catch! You're right - we should just initialize the IPI IDE registers to 0.

> 
> And I also have other comments. Some of them are below, and some others will 
> come in a reply to another patch.
> 
> Elie
> 
> On 07/21/2011 03:27 AM, Alexander Graf wrote:
>> @@ -934,6 +936,17 @@ static uint32_t openpic_cpu_read_internal(void *opaque, 
>> target_phys_addr_t addr,
>>                  reset_bit(&src->ipvp, IPVP_ACTIVITY);
>>                  src->pending = 0;
>>              }
>> +
>> +            if ((n_IRQ>= opp->irq_ipi0)&&   (n_IRQ<  (opp->irq_ipi0 + 4))) {
> 
> I think using (opp->irq_ipi0 + MAX_IPI) would be better?

Yes, totally. Must have overlooked this :).

> 
>> +                src->ide&= ~(1<<  idx);
>> +                if (src->ide&&  !test_bit(&src->ipvp, IPVP_SENSE)) {
>> +                    /* trigger on CPUs that didn't know about it yet */
>> +                    openpic_set_irq(opp, n_IRQ, 1);
>> +                    openpic_set_irq(opp, n_IRQ, 0);
>> +                    /* if all CPUs knew about it, set active bit again */
>> +                    set_bit(&src->ipvp, IPVP_ACTIVITY);
>> +                }
>> +            }
>>          }
>>          break;
>>      case 0xB0: /* PEOI */
> 
> Also, in openpic_cpu_read_internal() , there's is the following code :
> 
> > #if MAX_IPI > 0
> >     case 0x40: /* IDE */
> >     case 0x50:
> >         idx = (addr - 0x40) >> 4;
> >         retval = read_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE);
> >         break;
> > #endif
> 
> These are the IPI dispatch registers which are write only, so I suppose this 
> shouldn't be here right?

The code was there long before me :). No idea why it is there though - it tries 
to read out the IDE register for 2 IPIs. Maybe it was read-write in early 
versions of MPIC? Scott, any idea?


Alex