Hi, Rafael
> From: Rafael J. Wysocki [mailto:r...@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 <o...@kde.org>
> > Signed-off-by: Lv Zheng <lv.zh...@intel.com>
> > Signed-off-by: Bob Moore <robert.mo...@intel.com>
> > ---
> > 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�����r��y����b�X��ǧv�^�){.n�+����{����zX����ܨ}���Ơz�&j:+v�������zZ+��+zf���h���~����i���z� �w���?�����&�)ߢf��^jǫy�m��@A�a���
0��h���i
