Re: [PATCH 2/16] add hypervisor support for SPU

Re: [PATCH 2/16] add hypervisor support for SPU

[Ishizaki Kou]

Hi Arnd-san,

> On Wednesday 15 November 2006 10:28, Ishizaki Kou wrote:
> 
> > +static int __init find_spu_unit_number(struct device_node *spe)
> > +{
> > +       const unsigned int *reg;
> > +       reg = get_property(spe, "reg", NULL);
> > +       return reg ? *reg : 0;
> > +}
> > +
> 
> The unit number shold not be in the 'reg' property any more. With
> the device tree layout we want to migrate to for better OFW compatibility,
> 'reg' should contain the addresses of local store, problem state and priv2
> mappings, in that order.
> 
> Having the function is fine, but your interface needs to be a different
> property.

We don't think that standard binding for SPE exists. Do you have any
proposal? If you have, please send us your binding.

On our device tree, property "reg" has unit number. Addresses are in
property "priv2", "problem" and "local-store". Because of LPAR
environment, property "priv1" does not exist.


> > @@ -805,7 +816,11 @@
> >         if (ret)
> >                 goto out_unmap;
> >         spin_lock_init(&spu->register_lock);
> > -       spu_mfc_sdr_set(spu, mfspr(SPRN_SDR1));
> > +#ifdef CONFIG_SPU_NEED_SHADOW_INT_MASK
> > +       spin_lock_init(&spu->int_mask_lock);
> > +#endif
> > +       if (!(firmware_has_feature(FW_FEATURE_LPAR)))
> > +               spu_mfc_sdr_set(spu, mfspr(SPRN_SDR1));
> >         spu_mfc_sr1_set(spu, 0x33);
> > ...
> >         u32 node;
> > +       u32 unit_number;
> >         u64 flags;
> >         u64 dar;
> >         u64 dsisr;
> > +#ifdef CONFIG_SPU_NEED_SHADOW_INT_MASK
> > +       spinlock_t int_mask_lock;
> > +       u64 shadow_int_mask_RW[3];
> > +#endif
> >         size_t ls_size;
> >         unsigned int slb_replace;
> >         struct mm_struct *mm;
> 
> Not sure what others think about this, but I'd prefer not to have the
> #ifdef here at all. This part is not a fast path, so we can spend the
> few extra bytes and cycles on keeping it in the kernel always.
> 
> The place where it is used should still have a run-time check for whether
> it's running on BEAT.

When Geoff-san's "abstract spu management routines" patch is accepted,
we have to rewrite this code to meet with that patch.
These data and initializer will be moved to platform-dependent files.

Thank you,
Kou Ishizaki
Toshiba

Re: [PATCH 2/16] add hypervisor support for SPU

[Arnd Bergmann]

On Friday 17 November 2006 11:33, Ishizaki Kou wrote:
> We don't think that standard binding for SPE exists. Do you have any
> proposal? If you have, please send us your binding.
>
> On our device tree, property "reg" has unit number. Addresses are in
> property "priv2", "problem" and "local-store". Because of LPAR
> environment, property "priv1" does not exist.

Unfortunately, we don't have any official binding documents for this,
but you can look at how both methods are used in the source code now.

There should not be any priv2, problem or local-store properties
any more. We have chosen the order of these register areas in the
"reg" property to be "local-store", "problem", "priv2", "priv1",
where the last one is optional so you don't need to provide one
in a hypervisor.

Similarly, there should no longer be a single "isrc" property,
but rather a standard "interrupts" property with three values
that can be mapped to an interrupt number each, for class
0, 1 and 2 interrupts.

	Arnd <><

Re: [PATCH 2/16] add hypervisor support for SPU

[Ishizaki Kou]

Arnd-san,

> On Friday 17 November 2006 11:33, Ishizaki Kou wrote:
> > We don't think that standard binding for SPE exists. Do you have any
> > proposal? If you have, please send us your binding.
> >
> > On our device tree, property "reg" has unit number. Addresses are in
> > property "priv2", "problem" and "local-store". Because of LPAR
> > environment, property "priv1" does not exist.
> 
> Unfortunately, we don't have any official binding documents for this,
> but you can look at how both methods are used in the source code now.
> 
> There should not be any priv2, problem or local-store properties
> any more. We have chosen the order of these register areas in the
> "reg" property to be "local-store", "problem", "priv2", "priv1",
> where the last one is optional so you don't need to provide one
> in a hypervisor.
> 
> Similarly, there should no longer be a single "isrc" property,
> but rather a standard "interrupts" property with three values
> that can be mapped to an interrupt number each, for class
> 0, 1 and 2 interrupts.

Please tell us a little more.

Where is unit number on your device-tree?
What encoding is used for it?
How many cells does it use?


Thank you,
Kou Ishizaki
Toshiba

[PATCH 2/16] add hypervisor support for SPU

[Ishizaki Kou]

This patch adds hypervisor support for spu.

Linux(GuestOS) needs to designate SPE ID number when it uses SPE on Beat,
and must read the number from DT.

Signed-off-by: Kou Ishizaki <kou.ishizaki@toshiba.co.jp>
---

Index: linux-2.6.19/arch/powerpc/platforms/cell/Kconfig
diff -u linux-2.6.19/arch/powerpc/platforms/cell/Kconfig:1.1.1.1 linux-2.6.19/arch/powerpc/platforms/cell/Kconfig:1.2
--- linux-2.6.19/arch/powerpc/platforms/cell/Kconfig:1.1.1.1	Fri Oct  6 10:40:07 2006
+++ linux-2.6.19/arch/powerpc/platforms/cell/Kconfig	Fri Oct  6 12:26:35 2006
@@ -16,6 +16,10 @@
 	bool
 	default n
 
+config SPU_NEED_SHADOW_INT_MASK
+	bool
+	default n
+
 config CBE_RAS
 	bool "RAS features for bare metal Cell BE"
 	default y
Index: linux-2.6.19/arch/powerpc/platforms/cell/spu_base.c
diff -u linux-2.6.19/arch/powerpc/platforms/cell/spu_base.c:1.1.1.3 linux-2.6.19/arch/powerpc/platforms/cell/spu_base.c:1.6
--- linux-2.6.19/arch/powerpc/platforms/cell/spu_base.c:1.1.1.3	Tue Oct 24 13:35:57 2006
+++ linux-2.6.19/arch/powerpc/platforms/cell/spu_base.c	Tue Nov  7 12:44:38 2006
@@ -38,6 +38,7 @@
 #include <asm/spu.h>
 #include <asm/spu_priv1.h>
 #include <asm/mmu_context.h>
+#include <asm/firmware.h>
 
 #include "interrupt.h"
 
@@ -507,6 +508,13 @@
 	return id ? *id : 0;
 }
 
+static int __init find_spu_unit_number(struct device_node *spe)
+{
+	const unsigned int *reg;
+	reg = get_property(spe, "reg", NULL);
+	return reg ? *reg : 0;
+}
+
 static int __init cell_spuprop_present(struct spu *spu, struct device_node *spe,
 		const char *prop)
 {
@@ -554,10 +562,12 @@
 	int err = 0;
 
 	p = get_property(n, name, &proplen);
-	if (proplen != sizeof (struct address_prop))
+	if (!p || proplen != sizeof (struct address_prop))
 		return NULL;
 
 	prop = p;
+	if (prop->len == 0)
+		return NULL;
 
 	err = cell_spuprop_present(spu, n, name);
 	if (err && (err != -EEXIST))
@@ -788,6 +798,7 @@
 		printk(KERN_WARNING "Check if CONFIG_NUMA is enabled.\n");
 		return -ENODEV;
 	}
+	spu->unit_number = find_spu_unit_number(spe);
 	spu->nid = of_node_to_nid(spe);
 	if (spu->nid == -1)
 		spu->nid = 0;
@@ -805,7 +816,11 @@
 	if (ret)
 		goto out_unmap;
 	spin_lock_init(&spu->register_lock);
-	spu_mfc_sdr_set(spu, mfspr(SPRN_SDR1));
+#ifdef CONFIG_SPU_NEED_SHADOW_INT_MASK
+	spin_lock_init(&spu->int_mask_lock);
+#endif
+	if (!(firmware_has_feature(FW_FEATURE_LPAR)))
+		spu_mfc_sdr_set(spu, mfspr(SPRN_SDR1));
 	spu_mfc_sr1_set(spu, 0x33);
 	mutex_lock(&spu_mutex);
 
Index: linux-2.6.19/arch/powerpc/platforms/cell/spufs/switch.c
diff -u linux-2.6.19/arch/powerpc/platforms/cell/spufs/switch.c:1.1.1.1 linux-2.6.19/arch/powerpc/platforms/cell/spufs/switch.c:1.2
--- linux-2.6.19/arch/powerpc/platforms/cell/spufs/switch.c:1.1.1.1	Fri Oct  6 10:40:08 2006
+++ linux-2.6.19/arch/powerpc/platforms/cell/spufs/switch.c	Fri Oct  6 12:26:35 2006
@@ -48,6 +48,7 @@
 #include <asm/spu_priv1.h>
 #include <asm/spu_csa.h>
 #include <asm/mmu_context.h>
+#include <asm/firmware.h>
 
 #include "spu_save_dump.h"
 #include "spu_restore_dump.h"
@@ -2166,7 +2167,8 @@
 	    MFC_STATE1_RELOCATE_MASK | MFC_STATE1_BUS_TLBIE_MASK;
 
 	/* Set storage description.  */
-	csa->priv1.mfc_sdr_RW = mfspr(SPRN_SDR1);
+	if (!firmware_has_feature(FW_FEATURE_LPAR))
+		csa->priv1.mfc_sdr_RW = mfspr(SPRN_SDR1);
 
 	/* Enable OS-specific set of interrupts. */
 	csa->priv1.int_mask_class0_RW = CLASS0_ENABLE_DMA_ALIGNMENT_INTR |
Index: linux-2.6.19/include/asm-powerpc/spu.h
diff -u linux-2.6.19/include/asm-powerpc/spu.h:1.1.1.1 linux-2.6.19/include/asm-powerpc/spu.h:1.3
--- linux-2.6.19/include/asm-powerpc/spu.h:1.1.1.1	Fri Oct  6 10:43:21 2006
+++ linux-2.6.19/include/asm-powerpc/spu.h	Tue Nov  7 12:44:38 2006
@@ -120,9 +120,14 @@
 	unsigned int irqs[3];
 	u32 isrc;
 	u32 node;
+	u32 unit_number;
 	u64 flags;
 	u64 dar;
 	u64 dsisr;
+#ifdef	CONFIG_SPU_NEED_SHADOW_INT_MASK
+	spinlock_t int_mask_lock;
+	u64 shadow_int_mask_RW[3];
+#endif
 	size_t ls_size;
 	unsigned int slb_replace;
 	struct mm_struct *mm;

Re: [PATCH 2/16] add hypervisor support for SPU

[Arnd Bergmann]

On Wednesday 15 November 2006 10:28, Ishizaki Kou wrote:

> +static int __init find_spu_unit_number(struct device_node *spe)
> +{
> +=A0=A0=A0=A0=A0=A0=A0const unsigned int *reg;
> +=A0=A0=A0=A0=A0=A0=A0reg =3D get_property(spe, "reg", NULL);
> +=A0=A0=A0=A0=A0=A0=A0return reg ? *reg : 0;
> +}
> +

The unit number shold not be in the 'reg' property any more. With
the device tree layout we want to migrate to for better OFW compatibility,
'reg' should contain the addresses of local store, problem state and priv2
mappings, in that order.

Having the function is fine, but your interface needs to be a different
property.

> @@ -805,7 +816,11 @@
> =A0=A0=A0=A0=A0=A0=A0=A0if (ret)
> =A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto out_unmap;
> =A0=A0=A0=A0=A0=A0=A0=A0spin_lock_init(&spu->register_lock);
> -=A0=A0=A0=A0=A0=A0=A0spu_mfc_sdr_set(spu, mfspr(SPRN_SDR1));
> +#ifdef CONFIG_SPU_NEED_SHADOW_INT_MASK
> +=A0=A0=A0=A0=A0=A0=A0spin_lock_init(&spu->int_mask_lock);
> +#endif
> +=A0=A0=A0=A0=A0=A0=A0if (!(firmware_has_feature(FW_FEATURE_LPAR)))
> +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0spu_mfc_sdr_set(spu, mfspr(=
SPRN_SDR1));
> =A0=A0=A0=A0=A0=A0=A0=A0spu_mfc_sr1_set(spu, 0x33);
> ...
> =A0=A0=A0=A0=A0=A0=A0=A0u32 node;
> +=A0=A0=A0=A0=A0=A0=A0u32 unit_number;
> =A0=A0=A0=A0=A0=A0=A0=A0u64 flags;
> =A0=A0=A0=A0=A0=A0=A0=A0u64 dar;
> =A0=A0=A0=A0=A0=A0=A0=A0u64 dsisr;
> +#ifdef=A0CONFIG_SPU_NEED_SHADOW_INT_MASK
> +=A0=A0=A0=A0=A0=A0=A0spinlock_t int_mask_lock;
> +=A0=A0=A0=A0=A0=A0=A0u64 shadow_int_mask_RW[3];
> +#endif
> =A0=A0=A0=A0=A0=A0=A0=A0size_t ls_size;
> =A0=A0=A0=A0=A0=A0=A0=A0unsigned int slb_replace;
> =A0=A0=A0=A0=A0=A0=A0=A0struct mm_struct *mm;

Not sure what others think about this, but I'd prefer not to have the
#ifdef here at all. This part is not a fast path, so we can spend the
few extra bytes and cycles on keeping it in the kernel always.

The place where it is used should still have a run-time check for whether
it's running on BEAT.

	Arnd <><

Re: [PATCH 2/16] add hypervisor support for SPU

[Christoph Hellwig]

On Wed, Nov 15, 2006 at 06:28:46PM +0900, Ishizaki Kou wrote:
> This patch adds hypervisor support for spu.
> 
> Linux(GuestOS) needs to designate SPE ID number when it uses SPE on Beat,
> and must read the number from DT.

In addition to the comments from Arnd which I'd like to second, what
do you need int_mask_lock for?  It's initialized but never used.

Re: [PATCH 2/16] add hypervisor support for SPU

[Arnd Bergmann]

On Wednesday 15 November 2006 19:26, Christoph Hellwig wrote:
> On Wed, Nov 15, 2006 at 06:28:46PM +0900, Ishizaki Kou wrote:
> > This patch adds hypervisor support for spu.
> >
> > Linux(GuestOS) needs to designate SPE ID number when it uses SPE on Beat,
> > and must read the number from DT.
>
> In addition to the comments from Arnd which I'd like to second, what
> do you need int_mask_lock for?  It's initialized but never used.

It's used in patch 14/16 for the hypervisor access. As far as I can
see, it should be possible to use the existing register lock in it's
place though. We should hold that lock before calling any of the
priv1 accessors. In order to verify if this is done right,
you could add BUG_ON(!spin_is_locked(&spu->register_lock));
in the beat version of int_mask_{and,or,get,set}.

	Arnd <><

Re: [PATCH 2/16] add hypervisor support for SPU

[Ishizaki Kou]

Hi Arnd-san,

> On Wednesday 15 November 2006 19:26, Christoph Hellwig wrote:
> > On Wed, Nov 15, 2006 at 06:28:46PM +0900, Ishizaki Kou wrote:
> > > This patch adds hypervisor support for spu.
> > >
> > > Linux(GuestOS) needs to designate SPE ID number when it uses SPE on Beat,
> > > and must read the number from DT.
> >
> > In addition to the comments from Arnd which I'd like to second, what
> > do you need int_mask_lock for?  It's initialized but never used.
> 
> It's used in patch 14/16 for the hypervisor access. As far as I can
> see, it should be possible to use the existing register lock in it's
> place though. We should hold that lock before calling any of the
> priv1 accessors. In order to verify if this is done right,
> you could add BUG_ON(!spin_is_locked(&spu->register_lock));
> in the beat version of int_mask_{and,or,get,set}.

Should we hold spu->register_lock before calling 
int_mask_{and,or,get,set}, like as accessing registers?

If it's true, we will replace spu->int_mask_lock by spu->register_lock.
So we remove spu->int_mask_lock.  But we found a problem that
spu_irq_class_0_bottom calls int_mask_get without holding
spu->register_lock. We suspect that similar problems may exist.


Thank you,
Kou Ishizaki
Toshiba

Re: [PATCH 2/16] add hypervisor support for SPU

[Arnd Bergmann]

On Friday 17 November 2006 11:45, Ishizaki Kou wrote:
> > It's used in patch 14/16 for the hypervisor access. As far as I can
> > see, it should be possible to use the existing register lock in it's
> > place though. We should hold that lock before calling any of the
> > priv1 accessors. In order to verify if this is done right,
> > you could add BUG_ON(!spin_is_locked(&spu->register_lock));
> > in the beat version of int_mask_{and,or,get,set}.
>
> Should we hold spu->register_lock before calling
> int_mask_{and,or,get,set}, like as accessing registers?

yes

> If it's true, we will replace spu->int_mask_lock by spu->register_lock.
> So we remove spu->int_mask_lock. =A0But we found a problem that
> spu_irq_class_0_bottom calls int_mask_get without holding
> spu->register_lock. We suspect that similar problems may exist.

They should be changed then in the caller.

	Arnd <><