| Message ID | 20220310212805.3786-1-mario.limonciello@amd.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v6] ACPI: bus: For platform OSC negotiate capabilities | expand |
[Public] > I would do > > if (capbuf[OSC_SUPPORT_DWORD] == capbuf_ret[OSC_SUPPORT_DWORD]) > capbuf[OSC_QUERY_DWORD] = 0; > else > capbuf[OSC_SUPPORT_DWORD] &= capbuf_ret[OSC_SUPPORT_DWORD]; > > so that the loop terminates even if the firmware does strange things > and then it would only be necessary to check capbuf[OSC_QUERY_DWORD] > in the loop termination condition. > > Would that work? > I think it will. I'll try it and send up a v7 if so. > > + kfree(context.ret.pointer); > > + } while (capbuf[OSC_QUERY_DWORD] && > capbuf[OSC_SUPPORT_DWORD]); > > > > - /* Now run _OSC again with query flag clear */ > > - capbuf[OSC_QUERY_DWORD] = 0; > > + /* > > + * Avoid problems with BIOS dynamically loading tables by indicating > > + * support for CPPC even if it was masked. > > What exactly do you mean by "BIOS dynamically loading tables"? As mentioned in commit 159d8c274fd9: On certain systems the BIOS loads SSDT tables dynamically based on the capabilities the OS claims to support. However, on these systems the _OSC actually clears some of the bits (under certain conditions) so what happens is that now when we call the _OSC twice the second time we pass the cleared values and that results errors like below to appear on the system log: ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], AE_NOT_FOUND (20210105/psargs-330) ACPI Error: Aborting method \_PR.PR01._CPC due to previous error (AE_NOT_FOUND) (20210105/psparse-529) This block is to avoid regressing that again by forcing it on these systems. > > > + */ > > +#ifdef CONFIG_X86 > > + if (boot_cpu_has(X86_FEATURE_HWP)) { > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT; > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT; > > + } > > +#endif > > > > + /* Now run _OSC again with query flag clear */ > > if (ACPI_FAILURE(acpi_run_osc(handle, &context))) > > return; > > > > -- > > 2.34.1 > >
On Mon, Mar 14, 2022 at 12:45 AM Limonciello, Mario <Mario.Limonciello@amd.com> wrote: > > [Public] > > > I would do > > > > if (capbuf[OSC_SUPPORT_DWORD] == capbuf_ret[OSC_SUPPORT_DWORD]) > > capbuf[OSC_QUERY_DWORD] = 0; > > else > > capbuf[OSC_SUPPORT_DWORD] &= capbuf_ret[OSC_SUPPORT_DWORD]; > > > > so that the loop terminates even if the firmware does strange things > > and then it would only be necessary to check capbuf[OSC_QUERY_DWORD] > > in the loop termination condition. > > > > Would that work? > > > > I think it will. I'll try it and send up a v7 if so. > > > > + kfree(context.ret.pointer); > > > + } while (capbuf[OSC_QUERY_DWORD] && > > capbuf[OSC_SUPPORT_DWORD]); > > > > > > - /* Now run _OSC again with query flag clear */ > > > - capbuf[OSC_QUERY_DWORD] = 0; > > > + /* > > > + * Avoid problems with BIOS dynamically loading tables by indicating > > > + * support for CPPC even if it was masked. > > > > What exactly do you mean by "BIOS dynamically loading tables"? > > As mentioned in commit 159d8c274fd9: > > On certain systems the BIOS loads SSDT tables dynamically based on the > capabilities the OS claims to support. However, on these systems the > _OSC actually clears some of the bits (under certain conditions) so what > happens is that now when we call the _OSC twice the second time we pass > the cleared values and that results errors like below to appear on the > system log: > > ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], AE_NOT_FOUND (20210105/psargs-330) > ACPI Error: Aborting method \_PR.PR01._CPC due to previous error (AE_NOT_FOUND) (20210105/psparse-529) > > This block is to avoid regressing that again by forcing it on these systems. Well, this means that the code before and after the patch is not actually following the spec. First off, as mentioned in the changelog of commit 159d8c274fd9 (in the part that has not been quoted above), the OS is required to pass the same set of capabilities every time _OSC is evaluated. This doesn't happen after the change. Second, acpi_bus_osc_negotiate_platform_control() should take the capabilities mask returned by the platform which it doesn't do without the patch. That latter piece appears to be the bug in question here and IMO fixing it doesn't even require calling acpi_run_osc() with the query flag set for multiple times. > > > > > > + */ > > > +#ifdef CONFIG_X86 > > > + if (boot_cpu_has(X86_FEATURE_HWP)) { > > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT; > > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT; > > > + } > > > +#endif > > > > > > + /* Now run _OSC again with query flag clear */ > > > if (ACPI_FAILURE(acpi_run_osc(handle, &context))) > > > return; > > > > > > -- > > > 2.34.1 > > >
[AMD Official Use Only] > -----Original Message----- > From: Rafael J. Wysocki <rafael@kernel.org> > Sent: Monday, March 14, 2022 15:01 > To: Limonciello, Mario <Mario.Limonciello@amd.com> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Rafael J . Wysocki > <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > Mika Westerberg <mika.westerberg@linux.intel.com>; Hou, Xiaomeng > (Matthew) <Xiaomeng.Hou@amd.com>; Liu, Aaron <Aaron.Liu@amd.com>; > Huang, Ray <Ray.Huang@amd.com>; Hans de Goede > <hdegoede@redhat.com> > Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities > > On Mon, Mar 14, 2022 at 12:45 AM Limonciello, Mario > <Mario.Limonciello@amd.com> wrote: > > > > [Public] > > > > > I would do > > > > > > if (capbuf[OSC_SUPPORT_DWORD] == > capbuf_ret[OSC_SUPPORT_DWORD]) > > > capbuf[OSC_QUERY_DWORD] = 0; > > > else > > > capbuf[OSC_SUPPORT_DWORD] &= > capbuf_ret[OSC_SUPPORT_DWORD]; > > > > > > so that the loop terminates even if the firmware does strange things > > > and then it would only be necessary to check > capbuf[OSC_QUERY_DWORD] > > > in the loop termination condition. > > > > > > Would that work? > > > > > > > I think it will. I'll try it and send up a v7 if so. > > > > > > + kfree(context.ret.pointer); > > > > + } while (capbuf[OSC_QUERY_DWORD] && > > > capbuf[OSC_SUPPORT_DWORD]); > > > > > > > > - /* Now run _OSC again with query flag clear */ > > > > - capbuf[OSC_QUERY_DWORD] = 0; > > > > + /* > > > > + * Avoid problems with BIOS dynamically loading tables by > indicating > > > > + * support for CPPC even if it was masked. > > > > > > What exactly do you mean by "BIOS dynamically loading tables"? > > > > As mentioned in commit 159d8c274fd9: > > > > On certain systems the BIOS loads SSDT tables dynamically based on the > > capabilities the OS claims to support. However, on these systems the > > _OSC actually clears some of the bits (under certain conditions) so what > > happens is that now when we call the _OSC twice the second time we > pass > > the cleared values and that results errors like below to appear on the > > system log: > > > > ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], > AE_NOT_FOUND (20210105/psargs-330) > > ACPI Error: Aborting method \_PR.PR01._CPC due to previous error > (AE_NOT_FOUND) (20210105/psparse-529) > > > > This block is to avoid regressing that again by forcing it on these systems. > > Well, this means that the code before and after the patch is not > actually following the spec. > > First off, as mentioned in the changelog of commit 159d8c274fd9 (in > the part that has not been quoted above), the OS is required to pass > the same set of capabilities every time _OSC is evaluated. This > doesn't happen after the change. > > Second, acpi_bus_osc_negotiate_platform_control() should take the > capabilities mask returned by the platform which it doesn't do without > the patch. Right on both points. > > That latter piece appears to be the bug in question here and IMO > fixing it doesn't even require calling acpi_run_osc() with the query > flag set for multiple times. I think just taking the results will re-introduce the CPC bug though won't it? So how to avoid it but also to take the results? > > > > > > > > > > + */ > > > > +#ifdef CONFIG_X86 > > > > + if (boot_cpu_has(X86_FEATURE_HWP)) { > > > > + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT; > > > > + capbuf[OSC_SUPPORT_DWORD] |= > OSC_SB_CPCV2_SUPPORT; > > > > + } > > > > +#endif > > > > > > > > + /* Now run _OSC again with query flag clear */ > > > > if (ACPI_FAILURE(acpi_run_osc(handle, &context))) > > > > return; > > > > > > > > -- > > > > 2.34.1 > > > >
On Tue, Mar 15, 2022 at 5:30 AM Limonciello, Mario <Mario.Limonciello@amd.com> wrote: > > [AMD Official Use Only] > > > > > -----Original Message----- > > From: Rafael J. Wysocki <rafael@kernel.org> > > Sent: Monday, March 14, 2022 15:01 > > To: Limonciello, Mario <Mario.Limonciello@amd.com> > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Rafael J . Wysocki > > <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > > Mika Westerberg <mika.westerberg@linux.intel.com>; Hou, Xiaomeng > > (Matthew) <Xiaomeng.Hou@amd.com>; Liu, Aaron <Aaron.Liu@amd.com>; > > Huang, Ray <Ray.Huang@amd.com>; Hans de Goede > > <hdegoede@redhat.com> > > Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities > > > > On Mon, Mar 14, 2022 at 12:45 AM Limonciello, Mario > > <Mario.Limonciello@amd.com> wrote: > > > > > > [Public] > > > > > > > I would do > > > > > > > > if (capbuf[OSC_SUPPORT_DWORD] == > > capbuf_ret[OSC_SUPPORT_DWORD]) > > > > capbuf[OSC_QUERY_DWORD] = 0; > > > > else > > > > capbuf[OSC_SUPPORT_DWORD] &= > > capbuf_ret[OSC_SUPPORT_DWORD]; > > > > > > > > so that the loop terminates even if the firmware does strange things > > > > and then it would only be necessary to check > > capbuf[OSC_QUERY_DWORD] > > > > in the loop termination condition. > > > > > > > > Would that work? > > > > > > > > > > I think it will. I'll try it and send up a v7 if so. > > > > > > > > + kfree(context.ret.pointer); > > > > > + } while (capbuf[OSC_QUERY_DWORD] && > > > > capbuf[OSC_SUPPORT_DWORD]); > > > > > > > > > > - /* Now run _OSC again with query flag clear */ > > > > > - capbuf[OSC_QUERY_DWORD] = 0; > > > > > + /* > > > > > + * Avoid problems with BIOS dynamically loading tables by > > indicating > > > > > + * support for CPPC even if it was masked. > > > > > > > > What exactly do you mean by "BIOS dynamically loading tables"? > > > > > > As mentioned in commit 159d8c274fd9: > > > > > > On certain systems the BIOS loads SSDT tables dynamically based on the > > > capabilities the OS claims to support. However, on these systems the > > > _OSC actually clears some of the bits (under certain conditions) so what > > > happens is that now when we call the _OSC twice the second time we > > pass > > > the cleared values and that results errors like below to appear on the > > > system log: > > > > > > ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], > > AE_NOT_FOUND (20210105/psargs-330) > > > ACPI Error: Aborting method \_PR.PR01._CPC due to previous error > > (AE_NOT_FOUND) (20210105/psparse-529) > > > > > > This block is to avoid regressing that again by forcing it on these systems. > > > > Well, this means that the code before and after the patch is not > > actually following the spec. > > > > First off, as mentioned in the changelog of commit 159d8c274fd9 (in > > the part that has not been quoted above), the OS is required to pass > > the same set of capabilities every time _OSC is evaluated. This > > doesn't happen after the change. > > > > Second, acpi_bus_osc_negotiate_platform_control() should take the > > capabilities mask returned by the platform which it doesn't do without > > the patch. > > Right on both points. > > > > > That latter piece appears to be the bug in question here and IMO > > fixing it doesn't even require calling acpi_run_osc() with the query > > flag set for multiple times. > > I think just taking the results will re-introduce the CPC bug though > won't it? So how to avoid it but also to take the results? I think that the OS should not ask for the control of the CPPC bits if they are masked by the firmware and it should avoid invoking _CPC then. Otherwise we risk breaking legitimate cases in which the firmware actually doesn't want the OS to control those bits.
On Tue, Mar 15, 2022 at 11:34 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Tue, Mar 15, 2022 at 5:30 AM Limonciello, Mario > <Mario.Limonciello@amd.com> wrote: > > > > [AMD Official Use Only] > > > > > > > > > -----Original Message----- > > > From: Rafael J. Wysocki <rafael@kernel.org> > > > Sent: Monday, March 14, 2022 15:01 > > > To: Limonciello, Mario <Mario.Limonciello@amd.com> > > > Cc: Rafael J. Wysocki <rafael@kernel.org>; Rafael J . Wysocki > > > <rjw@rjwysocki.net>; ACPI Devel Maling List <linux-acpi@vger.kernel.org>; > > > Mika Westerberg <mika.westerberg@linux.intel.com>; Hou, Xiaomeng > > > (Matthew) <Xiaomeng.Hou@amd.com>; Liu, Aaron <Aaron.Liu@amd.com>; > > > Huang, Ray <Ray.Huang@amd.com>; Hans de Goede > > > <hdegoede@redhat.com> > > > Subject: Re: [PATCH v6] ACPI: bus: For platform OSC negotiate capabilities > > > > > > On Mon, Mar 14, 2022 at 12:45 AM Limonciello, Mario > > > <Mario.Limonciello@amd.com> wrote: > > > > > > > > [Public] > > > > > > > > > I would do > > > > > > > > > > if (capbuf[OSC_SUPPORT_DWORD] == > > > capbuf_ret[OSC_SUPPORT_DWORD]) > > > > > capbuf[OSC_QUERY_DWORD] = 0; > > > > > else > > > > > capbuf[OSC_SUPPORT_DWORD] &= > > > capbuf_ret[OSC_SUPPORT_DWORD]; > > > > > > > > > > so that the loop terminates even if the firmware does strange things > > > > > and then it would only be necessary to check > > > capbuf[OSC_QUERY_DWORD] > > > > > in the loop termination condition. > > > > > > > > > > Would that work? > > > > > > > > > > > > > I think it will. I'll try it and send up a v7 if so. > > > > > > > > > > + kfree(context.ret.pointer); > > > > > > + } while (capbuf[OSC_QUERY_DWORD] && > > > > > capbuf[OSC_SUPPORT_DWORD]); > > > > > > > > > > > > - /* Now run _OSC again with query flag clear */ > > > > > > - capbuf[OSC_QUERY_DWORD] = 0; > > > > > > + /* > > > > > > + * Avoid problems with BIOS dynamically loading tables by > > > indicating > > > > > > + * support for CPPC even if it was masked. > > > > > > > > > > What exactly do you mean by "BIOS dynamically loading tables"? > > > > > > > > As mentioned in commit 159d8c274fd9: > > > > > > > > On certain systems the BIOS loads SSDT tables dynamically based on the > > > > capabilities the OS claims to support. However, on these systems the > > > > _OSC actually clears some of the bits (under certain conditions) so what > > > > happens is that now when we call the _OSC twice the second time we > > > pass > > > > the cleared values and that results errors like below to appear on the > > > > system log: > > > > > > > > ACPI BIOS Error (bug): Could not resolve symbol [\_PR.PR00._CPC], > > > AE_NOT_FOUND (20210105/psargs-330) > > > > ACPI Error: Aborting method \_PR.PR01._CPC due to previous error > > > (AE_NOT_FOUND) (20210105/psparse-529) > > > > > > > > This block is to avoid regressing that again by forcing it on these systems. > > > > > > Well, this means that the code before and after the patch is not > > > actually following the spec. > > > > > > First off, as mentioned in the changelog of commit 159d8c274fd9 (in > > > the part that has not been quoted above), the OS is required to pass > > > the same set of capabilities every time _OSC is evaluated. This > > > doesn't happen after the change. > > > > > > Second, acpi_bus_osc_negotiate_platform_control() should take the > > > capabilities mask returned by the platform which it doesn't do without > > > the patch. > > > > Right on both points. > > > > > > > > That latter piece appears to be the bug in question here and IMO > > > fixing it doesn't even require calling acpi_run_osc() with the query > > > flag set for multiple times. > > > > I think just taking the results will re-introduce the CPC bug though > > won't it? So how to avoid it but also to take the results? > > I think that the OS should not ask for the control of the CPPC bits if > they are masked by the firmware and it should avoid invoking _CPC > then. > > Otherwise we risk breaking legitimate cases in which the firmware > actually doesn't want the OS to control those bits. I'm basically talking about reverting commit 159d8c274fd9, as the part of the _OSC definition in the spec it is based on appears to be bogus (that will be addressed separately via the ACPI spec process), and applying the attached change on top of that. If this looks good to you, I'll take care of it.
[Public] > > > > That latter piece appears to be the bug in question here and IMO > > > > fixing it doesn't even require calling acpi_run_osc() with the query > > > > flag set for multiple times. > > > > > > I think just taking the results will re-introduce the CPC bug though > > > won't it? So how to avoid it but also to take the results? > > > > I think that the OS should not ask for the control of the CPPC bits if > > they are masked by the firmware and it should avoid invoking _CPC > > then. > > > > Otherwise we risk breaking legitimate cases in which the firmware > > actually doesn't want the OS to control those bits. > > I'm basically talking about reverting commit 159d8c274fd9, as the part > of the _OSC definition in the spec it is based on appears to be bogus > (that will be addressed separately via the ACPI spec process), and > applying the attached change on top of that. > > If this looks good to you, I'll take care of it. Yes, that looks great and I checked with 159d8c274fd9 reverted and that applying the problem does not exist. I do think it has the possibility to cause CPPC to not be enabled outside of Intel though since HWP is only set there so I would suggest this other change on top of it: diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index 4df749b82568..e61dbd7f7108 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -314,10 +314,8 @@ static void acpi_bus_osc_negotiate_platform_control(void) #endif #ifdef CONFIG_X86 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_GENERIC_INITIATOR_SUPPORT; - if (boot_cpu_has(X86_FEATURE_HWP)) { - capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT; - capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT; - } + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT; + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT; #endif if (IS_ENABLED(CONFIG_SCHED_MC_PRIO))
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c index ec83c4f6d628..351bac98f70c 100644 --- a/drivers/acpi/bus.c +++ b/drivers/acpi/bus.c @@ -330,14 +330,29 @@ static void acpi_bus_osc_negotiate_platform_control(void) if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle))) return; - if (ACPI_FAILURE(acpi_run_osc(handle, &context))) - return; - kfree(context.ret.pointer); + do { + if (ACPI_FAILURE(acpi_run_osc(handle, &context))) + return; + capbuf_ret = context.ret.pointer; + if (capbuf[OSC_SUPPORT_DWORD] == capbuf_ret[OSC_SUPPORT_DWORD]) + capbuf[OSC_QUERY_DWORD] = 0; + capbuf[OSC_SUPPORT_DWORD] = capbuf_ret[OSC_SUPPORT_DWORD]; + kfree(context.ret.pointer); + } while (capbuf[OSC_QUERY_DWORD] && capbuf[OSC_SUPPORT_DWORD]); - /* Now run _OSC again with query flag clear */ - capbuf[OSC_QUERY_DWORD] = 0; + /* + * Avoid problems with BIOS dynamically loading tables by indicating + * support for CPPC even if it was masked. + */ +#ifdef CONFIG_X86 + if (boot_cpu_has(X86_FEATURE_HWP)) { + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT; + capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT; + } +#endif + /* Now run _OSC again with query flag clear */ if (ACPI_FAILURE(acpi_run_osc(handle, &context))) return;
According to the ACPI 6.4 spec: It is strongly recommended that the OS evaluate _OSC with the Query Support Flag set until _OSC returns the Capabilities Masked bit clear, to negotiate the set of features to be granted to the OS for native support; a platform may require a specific combination of features to be supported natively by an OS before granting native control of a given feature. After negotiation with the query flag set, the OS should evaluate without it so that any negotiated values can be made effective to hardware. Currently the code sends the exact same values in both executions of the _OSC and this leads to some problems on some AMD platforms in certain configurations. The following notable capabilities are set by OSPM when query is enabled: * OSC_SB_PR3_SUPPORT * OSC_SB_PCLPI_SUPPORT * OSC_SB_NATIVE_USB4_SUPPORT The first call to the platform OSC returns back a masked capabilities error because the firmware did not acknowledge OSC_SB_PCLPI_SUPPORT but it acknolwedged the others. The second call to the platform _OSC without the query flag set then fails because the OSPM still sent the exact same values. This leads to not acknowledging OSC_SB_NATIVE_USB4_SUPPORT and later USB4 PCIe tunnels can't be authorized. This problem was first introduced by commit 159d8c274fd9 ("ACPI: Pass the same capabilities to the _OSC regardless of the query flag") which subtly adjusted the behavior from 719e1f5 ("ACPI: Execute platform _OSC also with query bit clear"). The _OSC was called exactly 2 times: * Once to query and request from firmware * Once to commit to firmware without query To fix this problem, continue to call the _OSC until the firmware has indicated that capabilities are no longer masked. Furthermore, to avoid the problem that commit 159d8c274fd9 ("ACPI: Pass the same capabilities to the _OSC regardless of the query flag") introduced, explicitly mark support for CPC and CPPCv2 even if they were masked by the series of query calls due to table loading order on some systems. Fixes: 159d8c274fd9 ("ACPI: Pass the same capabilities to the _OSC regardless of the query flag") Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> changes from v5->v6 * drop mika's tag due to changes * negotiate until support result is empty --- drivers/acpi/bus.c | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-)