Re: [PATCH v2 03/28] ACPICA: Hardware: Enable 64-bit firmware waking vector for selected FACS.

[Rafael J. Wysocki]

On Wednesday, June 24, 2015 04:05:42 PM Rafael J. Wysocki wrote:
> On Wednesday, June 24, 2015 11:02:10 AM Lv Zheng wrote:
> > ACPICA commit 7aa598d711644ab0de5f70ad88f1e2de253115e4
> > 
> > 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 1.
> > 
> > ACPI specification says:
> > A. 32-bit FACS address (FIRMWARE_CTRL field in FADT):
> >    Physical memory address of the FACS, where OSPM and firmware exchange
> >    control information.
> >    If the X_FIRMWARE_CTRL field contains a non zero value then this field
> >    must be zero.
> >    A zero value indicates that no FACS is specified by this field.
> > B. 64-bit FACS address (X_FIRMWARE_CTRL field in FADT):
> >    64bit physical memory address of the FACS.
> >    This field is used when the physical address of the FACS is above 4GB.
> >    If the FIRMWARE_CTRL field contains a non zero value then this field
> >    must be zero.
> >    A zero value indicates that no FACS is specified by this field.
> > Thus the 32bit and 64bit firmware waking vector should indicate completely
> > different resuming environment - real mode (1MB addressable) and non real
> > mode (4GB+ addressable) and currently Linux only supports resuming from
> > real mode.
> > 
> > This patch enables 64-bit firmware waking vector for selected FACS via
> > acpi_set_firmware_waking_vector() so that it's up to OSPMs to determine which
> > resuming mode should be used by BIOS and ACPICA changes won't trigger the
> > bugs caused by the root cause 1. For example, Linux can pass
> > physical_address64=0 as the parameter of acpi_set_firmware_waking_vector() to
> > indicate no 64bit waking vector support. Lv Zheng.
> > 
> > This patch also updates acpi_set_firmware_waking_vector() invocations in
> > order to keep 32-bit firmware waking vector favor for Linux. 64-bit
> > firmware waking vector has never been enabled by Linux.  The
> > (acpi_physical_address)0 for 64-bit address can be used to force ACPICA to
> > set only 32-bit firmware waking vector for Linux.
> > 
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=74021
> > Link: https://github.com/acpica/acpica/commit/7aa598d7
> > Cc: 3.14.1+ <stable@xxxxxxxxxxxxxxx> # 3.14.1+
> > Reported-and-tested-by: Oswald Buddenhagen <ossi@xxxxxxx>
> > Signed-off-by: Lv Zheng <lv.zheng@xxxxxxxxx>
> > Signed-off-by: Bob Moore <robert.moore@xxxxxxxxx>
> > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> > Cc: x86@xxxxxxxxxx
> > Cc: Tony Luck <tony.luck@xxxxxxxxx>
> > Cc: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Cc: linux-ia64@xxxxxxxxxxxxxxx
> > ---
> >  arch/ia64/include/asm/acpi.h    |    3 +-
> >  arch/ia64/kernel/acpi.c         |    2 --
> >  arch/x86/include/asm/acpi.h     |    3 +-
> >  drivers/acpi/acpica/hwxfsleep.c |   61 ++++++++++++---------------------------
> >  drivers/acpi/sleep.c            |    8 +++--
> >  include/acpi/acpixf.h           |   11 +++----
> >  6 files changed, 33 insertions(+), 55 deletions(-)
> > 
> > diff --git a/arch/ia64/include/asm/acpi.h b/arch/ia64/include/asm/acpi.h
> > index aa0fdf1..0ac4fab 100644
> > --- a/arch/ia64/include/asm/acpi.h
> > +++ b/arch/ia64/include/asm/acpi.h
> > @@ -79,7 +79,8 @@ int acpi_gsi_to_irq (u32 gsi, unsigned int *irq);
> >  /* Low-level suspend routine. */
> >  extern int acpi_suspend_lowlevel(void);
> >  
> > -extern unsigned long acpi_wakeup_address;
> > +#define acpi_wakeup_address	((acpi_physical_address)0)
> > +#define acpi_wakeup_address64	((acpi_physical_address)0)
> >  
> >  /*
> >   * Record the cpei override flag and current logical cpu. This is
> > diff --git a/arch/ia64/kernel/acpi.c b/arch/ia64/kernel/acpi.c
> > index b1698bc..1b08d6f 100644
> > --- a/arch/ia64/kernel/acpi.c
> > +++ b/arch/ia64/kernel/acpi.c
> > @@ -60,8 +60,6 @@ int acpi_lapic;
> >  unsigned int acpi_cpei_override;
> >  unsigned int acpi_cpei_phys_cpuid;
> >  
> > -unsigned long acpi_wakeup_address = 0;
> > -
> >  #ifdef CONFIG_IA64_GENERIC
> >  static unsigned long __init acpi_find_rsdp(void)
> >  {
> > diff --git a/arch/x86/include/asm/acpi.h b/arch/x86/include/asm/acpi.h
> > index 3a45668..fc9608d 100644
> > --- a/arch/x86/include/asm/acpi.h
> > +++ b/arch/x86/include/asm/acpi.h
> > @@ -72,7 +72,8 @@ static inline void acpi_disable_pci(void)
> >  extern int (*acpi_suspend_lowlevel)(void);
> >  
> >  /* Physical address to resume after wakeup */
> > -#define acpi_wakeup_address ((unsigned long)(real_mode_header->wakeup_start))
> > +#define acpi_wakeup_address	((acpi_physical_address)(real_mode_header->wakeup_start))
> > +#define acpi_wakeup_address64	((acpi_physical_address)(0))
> 
> Do we need this?

In fact, I don't see why we need to redefine the symbols at all.

Couldn't acpi_set_firmware_waking_vector() be defined to take u32 and u64 so
we could just pass acpi_wakeup_address (as already defined) as the first argument
and 0 as the second argument to it?  The back-and-forth type casts from and
to acpi_physical_address don't look entirely clean to me.

Moreover, I don't really see a functional difference between the old and the
new code.

The old code does "set the 32-bit waking vector and clear the 64-bit waking
vector if present".  The new code does "set the 32-bit waking vector and
either clear the 64-bit one if present, or assign the second function argument
to it", but we always pass 0 as the second argument (which is *extremely*
obfuscated in your patch), so I really don't see the difference here.

Am I missing anything?

Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/