| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <17caf75c-a00f-41d8-bacc-af5ba6c485d9@arm.com>
Date: Mon, 10 Nov 2025 16:28:10 +0000
From: Ben Horgan <ben.horgan@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: james.morse@....com, amitsinght@...vell.com, baisheng.gao@...soc.com,
baolin.wang@...ux.alibaba.com, bobo.shaobowang@...wei.com,
carl@...amperecomputing.com, catalin.marinas@....com, dakr@...nel.org,
dave.martin@....com, david@...hat.com, dfustini@...libre.com,
fenghuay@...dia.com, gregkh@...uxfoundation.org, gshan@...hat.com,
guohanjun@...wei.com, jeremy.linton@....com, kobak@...dia.com,
lcherian@...vell.com, lenb@...nel.org, linux-acpi@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
lpieralisi@...nel.org, peternewman@...gle.com, quic_jiles@...cinc.com,
rafael@...nel.org, robh@...nel.org, rohit.mathew@....com,
scott@...amperecomputing.com, sdonthineni@...dia.com, sudeep.holla@....com,
tan.shaopeng@...itsu.com, will@...nel.org, xhao@...ux.alibaba.com
Subject: Re: [PATCH 03/33] ACPI / PPTT: Add acpi_pptt_cache_v1_full to use
pptt cache as one structure
Hi Jonathan,
On 11/10/25 15:46, Jonathan Cameron wrote:
> On Fri, 7 Nov 2025 12:34:20 +0000
> Ben Horgan <ben.horgan@....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@....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
Powered by blists - more mailing lists