From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CE6E8211B698D for ; Fri, 11 Jan 2019 01:46:41 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4096D80F9F; Fri, 11 Jan 2019 09:46:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-3.rdu2.redhat.com [10.10.120.3]) by smtp.corp.redhat.com (Postfix) with ESMTP id F040D5D6A9; Fri, 11 Jan 2019 09:46:39 +0000 (UTC) To: Eric Dong , edk2-devel@lists.01.org Cc: Aleksiy References: <20190111054639.25528-1-eric.dong@intel.com> <20190111054639.25528-3-eric.dong@intel.com> From: Laszlo Ersek Message-ID: Date: Fri, 11 Jan 2019 10:46:38 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190111054639.25528-3-eric.dong@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 11 Jan 2019 09:46:41 +0000 (UTC) Subject: Re: [Patch 2/2] MdeModulePkg/BootScriptExecuteorDxe: Use correct field name. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 11 Jan 2019 09:46:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/11/19 06:46, Eric Dong wrote: > ((Facs->Flags & EFI_ACPI_4_0_OSPM_64BIT_WAKE__F) != 0)) > > In above code, Facs->OspmFlags should be used instead. > EFI_ACPI_4_0_OSPM_64BIT_WAKE__F is a bit in OSPM Enabled Firmware > Control Structure Flags field, not in Firmware Control Structure > Feature Flags. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1430 I've now checked the ACPI 6.2 spec, and the bug report is correct. (1) The subject line of the patch is unhelpful. Instead, please use: MdeModulePkg/BootScriptExecuteorDxe: check 64BIT_WAKE_F in FACS.OSPMFlags This is 73 characters, uses names from the ACPI spec, and describes the change better. > Cc: Aleksiy > Cc: Jian Wang > Contributed-under: TianoCore Contribution Agreement 1.1 > signed-off-by: Eric Dong (2) Please remove this line; it is both mis-spelled (due to the lower-case "s"), and the next line makes it superfluous. > Signed-off-by: Eric Dong > --- > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c | 4 ++-- > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c > index e76abb7b7b..13af957a4d 100644 > --- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c > +++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/ScriptExecute.c > @@ -4,7 +4,7 @@ > This driver is dispatched by Dxe core and the driver will reload itself to ACPI reserved memory > in the entry point. The functionality is to interpret and restore the S3 boot script > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> Copyright (c) 2017, AMD Incorporated. All rights reserved.
> > This program and the accompanying materials > @@ -156,7 +156,7 @@ S3BootScriptExecutorEntryFunction ( > TempStackTop = (UINTN)&TempStack + sizeof(TempStack); > if ((Facs->Version == EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_VERSION) && > ((Facs->Flags & EFI_ACPI_4_0_64BIT_WAKE_SUPPORTED_F) != 0) && > - ((Facs->Flags & EFI_ACPI_4_0_OSPM_64BIT_WAKE__F) != 0)) { > + ((Facs->OspmFlags & EFI_ACPI_4_0_OSPM_64BIT_WAKE__F) != 0)) { > // > // X64 long mode waking vector > // > diff --git a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c > index 1c6bb47b60..7b9627d579 100644 > --- a/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c > +++ b/MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/X64/SetIdtEntry.c > @@ -3,7 +3,7 @@ > > Set a IDT entry for interrupt vector 3 for debug purpose for x64 platform > > -Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> Copyright (c) 2017, AMD Incorporated. All rights reserved.
> > > @@ -117,7 +117,7 @@ IsLongModeWakingVector ( > if (Facs->XFirmwareWakingVector != 0) { > if ((Facs->Version == EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_VERSION) && > ((Facs->Flags & EFI_ACPI_4_0_64BIT_WAKE_SUPPORTED_F) != 0) && > - ((Facs->Flags & EFI_ACPI_4_0_OSPM_64BIT_WAKE__F) != 0)) { > + ((Facs->OspmFlags & EFI_ACPI_4_0_OSPM_64BIT_WAKE__F) != 0)) { > // Both BIOS and OS wants 64bit vector > if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) { > return TRUE; > Otherwise, the patch looks correct; all 64BIT_WAKE_F mentions are covered, under BootScriptExecuteorDxe. (In fact, under all of MdeModulePkg.) The patch is expected to make no difference on QEMU / OVMF, because QEMU exports a revision 1 FACS table for OVMF to install, and the expressions that are modified by the patch are never evaluated. But, I'll report back with test results too. With (1) and (2) addressed: Reviewed-by: Laszlo Ersek Thanks, Laszlo