Re: [PATCH v6 02/15] ACPI: CPPC: Validate _CPC entry and control semantics

On 9/3/26 20:44, Rafael J. Wysocki (Intel) wrote:
> On Thu, Sep 3, 2026 at 9:27 PM Rafael J. Wysocki (Intel)
> <[email protected]> wrote:
>>
>> On Sun, Aug 30, 2026 at 3:32 PM Christian Loehle
>> <[email protected]> wrote:
>>>
>>> On 8/30/26 12:56, Christian Loehle wrote:
>>>> Writable _CPC controls are Register descriptors encoded as Buffer objects.
>>>> Integer entries represent fixed values or unsupported optional registers;
>>>> Autonomous Selection Integer 1 is the special immutable form which enables
>>>> operation without Desired Performance.
>>>>
>>>> The parser accepts arbitrary object types and cpc_write() assumes that its
>>>> argument contains a GAS. Malformed firmware can therefore make it interpret
>>>> an Integer union member as a register.
>>>>
>>>> Validate the portion of each encoding consumed by the driver: bound Integer
>>>> DWORD forms to 32 bits, and require Buffer entries to start with a complete
>>>> Generic Register descriptor with the expected header. Continue tolerating
>>>> Integer 0 for an absent optional register and retain type checks in
>>>> cpc_write() as defense in depth. Reject an attempt to disable immutable
>>>> Autonomous Selection instead of silently applying only the EPP part of the
>>>> request.
>>>>
>>>> Capability registers are read into u64 temporaries but exposed through u32
>>>> fields. Reject values above U32_MAX instead of allowing them to be
>>>> truncated. In particular, a truncated Highest Performance value can become
>>>> a zero divisor in the performance-to-frequency conversion. Enforce the
>>>> required ordering from Highest through Nominal, Lowest Nonlinear, and
>>>> Lowest Performance, and constrain a present Guaranteed Performance to the
>>>> inclusive Lowest-to-Nominal range. Also reject reversed frequency anchors
>>>> and unequal frequency anchors with identical performance anchors. Those
>>>> invalid tuples otherwise make affine-conversion differences wrap or divide
>>>> by zero.
>>>>
>>>> Check mandatory object presence separately from the Integer-zero convention
>>>> for absent optional fields. ACPI does not reserve zero in the abstract
>>>> Lowest Performance scale, so accept a present Lowest Performance DWORD of
>>>> zero when distinct frequency anchors provide a usable nonzero physical
>>>> minimum. Retain the old rejection when that mapping is unavailable and the
>>>> fallback conversion would expose a 0 kHz cpufreq endpoint.
>>>>
>>>> Minimum Performance also defines zero as a real no-limit value, but the
>>>> exported cppc_set_perf() interface historically used zero to omit a bound.
>>>> Add explicit validity flags so callers can request zero without changing
>>>> that legacy convention. Populate the flags when reading the controls and
>>>> mark the bounds supplied by amd-pstate explicitly.
>>>>
>>>> Performance Limited is listed as a required Buffer, but the interface does
>>>> not depend on it to control performance and the specification permits a
>>>> platform with no limiting indication to always report zero. Preserve
>>>> the compatibility with firmware that represents that case using a NULL
>>>> register descriptor instead of disabling CPPC entirely.
>>>>
>>>> Emit an error when a present _CPC package fails parsing or initialization
>>>> so such firmware and resource failures no longer silently suppress cpufreq.
>>>> Initialize malformed-package failures to -EINVAL and preserve specific
>>>> allocation, mapping, and unsupported-access errors in that diagnostic.
>>>>
>>>> Fixes: 337aadff8e45 ("ACPI: Introduce CPU performance controls using CPPC")
>>>> Reported-by: Sashiko <[email protected]>
>>>> Link: https://sashiko.dev/#/patchset/20260724134251.1632824-1-christian.loehle%40arm.com
>>>> Signed-off-by: Christian Loehle <[email protected]>
>>>> ---
>>>>  drivers/acpi/cppc_acpi.c     | 172 ++++++++++++++++++++++++++++++-----
>>>>  drivers/cpufreq/amd-pstate.c |  12 ++-
>>>>  include/acpi/cppc_acpi.h     |   2 +
>>>>  3 files changed, 159 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
>>>> index 3b8cdf88e31d..6f3ffa4a1845 100644
>>>> --- a/drivers/acpi/cppc_acpi.c
>>>> +++ b/drivers/acpi/cppc_acpi.c
>>>> @@ -129,6 +129,21 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>>>                               !!(cpc)->cpc_entry.int_value :          \
>>>>                               !IS_NULL_REG(&(cpc)->cpc_entry.reg))
>>>>
>>>> +static bool cpc_is_writable(const struct cpc_register_resource *cpc)
>>>> +{
>>>> +     return cpc->type == ACPI_TYPE_BUFFER &&
>>>> +            !IS_NULL_REG(&cpc->cpc_entry.reg);
>>>> +}
>>>> +
>>>> +static bool cpc_entry_present(const struct cpc_register_resource *cpc)
>>>> +{
>>>> +     if (cpc->type == ACPI_TYPE_INTEGER)
>>>> +             return true;
>>>> +
>>>> +     return cpc->type == ACPI_TYPE_BUFFER &&
>>>> +            !IS_NULL_REG(&cpc->cpc_entry.reg);
>>>> +}
>>>> +
>>>>  /*
>>>>   * Each bit indicates the optionality of the register in per-cpu
>>>>   * cpc_regs[] with the corresponding index. 0 means mandatory and 1
>>>> @@ -142,6 +157,29 @@ static DEFINE_PER_CPU(struct cpc_desc *, cpc_desc_ptr);
>>>>   */
>>>>  #define IS_OPTIONAL_CPC_REG(reg_idx) (REG_OPTIONAL & (1U << (reg_idx)))
>>>>
>>>> +static bool cpc_integer_entry_valid(unsigned int reg_idx, u64 value)
>>>> +{
>>>> +     switch (reg_idx) {
>>>> +     case HIGHEST_PERF:
>>>> +     case NOMINAL_PERF:
>>>> +     case LOW_NON_LINEAR_PERF:
>>>> +     case LOWEST_PERF:
>>>> +     case CTR_WRAP_TIME:
>>>> +     case REFERENCE_PERF:
>>>> +     case LOWEST_FREQ:
>>>> +     case NOMINAL_FREQ:
>>>> +             return value <= U32_MAX;
>>>
>>>
>>> Sashiko:
>>> "Does this incorrectly restrict the counter wraparound time to 32 bits?
>>> The ACPI specification allows firmware to provide a 64-bit QWord integer
>>> for the Counter Wraparound Time. The cppc_perf_fb_ctrs structure already
>>> models this as a 64-bit value internally
>>> If firmware provides a valid 64-bit integer exceeding U32_MAX for this
>>> register, cpc_integer_entry_valid() will return false and completely abort
>>> CPPC initialization for the CPU. Can we remove this restriction for
>>> CTR_WRAP_TIME?"
>>
>>> This is true. ACPI spec 6.6 and 6.5 (Table 8.23) describe it as
>>> Integer (DWORD) or Buffer
>>> The 64-bit internal representation is only for the case of firmware
>>> providing it as Buffer.
> 
> Sashiko is right, the table in the spec is wrong.  If it is Integer,
> it is 64-bit.
> 
> The size of an Integer in ASL cannot be restricted.

Duh, thanks!

> 
>>>
>>>> +     case AUTO_SEL_ENABLE:
>>>> +             return value <= 1;
>>>> +     case DESIRED_PERF:
>>>> +             /* Validated against Autonomous Selection after parsing. */
>>>> +             return value == 0;
>>>> +     default:
>>>> +             /* Tolerate the customary Integer 0 for an absent option. */
>>>> +             return value == 0 && IS_OPTIONAL_CPC_REG(reg_idx);
>>>> +     }
>>>> +}
>>>> +
>>> Sashiko:
>>> "Does this incorrectly restrict the counter wraparound time to 32 bits?
>>> The ACPI specification allows firmware to provide a 64-bit QWord integer
>>> for the Counter Wraparound Time. The cppc_perf_fb_ctrs structure already
>>> models this as a 64-bit value internally.
>>> If firmware provides a valid 64-bit integer exceeding U32_MAX for this
>>> register, cpc_integer_entry_valid() will return false and completely abort
>>> CPPC initialization for the CPU. Can we remove this restriction for
>>> CTR_WRAP_TIME?"
>>
>> It looks like you pasted the same comment twice.  Or did Sashiko hallucinate?
> 
> Well, its other comment is actually different from the first one.  Let
> me paste it:

Apparently I hallucinated :)

> 
> Will this strict rejection break CPPC initialization on compliant firmware
> that provides non-zero integers for other optional capabilities?
> For capability registers not explicitly listed in the switch statement
> above, such as GUARANTEED_PERF or TIME_WINDOW, the ACPI 6.5 specification
> explicitly allows platforms to provide fixed non-zero values encoded as
> Integer objects.
> 
> When firmware provides a valid non-zero integer for these optional
> registers, this default case enforces that the value must be zero. Since
> acpi_cppc_processor_probe() fails and returns -EINVAL when this returns
> false, it will completely disable cpufreq and CPPC support for the CPU.
> Should we allow non-zero integer values for these other optional registers?
> 
>>> GUARANTEED_PERF and TIME_WINDOW: both are Buffer-only Register descriptors.
>>> Nonzero Integer encodings are invalid and Integer 0 is tolerated because
>>> the previous parser allowed it too. I don't know of any platform describing
>>> this myself.
> 
> Strictly speaking Integer 0 is not allowed, see
> 
> https://uefi.org/specs/ACPI/6.6/08_Processor_Configuration_and_Control.html#cpc-continuous-performance-control

I'm happy to reject it too, I was just being careful here because I only have
a tiny subset of ACPI platforms to test this on with and it's for -fixes

> [snip]