Re: [PATCH V3 1/4] perf/x86/intel/uncore: Support MSR portal for discovery tables

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2025-07-03 6:04 p.m., Mi, Dapeng wrote:
> 
> On 7/4/2025 12:56 AM, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> Starting from the Panther Lake, the discovery table mechanism is also
>> supported in client platforms. The difference is that the portal of the
>> global discovery table is retrieved from an MSR.
>>
>> The layout of discovery tables are the same as the server platforms.
>> Factor out __parse_discovery_table() to parse discover tables.
>>
>> The uncore PMON is Die scope. Need to parse the discovery tables for
>> each die.
>>
>> Signed-off-by: Kan Liang <kan.liang@linux.intel.com>
>> ---
>>  arch/x86/events/intel/uncore_discovery.c | 87 ++++++++++++++++++------
>>  arch/x86/events/intel/uncore_discovery.h |  3 +
>>  2 files changed, 70 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/uncore_discovery.c b/arch/x86/events/intel/uncore_discovery.c
>> index 571e44b49691..8680f66c3e34 100644
>> --- a/arch/x86/events/intel/uncore_discovery.c
>> +++ b/arch/x86/events/intel/uncore_discovery.c
>> @@ -273,32 +273,15 @@ uncore_ignore_unit(struct uncore_unit_discovery *unit, int *ignore)
>>  	return false;
>>  }
>>  
>> -static int parse_discovery_table(struct pci_dev *dev, int die,
>> -				 u32 bar_offset, bool *parsed,
>> -				 int *ignore)
>> +static int __parse_discovery_table(resource_size_t addr, int die,
>> +				   bool *parsed, int *ignore)
>>  {
>>  	struct uncore_global_discovery global;
>>  	struct uncore_unit_discovery unit;
>>  	void __iomem *io_addr;
>> -	resource_size_t addr;
>>  	unsigned long size;
>> -	u32 val;
>>  	int i;
>>  
>> -	pci_read_config_dword(dev, bar_offset, &val);
>> -
>> -	if (val & ~PCI_BASE_ADDRESS_MEM_MASK & ~PCI_BASE_ADDRESS_MEM_TYPE_64)
>> -		return -EINVAL;
>> -
>> -	addr = (resource_size_t)(val & PCI_BASE_ADDRESS_MEM_MASK);
>> -#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> -	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> -		u32 val2;
>> -
>> -		pci_read_config_dword(dev, bar_offset + 4, &val2);
>> -		addr |= ((resource_size_t)val2) << 32;
>> -	}
>> -#endif
>>  	size = UNCORE_DISCOVERY_GLOBAL_MAP_SIZE;
>>  	io_addr = ioremap(addr, size);
>>  	if (!io_addr)
>> @@ -341,7 +324,32 @@ static int parse_discovery_table(struct pci_dev *dev, int die,
>>  	return 0;
>>  }
>>  
>> -bool intel_uncore_has_discovery_tables(int *ignore)
>> +static int parse_discovery_table(struct pci_dev *dev, int die,
>> +				 u32 bar_offset, bool *parsed,
>> +				 int *ignore)
>> +{
>> +	resource_size_t addr;
>> +	u32 val;
>> +
>> +	pci_read_config_dword(dev, bar_offset, &val);
>> +
>> +	if (val & ~PCI_BASE_ADDRESS_MEM_MASK & ~PCI_BASE_ADDRESS_MEM_TYPE_64)
>> +		return -EINVAL;
>> +
>> +	addr = (resource_size_t)(val & PCI_BASE_ADDRESS_MEM_MASK);
>> +#ifdef CONFIG_PHYS_ADDR_T_64BIT
>> +	if ((val & PCI_BASE_ADDRESS_MEM_TYPE_MASK) == PCI_BASE_ADDRESS_MEM_TYPE_64) {
>> +		u32 val2;
>> +
>> +		pci_read_config_dword(dev, bar_offset + 4, &val2);
>> +		addr |= ((resource_size_t)val2) << 32;
>> +	}
>> +#endif
>> +
>> +	return __parse_discovery_table(addr, die, parsed, ignore);
>> +}
>> +
>> +static bool intel_uncore_has_discovery_tables_pci(int *ignore)
>>  {
>>  	u32 device, val, entry_id, bar_offset;
>>  	int die, dvsec = 0, ret = true;
>> @@ -390,6 +398,45 @@ bool intel_uncore_has_discovery_tables(int *ignore)
>>  	return ret;
>>  }
>>  
>> +static bool intel_uncore_has_discovery_tables_msr(int *ignore)
>> +{
>> +	unsigned long *die_mask;
>> +	bool parsed = false;
>> +	int cpu, die;
>> +	u64 base;
>> +
>> +	die_mask = kcalloc(BITS_TO_LONGS(__uncore_max_dies),
> 
> Better use uncore_max_dies() instead of __uncore_max_dies.

They are the same thing, but the rest of the codes consistently use the
uncore_max_dies() to retrieve the number of the max dies.
I will update it in V4.

Thanks,
Kan>
> 
>> +			   sizeof(unsigned long), GFP_KERNEL);
>> +	if (!die_mask)
>> +		return false;
>> +
>> +	cpus_read_lock();
>> +	for_each_online_cpu(cpu) {
>> +		die = topology_logical_die_id(cpu);
>> +		if (__test_and_set_bit(die, die_mask))
>> +			continue;
>> +
>> +		if (rdmsrq_safe_on_cpu(cpu, UNCORE_DISCOVERY_MSR, &base))
>> +			continue;
>> +
>> +		if (!base)
>> +			continue;
>> +
>> +		__parse_discovery_table(base, die, &parsed, ignore);
>> +	}
>> +
>> +	cpus_read_unlock();
>> +
>> +	kfree(die_mask);
>> +	return parsed;
>> +}
>> +
>> +bool intel_uncore_has_discovery_tables(int *ignore)
>> +{
>> +	return intel_uncore_has_discovery_tables_msr(ignore) ||
>> +	       intel_uncore_has_discovery_tables_pci(ignore);
>> +}
>> +
>>  void intel_uncore_clear_discovery_tables(void)
>>  {
>>  	struct intel_uncore_discovery_type *type, *next;
>> diff --git a/arch/x86/events/intel/uncore_discovery.h b/arch/x86/events/intel/uncore_discovery.h
>> index 0e94aa7db8e7..690f737e6837 100644
>> --- a/arch/x86/events/intel/uncore_discovery.h
>> +++ b/arch/x86/events/intel/uncore_discovery.h
>> @@ -1,5 +1,8 @@
>>  /* SPDX-License-Identifier: GPL-2.0-only */
>>  
>> +/* Store the full address of the global discovery table */
>> +#define UNCORE_DISCOVERY_MSR			0x201e
>> +
>>  /* Generic device ID of a discovery table device */
>>  #define UNCORE_DISCOVERY_TABLE_DEVICE		0x09a7
>>  /* Capability ID for a discovery table device */
>