Patchwork [06/23] PPC: Fix IPI support in MPIC

login
register
mail settings
Submitter Alexander Graf
Date July 21, 2011, 1:27 a.m.
Message ID <1311211654-14326-7-git-send-email-agraf@suse.de>
Download mbox | patch
Permalink /patch/105877/
State New
Headers show

Comments

Alexander Graf - July 21, 2011, 1:27 a.m.
The current IPI support in the MPIC code is incomplete and doesn't work. This
code adds proper support for IPIs in MPIC by using the IDE register to remember
which CPUs IPIs are still outstanding to. New triggers through the IPI trigger
register only add to the list of CPUs we want to IPI.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 hw/openpic.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)
Elie Richa - July 22, 2011, 2:08 p.m.
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?
Alexander Graf - July 22, 2011, 3:01 p.m.
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
Scott Wood - July 22, 2011, 4:37 p.m.
On Fri, 22 Jul 2011 17:01:11 +0200
Alexander Graf <agraf@suse.de> wrote:

> On 22.07.2011, at 16:08, Elie Richa wrote:
> 
> > > #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?

I don't know about early MPICs, but I don't know how you'd even define that
register for read -- it's a command, not state.

I suspect it's just a bug -- especially given the initialization to 1,
which suggests the author saw it as just another destination register.

-Scott

Patch

diff --git a/hw/openpic.c b/hw/openpic.c
index ad45331..08a3a65 100644
--- a/hw/openpic.c
+++ b/hw/openpic.c
@@ -57,7 +57,7 @@ 
 #define MAX_MBX     4
 #define MAX_TMR     4
 #define VECTOR_BITS 8
-#define MAX_IPI     0
+#define MAX_IPI     4
 
 #define VID (0x00000000)
 
@@ -840,7 +840,9 @@  static void openpic_cpu_write_internal(void *opaque, target_phys_addr_t addr,
     case 0x60:
     case 0x70:
         idx = (addr - 0x40) >> 4;
-        write_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE, val);
+        /* we use IDE as mask which CPUs to deliver the IPI to still. */
+        write_IRQreg(opp, opp->irq_ipi0 + idx, IRQ_IDE,
+                     opp->src[opp->irq_ipi0 + idx].ide | val);
         openpic_set_irq(opp, opp->irq_ipi0 + idx, 1);
         openpic_set_irq(opp, opp->irq_ipi0 + idx, 0);
         break;
@@ -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))) {
+                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 */