RE: [PATCH v2 05/28] ACPICA: Hardware: Enable firmware waking vector for both 32-bit and 64-bit FACS.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> 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 <ossi@xxxxxxx>
> > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx>
> > Signed-off-by: Bob Moore <robert.moore@xxxxxxxxx>
> > ---
> >  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�����{�����ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f




[Index of Archives]     [Linux IBM ACPI]     [Linux Power Management]     [Linux Kernel]     [Linux Laptop]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux