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

login
register
mail settings
Submitter jschopp@austin.ibm.com
Date Feb. 1, 2010, 10:51 p.m.
Message ID <1265064662.5391.19.camel@jschopp-laptop>
Download mbox | patch
Permalink /patch/44231/
State Superseded
Delegated to: Benjamin Herrenschmidt
Headers show

Comments

jschopp@austin.ibm.com - Feb. 1, 2010, 10:51 p.m.
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> 
---
Tony Breeds - Feb. 2, 2010, 3:48 a.m.
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
jschopp@austin.ibm.com - Feb. 2, 2010, 6:37 p.m.
>> +		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.
Benjamin Herrenschmidt - Feb. 4, 2010, 3:27 a.m.
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.

Patch

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,