Re: [PATCH v6 15/15] ACPI: CPPC: Clear Performance Limited without a stale read

On 9/7/26 11:33, Sumit Gupta wrote:

Hi Christian,

Continuing the discussion from v4 [1].


On 30/08/26 17:26, Christian Loehle wrote:

quoted

External email: Use caution opening links or attachments


The Performance Limited status bits are sticky and write-zero-to-clear.
ACPI 6.6 Section 8.4.6.1.3.2 also requires both entities to use interlocked
accesses.

cppc_set_perf_limited() currently reads the register, computes a new value,
and writes it in a separate transaction. If the platform reports another
excursion between those transactions, the stale write can clear that new
event.

Write zero to the requested bits and one to the other defined status bits
directly. Keep reserved bits zero as required for hardware status registers
by ACPI 6.6 Section 4.6.1. This removes the stale read window.

A partial SystemMemory field would still make the generic writer perform a
read-modify-write to preserve the containing access unit. The
per-descriptor spinlock cannot interlock that RMW with platform updates, so
reject clears of such a field. Keep the descriptor mapped and readable,
because reading the containing access unit once and extracting the field
does not require RMW.

Classify a field as a writer during overlap validation only when its _CPC
semantics permit writes and its validated resource remains writable. This
allows partial Performance Limited fields whose clear path was disabled to
share an access unit with other read-only fields, while still rejecting an
actual writer in that access unit.

Also reject another writable SystemMemory field sharing Performance
Limited's access unit. Its RMW could similarly replay stale status bits,
and an OSPM lock cannot serialize against the platform.

Also reject 64-bit SystemMemory descriptions on 32-bit kernels, where
generic readq()/writeq() may be split into two 32-bit operations and cannot
provide the required portable interlocked access. A naturally aligned
full-width QWord remains supported on 64-bit kernels, where the
architecture provides a native 64-bit MMIO accessor.

Retain any inaccessible Performance Limited descriptor whose conservative
physical range is still locatable, while marking both reads and writes
unsupported. This includes a QWord on a 32-bit kernel. Skip its mapping and
the flexible-address-space capability gate, because Linux will issue no
access, without hiding the asynchronous status range from
neighbouring-writer validation. Both the interval registry and pairwise
overlap test use the larger of the access unit and logical field span, so a
malformed field extending beyond its nominal access unit remains covered.

Performance Limited status is not required for CPPC control. If firmware
describes it without even a locatable physical range, disable that status
register instead of rejecting the processor's otherwise usable _CPC
package. Report reads as unsupported rather than returning a synthetic
zero, and emit a single warning for each nonfatal fallback.

Fixes: 13c45a26635f ("ACPI: CPPC: add APIs and sysfs interface for perf_limited")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260807111303.1062391-1-christian.loehle%40arm.com
Signed-off-by: Christian Loehle <christian.loehle@arm.com>
---
  drivers/acpi/cppc_acpi.c | 76 +++++++++++++++++++++++++++++-----------
  1 file changed, 56 insertions(+), 20 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 25bccd4cfb34..a07440ed7f80 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c

@@ -443,6 +443,22 @@ static int cpc_validate_sysmem_reg(struct cpc_desc *cpc_desc,

         if (!cpc_reg_access_aligned(gas, access_size))
                 goto invalid;

+       if (reg_idx == PERF_LIMITED) {
+               if (access_width == 64 && !IS_ENABLED(CONFIG_64BIT)) {
+                       pr_warn("CPU%d: Performance Limited register cannot be accessed atomically; keeping its range reserved\n",
+                               cpc_desc->cpu_id);
+                       cpc_desc->cpc_regs[reg_idx].cpc_entry.read_unsupported = true;
+                       cpc_desc->cpc_regs[reg_idx].cpc_entry.write_unsupported = true;
+                       return 0;
+               }
+
+               if (gas->bit_offset || gas->bit_width != access_width) {
+                       pr_warn("CPU%d: Performance Limited register cannot be cleared safely; keeping it readable\n",
+                               cpc_desc->cpu_id);
+                       cpc_desc->cpc_regs[reg_idx].cpc_entry.write_unsupported = true;
+               }
+       }

Agreed on the generic behavior. With Bit Width 2 the remaining bits are
not part of the register, and the driver cannot treat them as reserved.

On the firmware option, I confirmed with the hardware team that
bits 31:2 here are unimplemented. They read as zero, have no side
effects when written, and are unused elsewhere.
Future firmware can describe the register with Bit Width 32, but systems
already shipped cannot be updated. For those I have a patch which widens
the descriptor to the access width, so the clear becomes the single
DWord write you describe. It is pasted below and same attached.

OK, good.
I'll start carrying a similar quirk like you proposed below on
"NVIDIA", "T41     ", affected_revision, ACPI_SIG_DSDT,
in the next version.

Testing with that applied uncovered a second issue.

The commit description says the status bits are write-zero-to-clear.
I could not find where that is specified, have I missed something?
ACPI spec describes the register as Read/Write and requires interlocked
operations, which reads as an expectation of read-modify-write,
but I found nothing defining what a written one does.

Ah right :/
Yeah that's really unfortunate then...

Here a written one sets the bit, and the hardware team confirmed the
register is plain Read/Write on my test platform.
Writing CPPC_PERF_LIMITED_MASK & ~bits_to_clear therefore sets the bit
which is not being cleared, and Linux reports an excursion the platform
never signalled:

 #cat /sys/devices/system/cpu/cpu0/cpufreq/perf_limited
 0
 #echo 0x1 > /sys/devices/system/cpu/cpu0/cpufreq/perf_limited
 #cat /sys/devices/system/cpu/cpu0/cpufreq/perf_limited
 2

The read before the write avoided this, at the cost of the stale read
race you describe. Clearing both bits would still need no read at all,
because the value written is zero in either case.

How would you prefer to handle that?

Yes that's a good question.
I think we should just not support partial clear then.

Reading the spec and given that these are supposed to be short-term
excursions I wouldn't think that keeping the register as read-only makes
much sense, therefore just support full clear and document the race window.
I think dropping the stale read still makes sense, even if it widens the
window, it'll be there anyway.

But I'm happy for Rafael and you to chime in, too! 
Do you have any userspace actually caring about Performance Limited?
What would be the most sane behaviour?
and thanks for testing!

[snip]