Re: [PATCH v2 3/3] powerpc/numa: Support coregroup on PowerNV

[IBM]

Srikar Dronamraju <srikar@xxxxxxxxxxxxx> writes:

> * Ritesh Harjani <ritesh.list@xxxxxxxxx> [2026-07-10 11:18:12]:
>
>> Srikar Dronamraju <srikar@xxxxxxxxxxxxx> writes:
>> 
>> > Coregroup support on powerpc has so far been limited to PowerVM LPARs.
>> > However, PowerNV can also support coregroups when firmware exposes the
>> > required coregroup information through the associativity hierarchy.
>> >
>> > Detect coregroup support by checking whether primary_domain_index is the
>> > penultimate domain in the CPU node's ibm,associativity property. On
>> > PowerNV, a non-penultimate primary_domain_index indicates that firmware
>> > provides an additional level for coregroup information.
>> >
>> > This keeps the logic compatible with PowerVM systems, where
>> > primary_domain_index is likewise not the penultimate associativity
>> > domain.
>> >
>> > Signed-off-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxx>
>> > ---
>> > Changelog from v1: https://lkml.kernel.org/r/20260524010017.140408-1-srikar@xxxxxxxxxxxxx
>> > - Handle comments from Christophe Leroy; make code more flat
>> >
>> >  arch/powerpc/mm/numa.c | 56 ++++++++++++++++++++++++++++++++++--------
>> >  1 file changed, 46 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> > index 9aa71eb7e96b..e97b624203ea 100644
>> > --- a/arch/powerpc/mm/numa.c
>> > +++ b/arch/powerpc/mm/numa.c
>> > @@ -889,12 +889,32 @@ static int __init numa_setup_drmem_lmb(struct drmem_lmb *lmb,
>> >  	return 0;
>> >  }
>
> Hi Ritesh,
>
> Thanks for the review.
>
>> >  
>> > +/*
>> > + * If hierarchy extends beyond primary_domain_index + 1, then next
>> > + * level corresponds to coregroup.
>> > + */
>> > +static int detect_and_enable_coregroup(const __be32 *associativity, int index)
>> 
>> Do we care about it's return value? We are not reading that in the
>> patch.
>
> Yes, We do care about the return value. If the index is set to -1, we don't
> retry enabling the coregroup.
>

yes, my bad. Agreed it is being used.


>> this function is mainly only needed in __init, can we mark it so.
>
> Yes, this will be done.
>
>> 
>> > +{
>> > +	if (!associativity || index == -1)
>> > +		goto out;
>> > +
>> > +	index = of_read_number(associativity, 1);
>> > +
>> > +	if (index > primary_domain_index + 1) {
>> > +		coregroup_enabled = 1;
>> > +		return index;
>> > +	}
>> > +out:
>> > +	coregroup_enabled = 0;
>> > +	return -1;
>> > +}
>> 
>> For PowerVM, we now have two places which will enable coregroup_enabled
>> during mem_topology_setup(). Is there some way we can unify that?
>> 
>
> For PowerVM, we have two extra associativity properties
> ibm,ibm,current-associativity-domains and ibm,max-associativity-domains.
> On PowerNV, these two properties are not used/exported.
>
> All we depend is the layout of these properties to determine if coregroup is
> enabled. If the layout tells us that there is place after
> primary_domain_index for coregroup, we assume coregroup is enabled.
>
> So in this patch, we hook at the place we look at each of the CPU
> associativity. This should work for both PowerVM and PowerNV.
>
> So, I can think of two options.
> 1. Remove the previous logic of depending on PowerVM specific code.
> 2. Allow the previous logic to be around. Since its not going to hurt
> functionally or performance wise.
>
>> This also means we enable coregroup in case of PowerVM with SPLPAR when
>> per-cpu VPHN associativity index > primary_domain_index+1. But this
>> isn't reflected in your commit msg. The commit msg only says this
>> affects PowerNV.
>
> I don't think, I said this affects PowerNV only. But I still don't think
> the logic would change. The logic to enable coregroup remains the same.
> Just that we may now be depending on the 1st CPU associativity instead of
> the PowerVM specific properties.
>

So let's just add this info in the commit msg please. Because it was not
clear otherwise.

-ritesh