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

[Elie Richa]

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 :)
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.

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.

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?

> +                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?