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

[Zheng, Lv]

Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@xxxxxxxxxxxxx]
> Sent: Thursday, June 25, 2015 7:24 AM
> To: Zheng, Lv
> 
> 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?

The story is:
According to the test, if both 32-bit waking vector and 64-bit waking vector is set by the OSPM, BIOSes only support 32-bit resume environment will jump to the 32-bit waking vector address and BIOSes support 64-bit resume environment will jump to 64-bit waking vector.
So they can be set by the OSPMs simultaneously to indicate that the OSPM can support both resume environments.
That's why ACPICA interface is changed.

Thanks and best regards
-Lv