Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932296AbbFYAoL (ORCPT ); Wed, 24 Jun 2015 20:44:11 -0400 Received: from mga09.intel.com ([134.134.136.24]:41832 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932222AbbFYAnr (ORCPT ); Wed, 24 Jun 2015 20:43:47 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.13,674,1427785200"; d="scan'208";a="734124032" From: "Zheng, Lv" To: "Rafael J. Wysocki" CC: "Wysocki, Rafael J" , "Brown, Len" , Lv Zheng , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "Moore, Robert" Subject: RE: [PATCH v2 05/28] ACPICA: Hardware: Enable firmware waking vector for both 32-bit and 64-bit FACS. Thread-Topic: [PATCH v2 05/28] ACPICA: Hardware: Enable firmware waking vector for both 32-bit and 64-bit FACS. Thread-Index: AQHQrtXVZn+INYmbOk62zhN20nmuqp28YK6Q Date: Thu, 25 Jun 2015 00:43:39 +0000 Message-ID: <1AE640813FDE7649BE1B193DEA596E8802733CA6@SHSMSX101.ccr.corp.intel.com> References: <24343352.3W7mrSPtdt@vostro.rjw.lan> In-Reply-To: <24343352.3W7mrSPtdt@vostro.rjw.lan> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id t5P0iFtC020029 Content-Length: 8366 Lines: 189 Hi, Rafael > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] > Sent: Thursday, June 25, 2015 7:57 AM > > On Wednesday, June 24, 2015 11:02:54 AM Lv Zheng wrote: > > ACPICA commit 368eb60778b27b6ae94d3658ddc902ca1342a963 > > ACPICA commit 70f62a80d65515e1285fdeeb50d94ee6f07df4bd > > > > The following commit is reported to have broken s2ram on some platforms: > > Commit: 0249ed2444d65d65fc3f3f64f398f1ad0b7e54cd > > ACPICA: Add option to favor 32-bit FADT addresses. > > The platform reports 2 FACS tables (which is not allowed by ACPI > > specification) and the new 32-bit address favor rule forces OSPMs to use > > the FACS table reported via FADT's X_FIRMWARE_CTRL field. > > > > The root cause of the reported bug might be one of the followings: > > 1. BIOS may favor the 64-bit firmware waking vector address when the > > version of the FACS is greater than 0 and Linux currently only supports > > resuming from the real mode, so the 64-bit firmware waking vector has > > never been set and might be invalid to BIOS while the commit enables > > higher version FACS. > > 2. BIOS may favor the FACS reported via the "FIRMWARE_CTRL" field in the > > FADT while the commit doesn't set the firmware waking vector address of > > the FACS reported by "FIRMWARE_CTRL", it only sets the firware waking > > vector address of the FACS reported by "X_FIRMWARE_CTRL". > > > > This patch excludes the cases that can trigger the bugs caused by the root > > cause 2. > > > > There is no handshaking mechanism can be used by OSPM to tell BIOS which > > FACS is currently used. Thus the FACS reported by "FIRMWARE_CTRL" may still > > be used by BIOS and the 0 value of the 32-bit firmware waking vector might > > trigger such failure. > > > > This patch enables the firmware waking vectors for both 32bit/64bit FACS > > tables in order to ensure we can exclude the cases that trigger the bugs > > caused by the root cause 2. The exclusion is split into 2 commits so that > > if it turns out not to be necessary, this single commit can be reverted > > without affecting the useful one. Lv Zheng, Bob Moore. > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=74021 > > Link: https://github.com/acpica/acpica/commit/368eb607 > > Link: https://github.com/acpica/acpica/commit/70f62a80 > > Reported-and-tested-by: Oswald Buddenhagen > > Signed-off-by: Lv Zheng > > Signed-off-by: Bob Moore > > --- > > drivers/acpi/acpica/acglobal.h | 2 ++ > > drivers/acpi/acpica/hwxfsleep.c | 74 ++++++++++++++++++++++++++++++++------- > > drivers/acpi/acpica/tbutils.c | 14 ++++---- > > 3 files changed, 71 insertions(+), 19 deletions(-) > > > > diff --git a/drivers/acpi/acpica/acglobal.h b/drivers/acpi/acpica/acglobal.h > > index a0c4787..53f96a3 100644 > > --- a/drivers/acpi/acpica/acglobal.h > > +++ b/drivers/acpi/acpica/acglobal.h > > @@ -61,6 +61,8 @@ ACPI_GLOBAL(struct acpi_table_header, acpi_gbl_original_dsdt_header); > > > > #if (!ACPI_REDUCED_HARDWARE) > > ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_FACS); > > +ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_facs32); > > +ACPI_GLOBAL(struct acpi_table_facs *, acpi_gbl_facs64); > > > > #endif /* !ACPI_REDUCED_HARDWARE */ > > > > diff --git a/drivers/acpi/acpica/hwxfsleep.c b/drivers/acpi/acpica/hwxfsleep.c > > index c67cd32..e273b2e 100644 > > --- a/drivers/acpi/acpica/hwxfsleep.c > > +++ b/drivers/acpi/acpica/hwxfsleep.c > > @@ -50,6 +50,13 @@ > > ACPI_MODULE_NAME("hwxfsleep") > > > > /* Local prototypes */ > > +#if (!ACPI_REDUCED_HARDWARE) > > +static acpi_status > > +acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs, > > + acpi_physical_address physical_address, > > + acpi_physical_address physical_address64); > > +#endif > > + > > static acpi_status acpi_hw_sleep_dispatch(u8 sleep_state, u32 function_id); > > > > /* > > @@ -79,9 +86,10 @@ static struct acpi_sleep_functions acpi_sleep_dispatch[] = { > > #if (!ACPI_REDUCED_HARDWARE) > > /******************************************************************************* > > * > > - * FUNCTION: acpi_set_firmware_waking_vector > > + * FUNCTION: acpi_hw_set_firmware_waking_vector > > * > > - * PARAMETERS: physical_address - 32-bit physical address of ACPI real mode > > + * PARAMETERS: facs - Pointer to FACS table > > + * physical_address - 32-bit physical address of ACPI real mode > > * entry point > > * physical_address64 - 64-bit physical address of ACPI protected > > * entry point > > @@ -92,11 +100,12 @@ static struct acpi_sleep_functions acpi_sleep_dispatch[] = { > > * > > ******************************************************************************/ > > > > -acpi_status > > -acpi_set_firmware_waking_vector(acpi_physical_address physical_address, > > - acpi_physical_address physical_address64) > > +static acpi_status > > +acpi_hw_set_firmware_waking_vector(struct acpi_table_facs *facs, > > + acpi_physical_address physical_address, > > + acpi_physical_address physical_address64) > > { > > - ACPI_FUNCTION_TRACE(acpi_set_firmware_waking_vector); > > + ACPI_FUNCTION_TRACE(acpi_hw_set_firmware_waking_vector); > > > > > > /* > > @@ -109,25 +118,66 @@ acpi_set_firmware_waking_vector(acpi_physical_address physical_address, > > > > /* Set the 32-bit vector */ > > > > - acpi_gbl_FACS->firmware_waking_vector = (u32)physical_address; > > + facs->firmware_waking_vector = (u32)physical_address; > > > > - if (acpi_gbl_FACS->length > 32) { > > - if (acpi_gbl_FACS->version >= 1) { > > + if (facs->length > 32) { > > + if (facs->version >= 1) { > > > > /* Set the 64-bit vector */ > > > > - acpi_gbl_FACS->xfirmware_waking_vector = > > - physical_address64; > > + facs->xfirmware_waking_vector = physical_address64; > > } else { > > /* Clear the 64-bit vector if it exists */ > > > > - acpi_gbl_FACS->xfirmware_waking_vector = 0; > > + facs->xfirmware_waking_vector = 0; > > } > > } > > > > return_ACPI_STATUS(AE_OK); > > } > > > > +/******************************************************************************* > > + * > > + * FUNCTION: acpi_set_firmware_waking_vector > > + * > > + * PARAMETERS: physical_address - 32-bit physical address of ACPI real mode > > + * entry point > > + * physical_address64 - 64-bit physical address of ACPI protected > > + * entry point > > + * > > + * RETURN: Status > > + * > > + * DESCRIPTION: Sets the firmware_waking_vector fields of the FACS > > + * > > + ******************************************************************************/ > > + > > +acpi_status > > +acpi_set_firmware_waking_vector(acpi_physical_address physical_address, > > + acpi_physical_address physical_address64) > > The question here is: Why does the host OS need to care about the second > argument of this function that will always be 0? Why didn't you keep the > old header of acpi_set_firmware_waking_vector() as a one-argument function > taking a u32 and why didn't you add something like > > acpi_status acpi_set_firmware_waking_vector_full(u32 real_mode_address, > acpi_physical_address high_address) > > and why didn't you redefine acpi_set_firmware_waking_vector() as > > acpi_status acpi_set_firmware_waking_vector(u32 real_mode_address) > { > return acpi_set_firmware_waking_vector_full(real_mode_address, 0); > } > > ? > > If you did that, there wouldn't be any need to touch the code in > drivers/acpi/sleep.c and the arch headers, so can you please explain to me > why *exactly* you didn't do that? Host OS can set non 0 address for both real_mode_address and high_address to indicate that it can support both 32-bit and 64-bit resume environments. So if a BIOS favors 32-bit resume environment, it can resume from here; if another BIOS favors 64-bit resume environment, it can resume from there. And host OSes can be implemented using only 1 binary to work with both BIOSes. Thanks and best regards -Lv ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?