[PATCH 0/2] Add max CPU nodes field to ibm,client-architecture call

[PATCH 0/2] Add max CPU nodes field to ibm,client-architecture call

[Joel Schopp]

Large NUMA machines require Linux to indicate support for more than 64
cpu cores.  This is done through the ibm,client-architecture call, and
is documented in the PAPR.  There is also another new field added to
indicate that the OS is Linux as a hint for possible future performance
settings.

The first patch attempts to statically initialize the number of
supported cores with NR_CPUS, the maximum Linux could boot if the cpus
did not have multi-threading (SMT).  It can overestimate by the factor
of SMT.  For instance on Power6 with 2 way SMT it would overestimate by
a factor of 2.  The result of this overestimation is that Linux might
not be able boot all the cpus assigned to it, but would still boot
NR_CPUS worth of SMT threads.

The second patch adjusts for SMT by reading the device tree before
unflattening.  

I've updated patch 1 from previous comments. Patch 1 can be taken on its
own if Patch 2 seems like overkill.  

[PATCH 1/2] Add static fields to ibm,client-architecture call

[Joel Schopp]

This patch adds 2 fields to the ibm_architecture_vec array.  

The first of these fields indicates the number of cores which Linux can
boot.  It does not account for SMT, so it may result in cpus assigned to
Linux which cannot be booted.  A second patch follows that dynamically
updates this for SMT.

The second field just indicates that our OS is Linux, and not another
OS.  The system may or may not use this hint to performance tune
settings for Linux.

Signed-off-by:Joel Schopp<jschopp@austin.ibm.com>
Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
===================================================================
--- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
+++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
@@ -654,6 +656,9 @@ static void __init early_cmdline_parse(v
#define OV5_CMO 0x00
#endif

+/* Option Vector 6: IBM PAPR hints */
+#define OV6_LINUX 0x02 /* Linux is our OS */
+
/*
  * The architecture vector has an array of PVR mask/value pairs,
  * followed by # option vectors - 1, followed by the option vectors.
@@ -665,7 +670,7 @@ static unsigned char ibm_architecture_ve
W(0xffffffff), W(0x0f000003), /* all 2.06-compliant */
W(0xffffffff), W(0x0f000002), /* all 2.05-compliant */
W(0xfffffffe), W(0x0f000001), /* all 2.04-compliant and earlier */
- 5 - 1, /* 5 option vectors */
+ 6 - 1, /* 6 option vectors */

/* option vector 1: processor architectures supported */
3 - 2, /* length */
@@ -697,12 +702,24 @@ static unsigned char ibm_architecture_ve
0, /* don't halt */

/* option vector 5: PAPR/OF options */
- 5 - 2, /* length */
+ 13 - 2, /* length */
0, /* don't ignore, don't halt */
OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_DRCONF_MEMORY |
OV5_DONATE_DEDICATE_CPU | OV5_MSI,
0,
OV5_CMO,
+ 0,
+ 0,
+ 0,
+ 0,
+ W(NR_CPUS), /* number of cores supported*/
+
+ /* option vector 6: IBM PAPR hints */
+ 4 - 2, /* length */
+ 0,
+ 0,
+ OV6_LINUX,
+
};

/* Old method - ELF header with PT_NOTE sections */

[PATCH 1/2] Add static fields to ibm,client-architecture call

[Joel Schopp]

This patch adds 2 fields to the ibm_architecture_vec array.  

The first of these fields indicates the number of cores which Linux can
boot.  It does not account for SMT, so it may result in cpus assigned to
Linux which cannot be booted.  A second patch follows that dynamically
updates this for SMT.

The second field just indicates that our OS is Linux, and not another
OS.  The system may or may not use this hint to performance tune
settings for Linux.

Sorry if the last version sent out got whitespace munged.  Hopefully
evolution works on this one.

Signed-off-by:Joel Schopp<jschopp@austin.ibm.com>
Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
===================================================================
--- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
+++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
@@ -654,6 +656,9 @@ static void __init early_cmdline_parse(v
 #define OV5_CMO			0x00
 #endif
 
+/* Option Vector 6: IBM PAPR hints */
+#define OV6_LINUX		0x02	/* Linux is our OS */
+
 /*
  * The architecture vector has an array of PVR mask/value pairs,
  * followed by # option vectors - 1, followed by the option vectors.
@@ -665,7 +670,7 @@ static unsigned char ibm_architecture_ve
 	W(0xffffffff), W(0x0f000003),	/* all 2.06-compliant */
 	W(0xffffffff), W(0x0f000002),	/* all 2.05-compliant */
 	W(0xfffffffe), W(0x0f000001),	/* all 2.04-compliant and earlier */
-	5 - 1,				/* 5 option vectors */
+	6 - 1,				/* 6 option vectors */
 
 	/* option vector 1: processor architectures supported */
 	3 - 2,				/* length */
@@ -697,12 +702,24 @@ static unsigned char ibm_architecture_ve
 	0,				/* don't halt */
 
 	/* option vector 5: PAPR/OF options */
-	5 - 2,				/* length */
+	13 - 2,				/* length */
 	0,				/* don't ignore, don't halt */
 	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_DRCONF_MEMORY |
 	OV5_DONATE_DEDICATE_CPU | OV5_MSI,
 	0,
 	OV5_CMO,
+	0,
+	0,
+	0,
+	0,
+	W(NR_CPUS),			/* number of cores supported*/
+
+	/* option vector 6: IBM PAPR hints */
+	4 - 2,				/* length */
+	0,
+	0,
+	OV6_LINUX,
+
 };
 
 /* Old method - ELF header with PT_NOTE sections */

Re: [PATCH 1/2] Add static fields to ibm,client-architecture call

[Benjamin Herrenschmidt]

On Thu, 2010-01-14 at 17:01 -0600, Joel Schopp wrote:
> This patch adds 2 fields to the ibm_architecture_vec array.  
> 
> The first of these fields indicates the number of cores which Linux can
> boot.  It does not account for SMT, so it may result in cpus assigned to
> Linux which cannot be booted.  A second patch follows that dynamically
> updates this for SMT.
> 
> The second field just indicates that our OS is Linux, and not another
> OS.  The system may or may not use this hint to performance tune
> settings for Linux.

Patch submission 101...

> Sorry if the last version sent out got whitespace munged.  Hopefully
> evolution works on this one.

The above should be past the signature marker (--) so it doesn't end up
in the changeset comments

> Signed-off-by:Joel Schopp<jschopp@austin.ibm.com>

The marker should be here or the line below:

> Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
> ===================================================================

Would also end up in the comments.

The rest looks fine.

Cheers,
Ben.

> --- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
> +++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
> @@ -654,6 +656,9 @@ static void __init early_cmdline_parse(v
>  #define OV5_CMO			0x00
>  #endif
>  
> +/* Option Vector 6: IBM PAPR hints */
> +#define OV6_LINUX		0x02	/* Linux is our OS */
> +
>  /*
>   * The architecture vector has an array of PVR mask/value pairs,
>   * followed by # option vectors - 1, followed by the option vectors.
> @@ -665,7 +670,7 @@ static unsigned char ibm_architecture_ve
>  	W(0xffffffff), W(0x0f000003),	/* all 2.06-compliant */
>  	W(0xffffffff), W(0x0f000002),	/* all 2.05-compliant */
>  	W(0xfffffffe), W(0x0f000001),	/* all 2.04-compliant and earlier */
> -	5 - 1,				/* 5 option vectors */
> +	6 - 1,				/* 6 option vectors */
>  
>  	/* option vector 1: processor architectures supported */
>  	3 - 2,				/* length */
> @@ -697,12 +702,24 @@ static unsigned char ibm_architecture_ve
>  	0,				/* don't halt */
>  
>  	/* option vector 5: PAPR/OF options */
> -	5 - 2,				/* length */
> +	13 - 2,				/* length */
>  	0,				/* don't ignore, don't halt */
>  	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_DRCONF_MEMORY |
>  	OV5_DONATE_DEDICATE_CPU | OV5_MSI,
>  	0,
>  	OV5_CMO,
> +	0,
> +	0,
> +	0,
> +	0,
> +	W(NR_CPUS),			/* number of cores supported*/
> +
> +	/* option vector 6: IBM PAPR hints */
> +	4 - 2,				/* length */
> +	0,
> +	0,
> +	OV6_LINUX,
> +
>  };
>  
>  /* Old method - ELF header with PT_NOTE sections */
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH 1/2] Add static fields to ibm,client-architecture call

[Benjamin Herrenschmidt]

Oh and :

> > Signed-off-by:Joel Schopp<jschopp@austin.ibm.com>
                 ^ space     ^ space

Thanks !

Cheers,
Ben.


> The marker should be here or the line below:
> 
> > Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
> > ===================================================================
> 
> Would also end up in the comments.
> 
> The rest looks fine.
> 
> Cheers,
> Ben.
> 
> > --- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
> > +++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
> > @@ -654,6 +656,9 @@ static void __init early_cmdline_parse(v
> >  #define OV5_CMO			0x00
> >  #endif
> >  
> > +/* Option Vector 6: IBM PAPR hints */
> > +#define OV6_LINUX		0x02	/* Linux is our OS */
> > +
> >  /*
> >   * The architecture vector has an array of PVR mask/value pairs,
> >   * followed by # option vectors - 1, followed by the option vectors.
> > @@ -665,7 +670,7 @@ static unsigned char ibm_architecture_ve
> >  	W(0xffffffff), W(0x0f000003),	/* all 2.06-compliant */
> >  	W(0xffffffff), W(0x0f000002),	/* all 2.05-compliant */
> >  	W(0xfffffffe), W(0x0f000001),	/* all 2.04-compliant and earlier */
> > -	5 - 1,				/* 5 option vectors */
> > +	6 - 1,				/* 6 option vectors */
> >  
> >  	/* option vector 1: processor architectures supported */
> >  	3 - 2,				/* length */
> > @@ -697,12 +702,24 @@ static unsigned char ibm_architecture_ve
> >  	0,				/* don't halt */
> >  
> >  	/* option vector 5: PAPR/OF options */
> > -	5 - 2,				/* length */
> > +	13 - 2,				/* length */
> >  	0,				/* don't ignore, don't halt */
> >  	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_DRCONF_MEMORY |
> >  	OV5_DONATE_DEDICATE_CPU | OV5_MSI,
> >  	0,
> >  	OV5_CMO,
> > +	0,
> > +	0,
> > +	0,
> > +	0,
> > +	W(NR_CPUS),			/* number of cores supported*/
> > +
> > +	/* option vector 6: IBM PAPR hints */
> > +	4 - 2,				/* length */
> > +	0,
> > +	0,
> > +	OV6_LINUX,
> > +
> >  };
> >  
> >  /* Old method - ELF header with PT_NOTE sections */
> > 
> > 
> > _______________________________________________
> > Linuxppc-dev mailing list
> > Linuxppc-dev@lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

[PATCHv2 1/2] Add static fields to ibm,client-architecture call

[Joel Schopp]

This patch adds 2 fields to the ibm_architecture_vec array.  

The first of these fields indicates the number of cores which Linux can
boot.  It does not account for SMT, so it may result in cpus assigned to
Linux which cannot be booted.  A second patch follows that dynamically
updates this for SMT.

The second field just indicates that our OS is Linux, and not another
OS.  The system may or may not use this hint to performance tune
settings for Linux.

Signed-off-by: Joel Schopp <jschopp@austin.ibm.com>
---
Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
===================================================================
--- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
+++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
@@ -654,6 +656,9 @@ static void __init early_cmdline_parse(v
 #define OV5_CMO			0x00
 #endif
 
+/* Option Vector 6: IBM PAPR hints */
+#define OV6_LINUX		0x02	/* Linux is our OS */
+
 /*
  * The architecture vector has an array of PVR mask/value pairs,
  * followed by # option vectors - 1, followed by the option vectors.
@@ -665,7 +670,7 @@ static unsigned char ibm_architecture_ve
 	W(0xffffffff), W(0x0f000003),	/* all 2.06-compliant */
 	W(0xffffffff), W(0x0f000002),	/* all 2.05-compliant */
 	W(0xfffffffe), W(0x0f000001),	/* all 2.04-compliant and earlier */
-	5 - 1,				/* 5 option vectors */
+	6 - 1,				/* 6 option vectors */
 
 	/* option vector 1: processor architectures supported */
 	3 - 2,				/* length */
@@ -697,12 +702,24 @@ static unsigned char ibm_architecture_ve
 	0,				/* don't halt */
 
 	/* option vector 5: PAPR/OF options */
-	5 - 2,				/* length */
+	13 - 2,				/* length */
 	0,				/* don't ignore, don't halt */
 	OV5_LPAR | OV5_SPLPAR | OV5_LARGE_PAGES | OV5_DRCONF_MEMORY |
 	OV5_DONATE_DEDICATE_CPU | OV5_MSI,
 	0,
 	OV5_CMO,
+	0,
+	0,
+	0,
+	0,
+	W(NR_CPUS),			/* number of cores supported*/
+
+	/* option vector 6: IBM PAPR hints */
+	4 - 2,				/* length */
+	0,
+	0,
+	OV6_LINUX,
+
 };
 
 /* Old method - ELF header with PT_NOTE sections */

[PATCH 2/2] Update ibm,client-architecture call field based on device tree

[Joel Schopp]

In the previous patch the client-architecture field for the number of
cores supported is set statically as high as is possible.  However, that
static setting could be too high if the system supports smt, resulting
in cpus assigned to Linux that are not booted.  This patch reads the
device tree (before it is unflattened) to determine the amount of smt.
It then dynamically updates the entires in the array with the proper
number of cores supported.  Tests show this correctly detecting SMT4 on
a Power7 and still booting all the supported cores on a large machine.


Signed-off-by:Joel Schopp<jschopp@austin.ibm.com> 
Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
===================================================================
--- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
+++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
@@ -141,6 +141,8 @@ typedef u32 cell_t;
 
 extern void __start(unsigned long r3, unsigned long r4, unsigned long r5);
 
+static int __init prom_smt_way(void);
+
 #ifdef CONFIG_PPC64
 extern int enter_prom(struct prom_args *args, unsigned long entry);
 #else
@@ -811,9 +813,17 @@ static void __init prom_send_capabilitie
 {
 	ihandle elfloader, root;
 	prom_arg_t ret;
+	u32 *cores;
 
 	root = call_prom("open", 1, 1, ADDR("/"));
 	if (root != 0) {
+		/*
+		 * If you add to the struct, please be sure the 100 index
+		 * didn't change.  The BUILD_BUG_ON is a reminder.
+		 */
+		BUILD_BUG_ON(sizeof(ibm_architecture_vec) != 108);
+		cores = (u32 *) &ibm_architecture_vec[100];
+		*cores = (u32) (NR_CPUS/prom_smt_way());
 		/* try calling the ibm,client-architecture-support method */
 		prom_printf("Calling ibm,client-architecture-support...");
 		if (call_prom_ret("call-method", 3, 2, &ret,
@@ -1031,6 +1041,45 @@ static void __init reserve_mem(u64 base,
 	RELOC(mem_reserve_cnt) = cnt + 1;
 }
 
+
+static int __init prom_smt_way(void)
+{
+	phandle node;
+	char type[64];
+	unsigned int plen;
+
+	for (node = 0; prom_next_node(&node); ) {
+		type[0] = 0;
+		prom_getprop(node, "device_type", type, sizeof(type));
+
+		if (type[0] == 0) {
+			/*
+			 * CHRP Longtrail machines have no device_type
+			 * on the memory node, so check the name instead...
+			 */
+			prom_getprop(node, "name", type, sizeof(type));
+		}
+		if (strcmp(type, RELOC("cpu")))
+			continue;
+
+		/*
+		 * There is an entry for each smt thread, each entry being
+		 * 4 bytes long.  All cpus should have the same number of
+		 * smt threads, so return after finding the first.
+		 */
+		plen = prom_getproplen(node, "ibm,ppc-interrupt-server#s");
+		prom_debug("smt %x\n", (unsigned long) plen);
+		if (plen >= 4)
+			return plen / 4;
+	}
+	/*
+	 * If things go wrong and we get here fallback to SMT1
+	 */
+	prom_debug("unable to determine smt from device tree, guessing smt1\n");
+	return 1;
+
+}
+
 /*
  * Initialize memory allocation mechanism, parse "memory" nodes and
  * obtain that way the top of memory and RMO to setup out local allocator

Re: [PATCH 2/2] Update ibm,client-architecture call field based on device tree

[Benjamin Herrenschmidt]

On Thu, 2010-01-14 at 14:41 -0600, Joel Schopp wrote:
> In the previous patch the client-architecture field for the number of
> cores supported is set statically as high as is possible.  However, that
> static setting could be too high if the system supports smt, resulting
> in cpus assigned to Linux that are not booted.  This patch reads the
> device tree (before it is unflattened) to determine the amount of smt.
> It then dynamically updates the entires in the array with the proper
> number of cores supported.  Tests show this correctly detecting SMT4 on
> a Power7 and still booting all the supported cores on a large machine.

Same comments about submission format as the previous patch...

> Signed-off-by:Joel Schopp<jschopp@austin.ibm.com> 
> Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
> ===================================================================
> --- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
> +++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
> @@ -141,6 +141,8 @@ typedef u32 cell_t;
>  
>  extern void __start(unsigned long r3, unsigned long r4, unsigned long r5);
>  
> +static int __init prom_smt_way(void);

Do you really need a forward declaration ? We generally avoid those and
prefer instead having the functions in the right order to make it
unnecessary.

>  #ifdef CONFIG_PPC64
>  extern int enter_prom(struct prom_args *args, unsigned long entry);
>  #else
> @@ -811,9 +813,17 @@ static void __init prom_send_capabilitie
>  {
>  	ihandle elfloader, root;
>  	prom_arg_t ret;
> +	u32 *cores;
>  
>  	root = call_prom("open", 1, 1, ADDR("/"));
>  	if (root != 0) {
> +		/*
> +		 * If you add to the struct, please be sure the 100 index
> +		 * didn't change.  The BUILD_BUG_ON is a reminder.
> +		 */
> +		BUILD_BUG_ON(sizeof(ibm_architecture_vec) != 108);

This is indeed a bit fishy... a nicer way may have been to have the
vector in an asm file with labels but that's probably overkill. Just
maybe add a runtime test that checks the value read initially is
NR_CORES and if not, print a big fat warning ?

> +		cores = (u32 *) &ibm_architecture_vec[100];
> +		*cores = (u32) (NR_CPUS/prom_smt_way());

The style is a bit gross though I suppose it will do. Drop the cast on
the second line, it's not useful, and stick some spaces around that
division. Also maybe print out a message saying to what value you
adjusted the max number of supported cores, might be useful on the field
to diagnose issues.

>  		/* try calling the ibm,client-architecture-support method */
>  		prom_printf("Calling ibm,client-architecture-support...");
>  		if (call_prom_ret("call-method", 3, 2, &ret,
> @@ -1031,6 +1041,45 @@ static void __init reserve_mem(u64 base,
>  	RELOC(mem_reserve_cnt) = cnt + 1;
>  }
>  
> +
> +static int __init prom_smt_way(void)
> +{
> +	phandle node;
> +	char type[64];
> +	unsigned int plen;
> +
> +	for (node = 0; prom_next_node(&node); ) {
> +		type[0] = 0;
> +		prom_getprop(node, "device_type", type, sizeof(type));
> +
> +		if (type[0] == 0) {
> +			/*
> +			 * CHRP Longtrail machines have no device_type
> +			 * on the memory node, so check the name instead...
> +			 */
> +			prom_getprop(node, "name", type, sizeof(type));

They also have no SMT :-) Just continue instead. You could also have
instead gone for /cpus and peeked at the first child. Might have lead to
a simpler construct (and faster runtime)

> +		}
> +		if (strcmp(type, RELOC("cpu")))
> +			continue;
> +
> +		/*
> +		 * There is an entry for each smt thread, each entry being
> +		 * 4 bytes long.  All cpus should have the same number of
> +		 * smt threads, so return after finding the first.
> +		 */
> +		plen = prom_getproplen(node, "ibm,ppc-interrupt-server#s");
> +		prom_debug("smt %x\n", (unsigned long) plen);

Might only be a debug message but it should be a tad more verbose about
what it's actually printing.

> +		if (plen >= 4)
> +			return plen / 4;
> +	}
> +	/*
> +	 * If things go wrong and we get here fallback to SMT1
> +	 */
> +	prom_debug("unable to determine smt from device tree, guessing smt1\n");
> +	return 1;
> +
> +}
> +

Cheers,
Ben.

>  /*
>   * Initialize memory allocation mechanism, parse "memory" nodes and
>   * obtain that way the top of memory and RMO to setup out local allocator
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCHv2 2/2] Update ibm,client-architecture call field based on device tree

[Joel Schopp]

In the previous patch the client-architecture field for the number of
cores supported is set statically as high as is possible.  However, that
static setting could be too high if the system supports smt, resulting
in cpus assigned to Linux that are not booted.  This patch reads the
device tree (before it is unflattened) to determine the amount of smt.
It then dynamically updates the entires in the array with the proper
number of cores supported.  Tests show this correctly detecting SMT4 on
a Power7 and still booting all the supported cores on a large machine.

Signed-off-by: Joel Schopp <jschopp@austin.ibm.com> 
---
Index: linux-2.6.git/arch/powerpc/kernel/prom_init.c
===================================================================
--- linux-2.6.git.orig/arch/powerpc/kernel/prom_init.c
+++ linux-2.6.git/arch/powerpc/kernel/prom_init.c
@@ -807,13 +807,55 @@ static struct fake_elf {
 	}
 };
 
+static int __init prom_smt_way(void)
+{
+	phandle node;
+	char type[64];
+	unsigned int plen;
+
+	for (node = 0; prom_next_node(&node); ) {
+		type[0] = 0;
+		prom_getprop(node, "device_type", type, sizeof(type));
+
+		if (strcmp(type, RELOC("cpu")))
+			continue;
+
+		/*
+		 * There is an entry for each smt thread, each entry being
+		 * 4 bytes long.  All cpus should have the same number of
+		 * smt threads, so return after finding the first.
+		 */
+		plen = prom_getproplen(node, "ibm,ppc-interrupt-server#s");
+		prom_printf("smt threads per core %x\n", (unsigned long) plen);
+		if (plen >= 4)
+			return plen / 4;
+	}
+	/*
+	 * If things go wrong and we get here fallback to SMT1
+	 */
+	prom_debug("unable to determine smt from device tree, guessing smt1\n");
+	return 1;
+
+}
+
+
 static void __init prom_send_capabilities(void)
 {
 	ihandle elfloader, root;
 	prom_arg_t ret;
+	u32 *cores;
 
 	root = call_prom("open", 1, 1, ADDR("/"));
 	if (root != 0) {
+		/*
+		 * If you add to the struct, please be sure the 100 index
+		 * didn't change.  The BUILD_BUG_ON is a reminder.
+		 */
+		cores = (u32 *) &ibm_architecture_vec[100];
+		if(*cores != NR_CPUS)
+			prom_printf("client-architecture structure corrupted\n");
+		*cores = (NR_CPUS / prom_smt_way());
+		prom_printf("setting client-architecture cores to %x\n", *cores);
 		/* try calling the ibm,client-architecture-support method */
 		prom_printf("Calling ibm,client-architecture-support...");
 		if (call_prom_ret("call-method", 3, 2, &ret,

Re: [PATCHv2 2/2] Update ibm,client-architecture call field based on device tree

[Tony Breeds]

On Mon, Feb 01, 2010 at 04:51:02PM -0600, Joel Schopp wrote:

>  static void __init prom_send_capabilities(void)
>  {
>  	ihandle elfloader, root;
>  	prom_arg_t ret;
> +	u32 *cores;
>  
>  	root = call_prom("open", 1, 1, ADDR("/"));
>  	if (root != 0) {
> +		/*
> +		 * If you add to the struct, please be sure the 100 index
> +		 * didn't change.  The BUILD_BUG_ON is a reminder.
> +		 */
> +		cores = (u32 *) &ibm_architecture_vec[100];
> +		if(*cores != NR_CPUS)
> +			prom_printf("client-architecture structure corrupted\n");
> +		*cores = (NR_CPUS / prom_smt_way());
> +		prom_printf("setting client-architecture cores to %x\n", *cores);

I don't know if I'm painting a bike shed of if this is a real concern, but if
*cores isn't NR_CPUS shouldn't we do nothing rather then clobbering it?

Yours Tony

Re: [PATCHv2 2/2] Update ibm,client-architecture call field based on device tree

[Joel Schopp]

>> +		if(*cores != NR_CPUS)
>> +			prom_printf("client-architecture structure corrupted\n");
>> +		*cores = (NR_CPUS / prom_smt_way());
>> +		prom_printf("setting client-architecture cores to %x\n", *cores);
>>     
>
> I don't know if I'm painting a bike shed of if this is a real concern, but if
> *cores isn't NR_CPUS shouldn't we do nothing rather then clobbering it?
>
> Yours Tony
>   
If it isn't NR_CPUS we're pretty broken if we set it or if we don't.  My 
previous version did a BUILD_BUG_ON() but Ben didn't like that and said 
he preferred just a warning message. 

Re: [PATCHv2 2/2] Update ibm,client-architecture call field based on device tree

[Benjamin Herrenschmidt]

On Tue, 2010-02-02 at 12:37 -0600, Joel Schopp wrote:
> >> +		if(*cores != NR_CPUS)
> >> +			prom_printf("client-architecture structure corrupted\n");
> >> +		*cores = (NR_CPUS / prom_smt_way());
> >> +		prom_printf("setting client-architecture cores to %x\n", *cores);
> >>     
> >
> > I don't know if I'm painting a bike shed of if this is a real concern, but if
> > *cores isn't NR_CPUS shouldn't we do nothing rather then clobbering it?
> >
> > Yours Tony
> >   
> If it isn't NR_CPUS we're pretty broken if we set it or if we don't.  My 
> previous version did a BUILD_BUG_ON() but Ben didn't like that and said 
> he preferred just a warning message. 

BUILD_BUG_ON would probably not have worked unless gcc smarter than I
think it is :-) Your latest approach is bad because when you detect
you are poking at the wrong thing ... you still proceed and do it. In
fact you -are- poking at the wrong thing always since you are missing
PTRRELOC() to relocate the pointer to the vec.

I'll post a new variant of that patch.

Cheers,
Ben.

[PATCHv2 0/2] Add max CPU nodes field to ibm,client-architecture call

[Joel Schopp]

Large NUMA machines require Linux to indicate support for more than 64
cpu cores.  This is done through the ibm,client-architecture call, and
is documented in the PAPR.  There is also another new field added to
indicate that the OS is Linux as a hint for possible future performance
settings.

The first patch attempts to statically initialize the number of
supported cores with NR_CPUS, the maximum Linux could boot if the cpus
did not have multi-threading (SMT).  It can overestimate by the factor
of SMT.  For instance on Power6 with 2 way SMT it would overestimate by
a factor of 2.  The result of this overestimation is that Linux might
not be able boot all the cpus assigned to it, but would still boot
NR_CPUS worth of SMT threads.

The second patch adjusts for SMT by reading the device tree before
unflattening.  

I've updated patch 1 from previous comments. Patch 1 can be taken on its
own if Patch 2 seems like overkill.  

v2 - Updated patch 2 style and messages.  Basic functionality is the
same as v1.