[M] Change in coreboot[master]: soc/intel/xeon_sp: Don't sort struct device cpus for numa

[Arthur Heymans (Code Review)]

Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Christian Walter, Angel Pons, Arthur Heymans, Shuming Chu (Shuming), TangYiwei.

Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/68912 )

Change subject: soc/intel/xeon_sp: Don't sort struct device cpus for numa
......................................................................

Patch Set 3:

(5 comments)

Patchset:

PS3: 
Thanks for reviewing! I think MADT code actually needs something similar. I also want to put the topology code inside common mp_init.c. It's common accros x86 CPUs.

File src/soc/intel/xeon_sp/cpx/cpu.c:

https://review.coreboot.org/c/coreboot/+/68912/comment/d385e81b_f911d095 
PS3, Line 81: apic_id
Should be package_id

File src/soc/intel/xeon_sp/nb_acpi.c:

https://review.coreboot.org/c/coreboot/+/68912/comment/a242c311_a3cfe66f 
PS3, Line 29: /* First all regular cpus, then all the threads */
            : 	for (cpu = all_devices; cpu; cpu = cpu->next) {
            : 		if (!is_enabled_cpu(cpu))
            : 			continue;
            : 		if (num_cpus >= ARRAY_SIZE(apic_ids))
            : 			break;
            : 		/* Is this a thread? */
            : 		if (cpu->path.apic.apic_id & 1)
            : 			continue;
            : 		apic_ids[num_cpus++] = cpu->path.apic.apic_id;
            : 	}
            : 	const uint32_t num_of_cores = num_cpus;
            : 	if (num_cpus > 1)
            : 		bubblesort(&apic_ids[0], num_of_cores, NUM_ASCENDING);
> Maybe rewrite this as a loop?
> const uint32_t max_th_per_core = 2;
> uint32_t cpus_so_far = 0;
> for (uint32_t th_in_core = 0; th_in_core < max_th_per_core; th_in_core++) {
>    for (cpu = all_devices; cpu; cpu = cpu->next) {
>    	if (!is_enabled_cpu(cpu))
>    		continue;
>    	if (num_cpus >= ARRAY_SIZE(apic_ids))
>    		break;
>    	/* Is this a thread? */
>    	if (cpu->path.apic.apic_id % max_th_per_core != th_in_core)
>    		continue;
>    	apic_ids[num_cpus++] = cpu->path.apic.apic_id;
>    }
>    bubblesort(&apic_ids[cpus_so_far], num_cpus - cpus_so_far, NUM_ASCENDING);
>    cpus_so_far = num_cpus;
> }
>
> Yes, no idea if we'll ever have to deal with more than 2 threads per core. The compiler should be able to do loop unrolling if it thinks it's worth it.

I want to put that information in struct path. It already has a tread_id entry. it can loop over thread_id. If no thread_id CPU is found a loop can break. That would allow for more than 2 threads per core_id.

https://review.coreboot.org/c/coreboot/+/68912/comment/c1ff5175_d857a048 
PS3, Line 54: 	if (num_cpus > 1)
> What's the purpose of this check? Did you mean to check if `num_cpus - num_of_cores > 1` instead? You don't need any checks, `bubblesort()` bails out early if there's less than 2 elements to sort.

copy pasta from MADT code.

https://review.coreboot.org/c/coreboot/+/68912/comment/ce370a9e_1581b684 
PS3, Line 59: for (cpu = all_devices; cpu; cpu = cpu->next) {
            : 			if (!is_enabled_cpu(cpu))
            : 				continue;
            : 			if (cpu->path.apic.apic_id == apic_ids[i])
            : 				break;
            : 		}
> Hmmm, having to re-do this search is a bit inefficient... The problem seems to be that the `bubblesort()` function doesn't allow sorting `struct device *` according to the value of some member. Making a generic sort function in C would involve void pointers and function pointers:
> typedef void (*comparator_func_t)(void *, void *);
> void generic_bubblesort(void *array[], size_t len, comparator_func_t fn);

Using array of struct *path or struct *device and a comparator is indeed the way to go :-)