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

[Rafael J. Wysocki (Intel)]

On Thu, Sep 3, 2026 at 10:02 PM Christian Loehle
<christian.loehle@xxxxxxx> wrote:
>
> On 9/3/26 20:44, Rafael J. Wysocki (Intel) wrote:
> > On Thu, Sep 3, 2026 at 9:27 PM Rafael J. Wysocki (Intel)
> > <rafael@xxxxxxxxxx> wrote:
> >>
> >> On Sun, Aug 30, 2026 at 3:32 PM Christian Loehle
> >> <christian.loehle@xxxxxxx> 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 <sashiko-bot@xxxxxxxxxx>
> >>>> Link: https://sashiko.dev/#/patchset/20260724134251.1632824-1-christian.loehle%40arm.com
> >>>> Signed-off-by: Christian Loehle <christian.loehle@xxxxxxx>
> >>>> ---
> >>>>  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

Instead of rejecting it entirely, you may log an FW_BUG message when
it is used, to avoid breaking legacy platforms that possibly used it.