Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752137AbbFXW6N (ORCPT ); Wed, 24 Jun 2015 18:58:13 -0400 Received: from v094114.home.net.pl ([79.96.170.134]:45320 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1751962AbbFXW6D (ORCPT ); Wed, 24 Jun 2015 18:58:03 -0400 From: "Rafael J. Wysocki" To: Lv Zheng Cc: "Rafael J. Wysocki" , Len Brown , Lv Zheng , linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org, Bob Moore , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , x86@kernel.org, Tony Luck , Fenghua Yu , linux-ia64@vger.kernel.org Subject: Re: [PATCH v2 03/28] ACPICA: Hardware: Enable 64-bit firmware waking vector for selected FACS. Date: Thu, 25 Jun 2015 01:24:11 +0200 Message-ID: <3959851.uJ0udqUY0b@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.1.0-rc5+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <2100131.SXLpyPbXcH@vostro.rjw.lan> References: <7ca3239e4066d1a33c523b1ccb475bcbfa6448d3.1435114811.git.lv.zheng@intel.com> <2100131.SXLpyPbXcH@vostro.rjw.lan> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6903 Lines: 144 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+ # 3.14.1+ > > Reported-and-tested-by: Oswald Buddenhagen > > Signed-off-by: Lv Zheng > > Signed-off-by: Bob Moore > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: x86@kernel.org > > Cc: Tony Luck > > Cc: Fenghua Yu > > Cc: linux-ia64@vger.kernel.org > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/