Re: [PATCH 03/33] ACPI / PPTT: Add acpi_pptt_cache_v1_full to use pptt cache as one structure

[Ben Horgan <ben.horgan@arm.com>]

Hi Jonathan,

On 11/10/25 15:46, Jonathan Cameron wrote:
> On Fri, 7 Nov 2025 12:34:20 +0000
> Ben Horgan <ben.horgan@arm.com> wrote:
> 
>> In actbl2.h, struct acpi_pptt_cache describes the fields in the original
>> cache type structure. In PPTT table version 3 a new field was added at the
>> end, cache_id. This is described in struct acpi_pptt_cache_v1. Introduce
>> the new, acpi_pptt_cache_v1_full to contain both these structures. Update
>> the existing code to use this new struct. This simplifies the code, removes
>> a non-standard use of ACPI_ADD_PTR and allows using the length in the
>> header to check if the cache_id is valid.
>>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> 
> Whilst I wish the ACPICA stuff did structures like this, I'm not sure
> if the ACPI maintainers will feel it is appropriate to work around it
> with generic sounding structures like this one.
> 
> I'd also say that we should only cast it to your _full structure
> if we know we have rev 3 of PPTT.  Otherwise we should continue manipulating
> it as a struct acpi_pptt_cache

Fair enough. My thinking was that you had to check the valid flag anyway
to use cache_id but it's less robust. I'll delay the casting to later
which IIUC is what Jeremy Linton suggested offline.

> 
>> ---
>> Changes since v3:
>> New patch
>> ---
>>  drivers/acpi/pptt.c | 104 ++++++++++++++++++++++++--------------------
>>  1 file changed, 58 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/acpi/pptt.c b/drivers/acpi/pptt.c
>> index 1027ca3566b1..1ed2099c0d1a 100644
>> --- a/drivers/acpi/pptt.c
>> +++ b/drivers/acpi/pptt.c
>> @@ -21,6 +21,11 @@
>>  #include <linux/cacheinfo.h>
>>  #include <acpi/processor.h>
>>  
>> +struct acpi_pptt_cache_v1_full {
>> +	struct acpi_pptt_cache		f;
>> +	struct acpi_pptt_cache_v1	extra;
>> +} __packed;
> 
>> +#define ACPI_PPTT_CACHE_V1_LEN sizeof(struct acpi_pptt_cache_v1_full)
>> +
>> +/*
>> + * From PPTT table version 3, a new field cache_id was added at the end of
>> + * the cache type structure.  We now use struct acpi_pptt_cache_v1_full,
>> + * containing the cache_id, everywhere but must check validity before accessing
>> + * the cache_id.
>> + */
>> +static bool acpi_pptt_cache_id_is_valid(struct acpi_pptt_cache_v1_full *cache)
>> +{
>> +	return (cache->f.header.length >= ACPI_PPTT_CACHE_V1_LEN &&
> 
> Although I later say I don't think you should pass a v1_full structure in here (as
> we don't know it is at least that large until after this check) if you do keep this
> why not use sizeof(*cache) and get rid of the V1_LEN definition as providing no obvious
> value here?

Yes, the define was never needed.

> 
>> +		cache->f.flags & ACPI_PPTT_CACHE_ID_VALID);
>>  }
> 
>> @@ -355,7 +374,6 @@ static struct acpi_pptt_cache *acpi_find_cache_node(struct acpi_table_header *ta
>>   * @this_leaf: Kernel cache info structure being updated
>>   * @found_cache: The PPTT node describing this cache instance
>>   * @cpu_node: A unique reference to describe this cache instance
>> - * @revision: The revision of the PPTT table
>>   *
>>   * The ACPI spec implies that the fields in the cache structures are used to
>>   * extend and correct the information probed from the hardware. Lets only
>> @@ -364,23 +382,20 @@ static struct acpi_pptt_cache *acpi_find_cache_node(struct acpi_table_header *ta
>>   * Return: nothing. Side effect of updating the global cacheinfo
>>   */
>>  static void update_cache_properties(struct cacheinfo *this_leaf,
>> -				    struct acpi_pptt_cache *found_cache,
>> -				    struct acpi_pptt_processor *cpu_node,
>> -				    u8 revision)
>> +				    struct acpi_pptt_cache_v1_full *found_cache,
>> +				    struct acpi_pptt_processor *cpu_node)
>>  {
>> -	struct acpi_pptt_cache_v1* found_cache_v1;
>> -
>>  	this_leaf->fw_token = cpu_node;
>> -	if (found_cache->flags & ACPI_PPTT_SIZE_PROPERTY_VALID)
>> -		this_leaf->size = found_cache->size;
>> -	if (found_cache->flags & ACPI_PPTT_LINE_SIZE_VALID)
>> -		this_leaf->coherency_line_size = found_cache->line_size;
>> -	if (found_cache->flags & ACPI_PPTT_NUMBER_OF_SETS_VALID)
>> -		this_leaf->number_of_sets = found_cache->number_of_sets;
>> -	if (found_cache->flags & ACPI_PPTT_ASSOCIATIVITY_VALID)
>> -		this_leaf->ways_of_associativity = found_cache->associativity;
>> -	if (found_cache->flags & ACPI_PPTT_WRITE_POLICY_VALID) {
>> -		switch (found_cache->attributes & ACPI_PPTT_MASK_WRITE_POLICY) {
>> +	if (found_cache->f.flags & ACPI_PPTT_SIZE_PROPERTY_VALID)
>> +		this_leaf->size = found_cache->f.size;
>> +	if (found_cache->f.flags & ACPI_PPTT_LINE_SIZE_VALID)
>> +		this_leaf->coherency_line_size = found_cache->f.line_size;
>> +	if (found_cache->f.flags & ACPI_PPTT_NUMBER_OF_SETS_VALID)
>> +		this_leaf->number_of_sets = found_cache->f.number_of_sets;
>> +	if (found_cache->f.flags & ACPI_PPTT_ASSOCIATIVITY_VALID)
>> +		this_leaf->ways_of_associativity = found_cache->f.associativity;
>> +	if (found_cache->f.flags & ACPI_PPTT_WRITE_POLICY_VALID) {
>> +		switch (found_cache->f.attributes & ACPI_PPTT_MASK_WRITE_POLICY) {
>>  		case ACPI_PPTT_CACHE_POLICY_WT:
>>  			this_leaf->attributes = CACHE_WRITE_THROUGH;
>>  			break;
>> @@ -389,8 +404,8 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>>  			break;
>>  		}
>>  	}
>> -	if (found_cache->flags & ACPI_PPTT_ALLOCATION_TYPE_VALID) {
>> -		switch (found_cache->attributes & ACPI_PPTT_MASK_ALLOCATION_TYPE) {
>> +	if (found_cache->f.flags & ACPI_PPTT_ALLOCATION_TYPE_VALID) {
>> +		switch (found_cache->f.attributes & ACPI_PPTT_MASK_ALLOCATION_TYPE) {
>>  		case ACPI_PPTT_CACHE_READ_ALLOCATE:
>>  			this_leaf->attributes |= CACHE_READ_ALLOCATE;
>>  			break;
>> @@ -415,13 +430,11 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>>  	 * specified in PPTT.
>>  	 */
>>  	if (this_leaf->type == CACHE_TYPE_NOCACHE &&
>> -	    found_cache->flags & ACPI_PPTT_CACHE_TYPE_VALID)
>> +	    found_cache->f.flags & ACPI_PPTT_CACHE_TYPE_VALID)
>>  		this_leaf->type = CACHE_TYPE_UNIFIED;
>>  
>> -	if (revision >= 3 && (found_cache->flags & ACPI_PPTT_CACHE_ID_VALID)) {
>> -		found_cache_v1 = ACPI_ADD_PTR(struct acpi_pptt_cache_v1,
>> -	                                      found_cache, sizeof(struct acpi_pptt_cache));
>> -		this_leaf->id = found_cache_v1->cache_id;
>> +	if (acpi_pptt_cache_id_is_valid(found_cache)) {
> 
> Only here do we know that found_cache is the _full type. 
> 
>> +		this_leaf->id = found_cache->extra.cache_id;
>>  		this_leaf->attributes |= CACHE_ID;
>>  	}
>>  }
>> @@ -429,7 +442,7 @@ static void update_cache_properties(struct cacheinfo *this_leaf,
>>  static void cache_setup_acpi_cpu(struct acpi_table_header *table,
>>  				 unsigned int cpu)
>>  {
>> -	struct acpi_pptt_cache *found_cache;
>> +	struct acpi_pptt_cache_v1_full *found_cache;
> 
> This isn't necessarily valid. Until deep in update_cache_properties() we don't care about the ID
> so this structure may be smaller than this implies.
> 
>>  	struct cpu_cacheinfo *this_cpu_ci = get_cpu_cacheinfo(cpu);
>>  	u32 acpi_cpu_id = get_acpi_id_for_cpu(cpu);
>>  	struct cacheinfo *this_leaf;
>> @@ -445,8 +458,7 @@ static void cache_setup_acpi_cpu(struct acpi_table_header *table,
>>  		pr_debug("found = %p %p\n", found_cache, cpu_node);
>>  		if (found_cache)
>>  			update_cache_properties(this_leaf, found_cache,
>> -						ACPI_TO_POINTER(ACPI_PTR_DIFF(cpu_node, table)),
>> -						table->revision);
>> +						ACPI_TO_POINTER(ACPI_PTR_DIFF(cpu_node, table)));
>>  
>>  		index++;
>>  	}
> 

Thanks,

Ben