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

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

[Sumit Gupta]

On 26/08/26 12:00, Christian Loehle wrote:
> 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.
>
> 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.
>
> Performance Limited status is not required for CPPC control. If firmware
> describes it using an access which Linux cannot read safely, 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>
> ---

Performance Limited is readable but not clear-able on my test platform,
with a warning on every CPU:

   ACPI CPPC: CPU0: Performance Limited register cannot be cleared 
safely; keeping it readable

This occurs with both _CPC revision 3 and revision 4.

Could clearing remain supported?
cppc_set_perf_limited() now builds the W0C value without reading the
register first. Here the field is 2 bits at Bit Offset 0 in a 32-bit
access unit. If Section 4.6.1 allows bits 31:2 to be treated as
reserved status bits, the value could be issued as a single 32-bit write
with those bits zero. That avoids RMW.

A full-width description already writes zero to the reserved bits.
Is the same treatment valid for bits outside the GAS Bit Width when
Bit Offset is zero?

Apart from this, cppc_cpufreq probes and basic frequency scaling work
on this platform.

Thanks,
Sumit
....

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

[Christian Loehle]

On 8/26/26 14:59, Sumit Gupta wrote:
> 
> On 26/08/26 12:00, Christian Loehle wrote:
>> 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.
>>
>> 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.
>>
>> Performance Limited status is not required for CPPC control. If firmware
>> describes it using an access which Linux cannot read safely, 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>
>> ---
> 
> Performance Limited is readable but not clear-able on my test platform,
> with a warning on every CPU:
> 
>   ACPI CPPC: CPU0: Performance Limited register cannot be cleared safely; keeping it readable
> 
> This occurs with both _CPC revision 3 and revision 4.
> 
> Could clearing remain supported?
> cppc_set_perf_limited() now builds the W0C value without reading the
> register first. Here the field is 2 bits at Bit Offset 0 in a 32-bit
> access unit. If Section 4.6.1 allows bits 31:2 to be treated as
> reserved status bits, the value could be issued as a single 32-bit write
> with those bits zero. That avoids RMW.

Thanks. I agree that a single DWord write would avoid the RMW need but I
don't think we can safely infer that bits 31:2 are reserved.

Section 5.2.3.2 defines Register Bit Width as the size of the given
register, while Access Size only specifies the transaction size.
With Bit Width 2, Table 8.26 designates reserved bits only within that
described register, it does not make the remaining bits of the 32-bit
access unit part of Performance Limited. Those bits could contain
unrelated state which must (generally) be preserved.

> 
> A full-width description already writes zero to the reserved bits.
> Is the same treatment valid for bits outside the GAS Bit Width when
> Bit Offset is zero?
> 

The full-width case is different: Bit Width 32 makes bits 31:2 part of the
Performance Limited register, so Table 8.26 identifies them as reserved and
Section 4.6.1 permits writing zero.

Therefore I think the generic driver must keep a 2-bit/DWord descriptor
readable but not clearable. If this platform guarantees that bits 31:2
have no side effects, firmware should just describe the register with
Bit Width 32? Then the current code would clear it using one DWord write.

Does that make sense? Did I miss anything?

> Apart from this, cppc_cpufreq probes and basic frequency scaling work
> on this platform.
> 

Good to hear and thanks for testing!
(That being said, I'll drop your tags for v5 unfortunately as I have changes
in the middle of the series :/ )

Re: [PATCH v4 00/15] ACPI: CPPC: Fix register access and lifetime bugs

[Sumit Gupta]

On 26/08/26 12:00, Christian Loehle wrote:
> External email: Use caution opening links or attachments
>
>
> First of all, sorry this got so out of hand, initially this was just
> trying to fix some relatively simple issues found by sashiko in an
> earlier (unrelated) series.
> But with me touching more and more code and going through rounds of AI
> review that kept finding more and more pre-existing issues I've arrived
> at this.
>
> This series fixes correctness and robustness issues found while reviewing
> the CPPC control path. They affect malformed _CPC handling, error
> propagation, PCC ownership and cleanup, CPC object lifetime, register field
> access, cross-processor aliases, and Performance Limited clearing.
>
> Series structure
> ================
>
> Patches 1-8 are deliberately small, independently useful fixes. They
> validate the _CPC encoding consumed by cppc-acpi, propagate control-write
> errors, serialize PCC payload updates, correct 64-bit field masks, and fix
> descriptor and PCC lifetime handling.
>
> Patches 9-15 are the register-layout hardening portion. Geometry validation
> is more substantial because safe RMW and alias handling depend on the
> physical access unit, not merely on a logical _CPC entry or _PSD domain.
> These patches normalize and validate each supported address space before
> building probe-only physical interval registries. Keeping this work in the
> same posting gives the complete safety boundary and a single base for
> review, while each transport and bug retains its own Fixes provenance.
> Feel free to treat the two parts as independent series, I didn't split it
> because they're all technically fixes and to get Sashiko review for the
> whole lot.
>
> No interval lookup is added to the scheduler hot path. Full-width
> SystemMemory writes remain lockless. RMW locking remains necessary only for
> a partial field, where we must preserve the other bits in its access
> unit. The existing per-descriptor raw lock continues to cover disjoint
> partial fields within one _CPC package; probe rejects cross-descriptor
> layouts that it cannot protect.
>
> Parsing and control semantics
> =============================
>
> The parser now validates the package header before indexing it, bounds the
> BYTE and DWORD Integer forms before conversion, and validates the Generic
> Register descriptor consumed by cppc-acpi. NumEntries may not exceed the AML
> package count, but additional trailing package elements are ignored because
> doing so is safe and preserves compatibility with padded firmware. The parser
> likewise tolerates trailing ResourceTemplate data instead of imposing a new
> EndTag compatibility requirement.
>
> Writable controls must be Buffer-encoded registers. Minimum and Maximum
> Performance are checked as the pair required by ACPI 6.6 Sections
> 8.4.6.1.2.1 and 8.4.6.1.2.2. Object presence is kept separate from the
> Integer-zero convention for absent optional fields, so Lowest Performance
> may retain the valid abstract value zero.
>
> Performance Limited is one deliberate compatibility exception. ACPI lists
> it as required, but permits a platform with no limiting indication to
> always return zero, and deployed firmware represents that case with a NULL
> descriptor. CPPC control does not depend on this status register, so we
> continue to accept that encoding. A present _CPC package which otherwise
> fails parsing or initialization now emits an error instead of silently
> preventing cpufreq registration.
>
> Compound performance and EPP updates propagate errors and perform every
> fallible non-PCC write before modifying the PCC payload. Updates across
> address spaces cannot be atomic, but a known non-PCC failure can no longer
> commit only the PCC portion or leave an unsent value for a later command.
>
> SystemMemory locking and support boundary
> =========================================
>
> A partial SystemMemory field requires RMW to preserve the rest of its
> access unit. Commit 60949b7b8054 ("ACPI: CPPC: Fix MASK_VAL() usage") used
> a per-_CPC lock and noted that a global lock would be needed if physical
> registers were shared between packages.
>
> ACPI does not make _PSD a physical-register ownership boundary. Rather than
> put a global raw lock or lookup into the scheduler path, this series makes
> the cheaper per-descriptor model's assumptions enforceable at probe.
>
> Supported SystemMemory layouts are:
>
>    - naturally aligned 8-, 16-, 32-, and 64-bit access units;
>    - lockless full-width controls;
>    - read-only aliases;
>    - exact full-width writable aliases, including 64-bit aliases on 64-bit
>      kernels;
>    - disjoint partial writers within one descriptor, serialized by its
>      rmw_lock; and
>    - a partial writer sharing an access unit with a disjoint read-only
>      field, except Performance Limited.
>
> Probe rejects overlapping logical fields involving a writer, another field
> inside a full-width writable access unit, cross-descriptor partial writers,
> writers sharing Performance Limited's access unit, unaligned accesses, and
> exact writable 64-bit aliases on 32-bit kernels. These layouts were not
> safely supported by the old per-descriptor lock or generic writeq();
> rejecting them turns possible corruption into a visible probe failure rather
> than removing working support.
>
> PCC access and locking
> ======================
>
> The PCC protocol requires OSPM to acquire the subspace before changing its
> command or payload. Single-register and EPP updates now hold pcc_lock
> across ownership acquisition, payload staging, and command submission.
>
> ACPI 6.6's implementation example places a mandatory 32-bit Delivered
> Performance Counter at unaligned PCC offset 0x116. Performance controls may
> also use byte-multiple widths such as 24 bits. PCC therefore uses
> byte-oriented I/O with explicit little-endian encoding for zero-offset,
> byte-multiple fields from 8 through 64 bits. A short per-subspace payload
> lock protects concurrent aliased copies made under the shared side of
> pcc_lock; it does not replace the protocol ownership lock.
>
> Bit-level PCC fields require RMW and remain unsupported. An unsupported
> optional field is marked absent, but a present inaccessible CPPC Enable
> fails probe because OSPM must write it before using CPPC. Thus the
> ACPI-legal one-bit CPPC Enable used by the specification example is a
> documented kernel limitation. The old accessor could not program it
> correctly either, so an explicit error is safer than silently proceeding
> without enabling CPPC.
>
> Every retained PCC field is bounds checked against the shared-memory
> region. A subspace-keyed interval registry permits read-only overlap and
> exact same-control aliases while rejecting every other writable overlap
> across processors.
>
> SystemIO support boundary
> =========================
>
> SystemIO supports Bit Offset zero, naturally aligned, full 8-, 16-, or
> 32-bit accesses ending at or below port 0xffff, including legacy Access
> Size zero when Bit Width supplies the size. Partial fields never worked
> because the driver neither shifted them nor preserved adjacent bits, so
> they now fail visibly instead of being misprogrammed.
>
> On kernels without CONFIG_HAS_IOPORT, SystemIO entries are rejected or
> disabled according to the affected control's semantics. Runtime accessors
> also return -EOPNOTSUPP rather than treating an I/O port as a
> physical-memory address. A global port interval registry rejects
> cross-processor writable overlap.
>
> Write-only and Performance Limited controls
> ===========================================
>
> Between _CPC revisions 3 and 4, Desired Performance changed from
> Read/Write to Write, and revision 4 added write-only OSPM Nominal
> Performance. ACPI 6.6 Section 4.6.3 says reads from write-only positions
> are undefined. Explicit reads of both controls are rejected. Partial
> SystemMemory fields remain writable because RMW replaces every bit of the
> field and therefore does not propagate its undefined readback.
>
> Performance Limited is sticky, write-zero-to-clear, and requires
> interlocked accesses under ACPI 6.6 Section 8.4.6.1.3.2. The old separate
> read and write could clear a new event reported between transactions. The
> clear path now writes zero only to requested status bits and one to the
> other defined bits. Partial SystemMemory forms remain readable but cannot
> be cleared because a spinlock cannot interlock an enclosing RMW with
> platform updates. Probe also rejects another writable field sharing its
> access unit. QWord forms cannot be used on 32-bit kernels, where the MMIO
> accessor may be split into two 32-bit operations; naturally aligned,
> full-width QWords remain supported on 64-bit kernels. Since CPPC control
> does not depend on Performance Limited status, an unreadable description
> disables that status register instead of rejecting the processor's
> otherwise usable _CPC.
>
> Lifetime and cleanup
> ====================
>
> CPC descriptors are released through their kobject callback, keeping their
> storage and mappings alive for outstanding sysfs references. Every PCC
> allocation, reference, and acquired channel is unwound on probe failure,
> and the per-CPU PCC index is initialized before every early return. PCC
> allocation uses a separate temporary result, so its success cannot turn a
> later parse failure into a successful probe return.
>
> Changes since v3
> ================
>
>    - Allowed partial SystemMemory Desired and OSPM Nominal controls when RMW
>      discards their undefined readback, supporting NVIDIA's separate 9-bit
>      controls in _CPC revision 4.
>    - Reported an unavailable Desired Performance control as unsupported from
>      the common getter instead of returning a synthetic zero.
>    - Required natural alignment for SystemIO access units, preventing faults
>      on architectures which implement port I/O through Device-memory MMIO.
>    - Kept partial Performance Limited fields readable but not clearable,
>      rejected another writer sharing their access unit, reported fully
>      inaccessible forms as unsupported instead of returning a synthetic
>      zero, and consolidated each nonfatal fallback into a single warning.
>
> Changes since v2
> ================
>
>    - Relaxed the exact NumEntries/package-count match to tolerate safe trailing
>      package elements while still rejecting any count that could cause an
>      out-of-bounds walk.
>    - Made patch 10 independently preserve immutable-autonomous setups whose
>      inaccessible Desired Performance register requires RMW, rather than
>      relying on patch 11 to restore that exception.
>
> Sashiko v2 review not addressed
> ===============================
>
>    - Kept Guaranteed Performance Buffer-only. The suggestion was to accept a
>      nonzero Integer, but ACPI 6.6 Table 8.23 permits only a Buffer for this
>      entry.
>
> Deferred follow-up work
> =======================
>
> Sashiko also identified a broader pre-existing lifetime question which this
> series does not attempt to solve. In-kernel accessors read the per-CPU
> cpc_desc_ptr without acquiring a reference, while processor teardown can
> unpublish and eventually release the descriptor and its PCC data. The kobject
> change here fixes the concrete sysfs lifetime bug, but a NULL pcc_data check
> would not protect a caller which already holds a stale pointer. Closing this
> properly requires defining the kernel accessor lifetime contract and then
> using CPU-hotplug serialization / safe referencing across all callers,
> therefore will be handled by a follow-up.
>
> ACPI-legal bit-level PCC and SystemIO fields also remain unsupported. In
> particular, the ACPI example's one-bit PCC CPPC Enable register cannot be
> implemented by the old whole-value accessors. Supporting these descriptions
> requires transport-specific field extraction and an RMW operation which obeys
> PCC ownership or safely preserves adjacent SystemIO bits, just accepting the
> descriptors would silently program the wrong value. Therefore continue to
> disable optional inaccessible fields where safe and reject a present
> inaccessible CPPC Enable control.
> Full support, if even needed, belongs in a separate follow-up.
>
> The review additionally suggested validating the complete AML
> ResourceTemplate, including its EndTag. We currently validate the Register
> descriptor we consume and tolerate trailing firmware data. I don't really
> see the point of ever doing this, but definitely not in this series,
> where I'm trying to guarantee that no reasonably working platform is
> regressing.
>
> Patches 1, 2, 4-7, and 9 address findings reported by Sashiko while
> reviewing:
>
>    https://sashiko.dev/#/patchset/20260724134251.1632824-1-christian.loehle%40arm.com
>
> Patches 3, 5, 6, 9, 10, and 15 address findings from the follow-up review:
>
>    https://sashiko.dev/#/patchset/20260807111303.1062391-1-christian.loehle%40arm.com
>
> Patches 1 and 10 address findings from the v2 review:
>
>    https://sashiko.dev/#/patchset/20260808082644.1251332-1-christian.loehle%40arm.com
>
> Christian Loehle (15):
>    ACPI: CPPC: Validate the _CPC package header
>    ACPI: CPPC: Validate _CPC entry and control semantics
>    ACPI: CPPC: Propagate performance-control write errors
>    ACPI: CPPC: Use 64-bit masks for register fields
>    ACPI: CPPC: Serialize PCC single-register payload updates
>    ACPI: CPPC: Serialize PCC EPP payload updates
>    ACPI: CPPC: Release CPC descriptors through kobject
>    ACPI: CPPC: Release PCC data after probe failures
>    ACPI: CPPC: Reject unsafe cross-CPU SystemMemory RMW
>    ACPI: CPPC: Reject direct reads of write-only controls
>    ACPI: CPPC: Validate and access PCC register layouts
>    ACPI: CPPC: Validate SystemIO register layouts
>    ACPI: CPPC: Validate PCC overlaps across processors
>    ACPI: CPPC: Validate SystemIO overlaps across processors
>    ACPI: CPPC: Clear Performance Limited without a stale read
>
>   drivers/acpi/cppc_acpi.c | 1313 +++++++++++++++++++++++++++++++++++++++-------
>   include/acpi/cppc_acpi.h |    8 +-
>   2 files changed, 1131 insertions(+), 190 deletions(-)
>
> base-commit: 0a0d1d55dad570724bf8c7ea83409639cfb4be9b
> --
> 2.34.1

For the entire series, except patch 15 where I replied separately:

Tested-by: Sumit Gupta <sumitg@nvidia.com>

Thanks,
Sumit
....