Re: [PATCH] x86/ACPI: _PDC bits vs HWP/CPPC

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Mar 04, 2026 at 03:37:25PM +0100, Jan Beulich wrote:
> The treatment of ACPI_PDC_CPPC_NATIVE_INTR should follow that of other P-
> state related bits. Add the bit to ACPI_PDC_P_MASK and apply "mask" in
> arch_acpi_set_pdc_bits() when setting that bit. Move this next to the
> other P-state related logic.
> 
> Further apply ACPI_PDC_P_MASK also when the amd-cppc driver is in use.
> 
> Also leave a comment regarding the clearing of bits and add a couple of
> blank lines.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> Including XEN_PROCESSOR_PM_CPPC may need accompanying with some change to
> arch_acpi_set_pdc_bits(), but it's entirely unclear to me what to do
> there. I'm unaware of an AMD counterpart of Intel's "Intel® Processor
> Vendor-Specific ACPI". Plus even when the powernow driver is in use, we
> never set any bits, as EIST is an Intel-only feature.

We possibly never need to set any bits there for AMD, as those _PDC
Processor bits are Intel specific?

> acpi_set_pdc_bits() having moved to the cpufreq driver looks to have been
> a mistake. It covers not only P-state related bits, but also C-state and
> T-state ones. (This is only a latent issue as long as
> https://lists.xen.org/archives/html/xen-devel/2026-02/msg00875.html
> wouldn't land.)
> 
> --- a/xen/arch/x86/acpi/lib.c
> +++ b/xen/arch/x86/acpi/lib.c
> @@ -124,6 +124,9 @@ int arch_acpi_set_pdc_bits(u32 acpi_id,
>       if (cpu_has(c, X86_FEATURE_EIST))
>               pdc[2] |= ACPI_PDC_EST_CAPABILITY_SWSMP & mask;
>  
> +     if (hwp_active())
> +             pdc[2] |= ACPI_PDC_CPPC_NATIVE_INTR & mask;
> +
>       if (cpu_has(c, X86_FEATURE_ACPI))
>               pdc[2] |= ACPI_PDC_T_FFH & mask;
>  
> @@ -142,8 +145,5 @@ int arch_acpi_set_pdc_bits(u32 acpi_id,
>           !(ecx & CPUID5_ECX_INTERRUPT_BREAK))
>               pdc[2] &= ~(ACPI_PDC_C_C1_FFH | ACPI_PDC_C_C2C3_FFH);
>  
> -     if (hwp_active())
> -             pdc[2] |= ACPI_PDC_CPPC_NATIVE_INTR;
> -
>       return 0;
>  }
> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -694,14 +694,23 @@ int acpi_set_pdc_bits(unsigned int acpi_
>      {
>          uint32_t mask = 0;
>  
> +        /*
> +         * Accumulate all the bits under Xen's control, to mask them off, for
> +         * arch_acpi_set_pdc_bits() to then set those we want set.
> +         */
>          if ( xen_processor_pmbits & XEN_PROCESSOR_PM_CX )
>              mask |= ACPI_PDC_C_MASK | ACPI_PDC_SMP_C1PT;
> -        if ( xen_processor_pmbits & XEN_PROCESSOR_PM_PX )
> +
> +        if ( xen_processor_pmbits &
> +             (XEN_PROCESSOR_PM_PX | XEN_PROCESSOR_PM_CPPC) )

Currently the CPPC driver is AMD only, and hence when using it we
don't care about filtering the _PDC bits, because the ones Xen knows
about are Intel-only?

As you say, we likely need some clarification about whether there's
_PDC bits AMD care about?

Linux seems to unconditionally set bits in _PDC, so some of those
might actually be parsed by AMD.

I think we might want to split the setting of XEN_PROCESSOR_PM_CPPC
here from the addition of ACPI_PDC_CPPC_NATIVE_INTR into
ACPI_PDC_P_MASK.  The latter we can possibly untie from the questions
we have about AMD usage of _PDC.

Thanks, Roger.