Re: [PATCH] platform/x86/intel-uncore-freq: Present unique domain ID per package

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Wed, 2025-08-06 at 12:17 +0300, Ilpo Järvinen wrote:
> On Sun, 27 Jul 2025, Srinivas Pandruvada wrote:
> 
> > In partitioned systems, the domain ID is unique in the partition
> > and a
> > package can have multiple partitions.
> > 
> > Some user-space tools, such as turbostat, assume the domain ID is
> > unique
> > per package. These tools map CPU power domains, which are unique to
> > a
> > package. However, this approach does not work in partitioned
> > systems.
> > 
> > There is no architectural definition of "partition" to present to
> > user
> > space.
> > 
> > To support these tools, set the domain_id to be unique per package.
> > For
> > compute die IDs, uniqueness can be achieved using the platform info
> > cdie_mask, mirroring the behavior observed in non-partitioned
> > systems.
> > 
> > For IO dies, which lack a direct CPU relationship, any unique
> > logical
> > ID can be assigned. Here domain IDs for IO dies are configured
> > after all
> > compute domain IDs. During the probe, keep the index of the next IO
> > domain ID after the last IO domain ID of the current partition.
> > Since
> > CPU packages are symmetric, partition information is same for all
> > packages.
> > 
> > The Intel Speed Select driver has already implemented a similar
> > change
> > to make the domain ID unique, with compute dies listed first,
> > followed
> > by I/O dies.
> > 

[...]

> > +	/* For non partitioned system or invalid partition number,
> > return */
> 
> non-partitioned
> 
Will correct that.

> > +	if (!plat_info->cdie_mask || max_dies <= 1 || plat_info-
> > >partition >= MAX_PARTITIONS)
> > +		return;
> > +
> > +	if (cluster_info->uncore_data.agent_type_mask &
> > AGENT_TYPE_CORE) {
> > +		cluster_info->uncore_data.domain_id =
> > cluster_info->cdie_id;
> > +		return;
> > +	}
> > +
> > +	cdie_cnt = fls(plat_info->cdie_mask) - ffs(plat_info-
> > >cdie_mask) + 1;
> 
> Is it intentional that you didn't use hweight here? (unfortunately,
> I don't recall details of the cdie_mask).
> 
Although unlikely but nothing stops of being holes in the die mask. But
for usage here it will not make difference.

> > +	guard(mutex)(&domain_lock)
> > +
> > +	if (!io_die_index_next)
> > +		io_die_index_next = max_dies;
> > +
> > +	if (!io_die_start[plat_info->partition]) {
> > +		io_die_start[plat_info->partition] =
> > io_die_index_next;
> > +		io_die_index_next += (num_resource - cdie_cnt);
> > +	}
> > +
> > +	cluster_info->uncore_data.domain_id +=
> > (io_die_start[plat_info->partition] - cdie_cnt);
> 
> I failed to wrap my head around what this math aims to do (mainly
> what 
> cdie_cnt has to do with this). Can you explain (might be useful to
> have a 
> comment if it's something particularly tricky / non-obvious)?
> 
Seems not obvious but something like below in #if 0

#if 0
/*
Index from IO die start with in the partition

For example the current resource index 5 (cluster_info-
>uncore_data.domain_id) and compute dies end at index 3 (cdie_cnt = 4).
then the io only index 5 - 4 = 1
*/
u8 part_io_index = cluster_info->uncore_data.domain_id - cdie_cnt;

/* Add to the IO die start index for this partition in this package to
make unique in the package */
u8 pkg_io_index = io_die_start[plat_info->partition] + part_io_index;

/* Assign this to domain ID */
cluster_info->uncore_data.domain_id = pkg_io_index;
#endif

In one line the above whole #if block
"cluster_info->uncore_data.domain_id = io_die_start[plat_info-
>partition] + cluster_info->uncore_data.domain_id - cdie_cnt;"
which is
cluster_info->uncore_data.domain_id += (io_die_start[plat_info-
>partition] - cdie_cnt)
}


> It could be that to make this simpler, one shouldn't assign value in 
> uncore_probe() to .domain_id at all but pass the index here (and
> rename 
> this function to set_domain_id()).
> 
Can do if that is any simpler here.

Thanks,
Srinivas

> > +}
> > +
> >  /* Callback for sysfs write for TPMI uncore data. Called under
> > mutex locks. */
> >  static int uncore_write(struct uncore_data *data, unsigned int
> > value, enum uncore_index index)
> >  {
> > @@ -614,6 +655,7 @@ static int uncore_probe(struct auxiliary_device
> > *auxdev, const struct auxiliary_
> >  			cluster_info->uncore_data.cluster_id = j;
> >  
> >  			set_cdie_id(i, cluster_info, plat_info);
> > +			update_domain_id(cluster_info, plat_info,
> > num_resources);
> >  
> >  			cluster_info->uncore_root = tpmi_uncore;
> >  
> > 
>