| Message ID | 20220627165832.10246-1-mario.limonciello@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is supported | expand |
[Public] > -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Wednesday, June 29, 2022 13:42 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; > Pierre Gondois <pierre.gondois@arm.com>; Sudeep Holla > <sudeep.holla@arm.com>; Yuan, Perry <Perry.Yuan@amd.com>; ACPI Devel > Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org> > Subject: Re: [PATCH] ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is > supported > > On Mon, Jun 27, 2022 at 6:58 PM Mario Limonciello > <mario.limonciello@amd.com> wrote: > > > > commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and > > when CPPC_LIB is supported") added support for claiming to > > support CPPC in _OSC on non-Intel platforms. > > > > This unfortunately caused a regression on a vartiety of AMD > > platforms in the field because a number of AMD platforms don't set > > the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. > > > > As these AMD platforms already claim CPPC support via `X86_FEATURE_CPPC`, > > use this enable this feature rather than requiring the `_OSC`. > > > > Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is > supported") > > Reported-by: Perry Yuan <perry.yuan@amd.com> > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > --- > > drivers/acpi/cppc_acpi.c | 12 +++++++++--- > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > > index 903528f7e187..5463e6309b9a 100644 > > --- a/drivers/acpi/cppc_acpi.c > > +++ b/drivers/acpi/cppc_acpi.c > > @@ -629,6 +629,15 @@ static bool is_cppc_supported(int revision, int > num_ent) > > return false; > > } > > > > + if (osc_sb_cppc_not_supported) { > > + pr_debug("Firmware missing _OSC support\n"); > > +#ifdef CONFIG_X86 > > + return boot_cpu_has(X86_FEATURE_CPPC); > > +#else > > + return false; > > +#endif > > What about doing > > if (osc_sb_cppc_not_supported) { > pr_debug("Firmware missing _OSC support\n"); > return IS_ENABLED(CONFIG_X86) && boot_cpu_has(X86_FEATURE_CPPC); > } > > instead for the sake of reducing #ifdeffery? I don't think that would compile on non-X86. X86_FEATURE_CPPC comes as part of arch/x86/include/asm/cpufeatures.h, which I wouldn't expect is included on !x86. > > Also, this is somewhat risky, because even if the given processor has > X86_FEATURE_CPPC set, the platform may still not want to expose CPPC > through ACPI. How's that going to work after this change? > Well actually doing that through _OSC wouldn't have worked before 72f2ecb7ece7 either. If desirable - a platform could avoid populating _CPC objects in ACPI tables in this case. I do know of OEM platforms that the underlying APU supports CPPC but the OEM doesn't populate _CPC. Presumably for this exact reason. > > > + } > > + > > return true; > > } > > > > @@ -684,9 +693,6 @@ int acpi_cppc_processor_probe(struct acpi_processor > *pr) > > acpi_status status; > > int ret = -ENODATA; > > > > - if (osc_sb_cppc_not_supported) > > - return -ENODEV; > > - > > /* Parse the ACPI _CPC table for this CPU. */ > > status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output, > > ACPI_TYPE_PACKAGE); > > -- > > 2.34.1 > >
On Wed, Jun 29, 2022 at 8:49 PM Limonciello, Mario <Mario.Limonciello@amd.com> wrote: > > [Public] > > > > > -----Original Message----- > > From: Rafael J. Wysocki <rafael@kernel.org> > > Sent: Wednesday, June 29, 2022 13:42 > > To: Limonciello, Mario <Mario.Limonciello@amd.com> > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; > > Pierre Gondois <pierre.gondois@arm.com>; Sudeep Holla > > <sudeep.holla@arm.com>; Yuan, Perry <Perry.Yuan@amd.com>; ACPI Devel > > Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux- > > kernel@vger.kernel.org> > > Subject: Re: [PATCH] ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is > > supported > > > > On Mon, Jun 27, 2022 at 6:58 PM Mario Limonciello > > <mario.limonciello@amd.com> wrote: > > > > > > commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and > > > when CPPC_LIB is supported") added support for claiming to > > > support CPPC in _OSC on non-Intel platforms. > > > > > > This unfortunately caused a regression on a vartiety of AMD > > > platforms in the field because a number of AMD platforms don't set > > > the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. > > > > > > As these AMD platforms already claim CPPC support via `X86_FEATURE_CPPC`, > > > use this enable this feature rather than requiring the `_OSC`. > > > > > > Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is > > supported") > > > Reported-by: Perry Yuan <perry.yuan@amd.com> > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > --- > > > drivers/acpi/cppc_acpi.c | 12 +++++++++--- > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > > > index 903528f7e187..5463e6309b9a 100644 > > > --- a/drivers/acpi/cppc_acpi.c > > > +++ b/drivers/acpi/cppc_acpi.c > > > @@ -629,6 +629,15 @@ static bool is_cppc_supported(int revision, int > > num_ent) > > > return false; > > > } > > > > > > + if (osc_sb_cppc_not_supported) { > > > + pr_debug("Firmware missing _OSC support\n"); > > > +#ifdef CONFIG_X86 > > > + return boot_cpu_has(X86_FEATURE_CPPC); > > > +#else > > > + return false; > > > +#endif > > > > What about doing > > > > if (osc_sb_cppc_not_supported) { > > pr_debug("Firmware missing _OSC support\n"); > > return IS_ENABLED(CONFIG_X86) && boot_cpu_has(X86_FEATURE_CPPC); > > } > > > > instead for the sake of reducing #ifdeffery? > > I don't think that would compile on non-X86. X86_FEATURE_CPPC comes as part of > arch/x86/include/asm/cpufeatures.h, which I wouldn't expect is included on !x86. Good point. Something like this would still look better though IMO: if (!osc_sb_cppc_not_supported) return true; #ifdef CONFIG_X86 return boot_cpu_has(X86_FEATURE_CPPC); #else return false; #endif } > > > > > Also, this is somewhat risky, because even if the given processor has > > X86_FEATURE_CPPC set, the platform may still not want to expose CPPC > > through ACPI. How's that going to work after this change? > > > > Well actually doing that through _OSC wouldn't have worked before 72f2ecb7ece7 either. > If desirable - a platform could avoid populating _CPC objects in ACPI tables in this case. > > I do know of OEM platforms that the underlying APU supports CPPC but the OEM doesn't > populate _CPC. Presumably for this exact reason. That is an option, but there is no requirement that _CPC must not be populated when CPPC is not supported. _OSC is the proper mechanism for negotiating CPPC support. Still, if you know for a fact that on AMD systems X86_FEATURE_CPPC always means that CPPC is supported, I can live with an extra vendor check in the code above.
[AMD Official Use Only - General] > -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Wednesday, June 29, 2022 14:09 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; > Pierre Gondois <pierre.gondois@arm.com>; Sudeep Holla > <sudeep.holla@arm.com>; Yuan, Perry <Perry.Yuan@amd.com>; ACPI Devel > Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux- > kernel@vger.kernel.org> > Subject: Re: [PATCH] ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC is > supported > > On Wed, Jun 29, 2022 at 8:49 PM Limonciello, Mario > <Mario.Limonciello@amd.com> wrote: > > > > [Public] > > > > > > > > > -----Original Message----- > > > From: Rafael J. Wysocki <rafael@kernel.org> > > > Sent: Wednesday, June 29, 2022 13:42 > > > To: Limonciello, Mario <Mario.Limonciello@amd.com> > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Len Brown <lenb@kernel.org>; > > > Pierre Gondois <pierre.gondois@arm.com>; Sudeep Holla > > > <sudeep.holla@arm.com>; Yuan, Perry <Perry.Yuan@amd.com>; ACPI Devel > > > Maling List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux- > > > kernel@vger.kernel.org> > > > Subject: Re: [PATCH] ACPI: CPPC: Don't require _OSC if X86_FEATURE_CPPC > is > > > supported > > > > > > On Mon, Jun 27, 2022 at 6:58 PM Mario Limonciello > > > <mario.limonciello@amd.com> wrote: > > > > > > > > commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and > > > > when CPPC_LIB is supported") added support for claiming to > > > > support CPPC in _OSC on non-Intel platforms. > > > > > > > > This unfortunately caused a regression on a vartiety of AMD > > > > platforms in the field because a number of AMD platforms don't set > > > > the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. > > > > > > > > As these AMD platforms already claim CPPC support via > `X86_FEATURE_CPPC`, > > > > use this enable this feature rather than requiring the `_OSC`. > > > > > > > > Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is > > > supported") > > > > Reported-by: Perry Yuan <perry.yuan@amd.com> > > > > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> > > > > --- > > > > drivers/acpi/cppc_acpi.c | 12 +++++++++--- > > > > 1 file changed, 9 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c > > > > index 903528f7e187..5463e6309b9a 100644 > > > > --- a/drivers/acpi/cppc_acpi.c > > > > +++ b/drivers/acpi/cppc_acpi.c > > > > @@ -629,6 +629,15 @@ static bool is_cppc_supported(int revision, int > > > num_ent) > > > > return false; > > > > } > > > > > > > > + if (osc_sb_cppc_not_supported) { > > > > + pr_debug("Firmware missing _OSC support\n"); > > > > +#ifdef CONFIG_X86 > > > > + return boot_cpu_has(X86_FEATURE_CPPC); > > > > +#else > > > > + return false; > > > > +#endif > > > > > > What about doing > > > > > > if (osc_sb_cppc_not_supported) { > > > pr_debug("Firmware missing _OSC support\n"); > > > return IS_ENABLED(CONFIG_X86) && > boot_cpu_has(X86_FEATURE_CPPC); > > > } > > > > > > instead for the sake of reducing #ifdeffery? > > > > I don't think that would compile on non-X86. X86_FEATURE_CPPC comes as > part of > > arch/x86/include/asm/cpufeatures.h, which I wouldn't expect is included on > !x86. > > Good point. > > Something like this would still look better though IMO: > > if (!osc_sb_cppc_not_supported) > return true; > > #ifdef CONFIG_X86 > return boot_cpu_has(X86_FEATURE_CPPC); > #else > return false; > #endif > } > Thanks, I'll respin it with something similar to that. > > > > > > > > > Also, this is somewhat risky, because even if the given processor has > > > X86_FEATURE_CPPC set, the platform may still not want to expose CPPC > > > through ACPI. How's that going to work after this change? > > > > > > > Well actually doing that through _OSC wouldn't have worked before > 72f2ecb7ece7 either. > > If desirable - a platform could avoid populating _CPC objects in ACPI tables in > this case. > > > > I do know of OEM platforms that the underlying APU supports CPPC but the > OEM doesn't > > populate _CPC. Presumably for this exact reason. > > That is an option, but there is no requirement that _CPC must not be > populated when CPPC is not supported. > > _OSC is the proper mechanism for negotiating CPPC support. > Right; I agree this should have been the proper mechanism. I'll talk to our internal BIOS team to double check reference BIOS is populated with this correctly for programs going forward too. > Still, if you know for a fact that on AMD systems X86_FEATURE_CPPC > always means that CPPC is supported, I can live with an extra vendor > check in the code above. Thanks. The definition of that CPUID 8000_0008 EBX bit 27 used to populate X86_FEATURE_CPPC indicates whether the CPU/APU supports the dedicated MSR. There are also technically designs that can work in shared memory mode that I think the only way to "safely" discover will be via the _OSC. If this same regression from 72f2ecb7ece7 crops up on those we might need to look at changing the amd-pstate module parameter override that enables it for shared memory into a general kernel command line override for users to use.
diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 903528f7e187..5463e6309b9a 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -629,6 +629,15 @@ static bool is_cppc_supported(int revision, int num_ent) return false; } + if (osc_sb_cppc_not_supported) { + pr_debug("Firmware missing _OSC support\n"); +#ifdef CONFIG_X86 + return boot_cpu_has(X86_FEATURE_CPPC); +#else + return false; +#endif + } + return true; } @@ -684,9 +693,6 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr) acpi_status status; int ret = -ENODATA; - if (osc_sb_cppc_not_supported) - return -ENODEV; - /* Parse the ACPI _CPC table for this CPU. */ status = acpi_evaluate_object_typed(handle, "_CPC", NULL, &output, ACPI_TYPE_PACKAGE);
commit 72f2ecb7ece7 ("ACPI: bus: Set CPPC _OSC bits for all and when CPPC_LIB is supported") added support for claiming to support CPPC in _OSC on non-Intel platforms. This unfortunately caused a regression on a vartiety of AMD platforms in the field because a number of AMD platforms don't set the `_OSC` bit 5 or 6 to indicate CPPC or CPPC v2 support. As these AMD platforms already claim CPPC support via `X86_FEATURE_CPPC`, use this enable this feature rather than requiring the `_OSC`. Fixes: 72f2ecb7ece7 ("Set CPPC _OSC bits for all and when CPPC_LIB is supported") Reported-by: Perry Yuan <perry.yuan@amd.com> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> --- drivers/acpi/cppc_acpi.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)