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 <[email protected]> > --- > 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.
