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 722B1202E60EA for ; Mon, 16 Oct 2017 10:02:30 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0BD90820F0; Mon, 16 Oct 2017 17:06:05 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 0BD90820F0 Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx02.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-250.rdu2.redhat.com [10.10.120.250]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8B45117F35; Mon, 16 Oct 2017 17:06:03 +0000 (UTC) To: "Yao, Jiewen" , "Duran, Leo" Cc: "edk2-devel@lists.01.org" , Paolo Bonzini References: <1507751131-32404-1-git-send-email-leo.duran@amd.com> <74D8A39837DF1E4DA445A8C0B3885C503A9E505E@SHSMSX104.ccr.corp.intel.com> <279D7F72-28F1-4DAD-B75E-780F2638BA5E@intel.com> From: Laszlo Ersek Message-ID: Date: Mon, 16 Oct 2017 19:06:02 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <279D7F72-28F1-4DAD-B75E-780F2638BA5E@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 16 Oct 2017 17:06:05 +0000 (UTC) Subject: Re: [PATCH v5 0/2] Enhanced SMM support for AMD-based x86 systems. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 16 Oct 2017 17:02:30 -0000 Content-Type: text/plain; charset=UTF-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 10/15/17 02:58, Yao, Jiewen wrote: > for runtime test, I recommend using ovmf. You don't need real hardware. It can run both 32bit or 64bit. It can run in both Linux and windows. > > You need use -D SMM_REQUIRE option to build ovmf. > If you have any problem, Laszlo is the good contact. I don't have much context about this series, but looking at the blurb, I see that version 3 removed OvmfPkg patches: > Changes since v2: > The intent of this revision is to maintain compatibility with existing > packages. To that end, changes to OvmgfPkg and QuarkSocPkg are > reverted. Moreover, pertinent macros are replaced in the C code, > rather than on header files that are shared globally. Judged on the diffstat of patch #1 -- only "UefiCpuPkg/Library/SmmCpuFeaturesLib" files are modified -- I would say that changes in patch #1 are invisible to OVMF. The reason is that OVMF uses a separate SmmCpuFeaturesLib instance, namely OvmfPkg/Library/SmmCpuFeaturesLib This means two things: - changes from patch #1 cannot be tested with OVMF, simply because "UefiCpuPkg/Library/SmmCpuFeaturesLib" is never built for OVMF; - changes from patch #2 may or may not break SMM in OVMF, dependent on whether patch #2 is tied closely to patch #1. In order to see why OvmfPkg has a separate SmmCpuFeaturesLib instance, please review the commit log: git log --reverse -- OvmfPkg/Library/SmmCpuFeaturesLib At this point I cannot determine if this patch set should ignore OvmfPkg completely, or else patch #1 should be duplicated for "OvmfPkg/Library/SmmCpuFeaturesLib" as well. (I guess I don't understand the goal of the patch set -- I've read the blurb, but the problem has not been stated well enough for me to understand. Or maybe it was stated long ago, and I've forgotten it :) ) Leo, I have a separate request: the diffstats in the blurb and on patch #1 are practically unreadable. This is because edk2 uses long names for files and directories, and because the nesting can get deep -- and "git" by default truncates the diffstat to quite narrow lines. In order to avoid this, I recommend formatting edk2 patch sets as follows: git format-patch --stat=1000 --stat-graph-width=20 ... This makes the pathname column just as wide as necessary, while keeping the actual "stats" column reasonably narrow. Thanks, Laszlo >> 在 2017年10月15日，上午12:04，Duran, Leo 写道： >> >> >> >>> -----Original Message----- >>> From: Yao, Jiewen [mailto:jiewen.yao@intel.com] >>> Sent: Thursday, October 12, 2017 8:53 PM >>> To: Duran, Leo ; edk2-devel@lists.01.org >>> Subject: RE: [edk2] [PATCH v5 0/2] Enhanced SMM support for AMD-based >>> x86 systems. >>> >>> HI Leo >>> Thank you very much. This patch looks good to me in general. >>> >>> Some minor comment: >>> >>> 1) For AMD smm save state. >>> I saw Paolo gave the comment on how to detect AMD save state. I do not >>> have strong opinion on that. I think you and Paolo can make decision. >>> >>> I recommend we move AMD_SMRAM_SAVE_STATE_MAP_OFFSET to >>> UefiCpuPkg\Include\Register\Amd\SmramSaveStateMap.h, because it is >>> standard. >> Hi Yao, >> >> Sure thing, I will do that. >> >> Leo >> >>> +// >>> +// Definitions for AMD systems are based on contents of the // AMD64 >>> +Architecture Programmer's Manual // Volume 2: System Programming, >>> +Section 10 System-Management Mode // #define >>> +AMD_SMRAM_SAVE_STATE_MAP_OFFSET 0xfe00 >>> >>> We can leave AMD_SMM_PSD_OFFSET in UefiCpuPkg/PiSmmCpuDxeSmm, >>> if it is implementation specific. >>> +#define AMD_SMM_PSD_OFFSET 0xfc00 >>> >>> >>> >>> 2) For Intel save state, I assume you already did some test to make sure >>> there is no regression. >>> If so, would you please add some description on what test you have done? >>> For example, >>> If both IA32 and X64 are validated? >>> If all three .S, .asm, .nasm are validated? >>> If OS boot and S3 resume are validated? >>> >>> If you did any other test, please also add. >>> >> >> Hi Yao, >> >> I have not done runtime validation testing. >> Instead, I tested the assembly code snippets in a vacuum (in their .asm, .nasm, and .S forms), >> to ensure the source produced the expected compiled code, and the expected execution. >> >> So any runtime 'Tested-by' would be greatly appreciated >> Thanks, >> Leo. >> >>> Thank you >>> Yao Jiewen >>> >>> >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of >>>> Leo Duran >>>> Sent: Thursday, October 12, 2017 3:45 AM >>>> To: edk2-devel@lists.01.org >>>> Subject: [edk2] [PATCH v5 0/2] Enhanced SMM support for AMD-based x86 >>>> systems. >>>> >>>> This patch-set replaces Intel-specific macros with global variables to >>>> provide support for AMD-based x86 systems. >>>> >>>> The replaced macros are: >>>> 1) SRAM_SAVE_STATE_MAP_OFFSET >>>> 2) TXT_SMM_PSD_OFFSET >>>> 3) SMM_PSD_OFFSET >>>> >>>> Changes since v4: >>>> Make runtime CPUID checks and use global variables instead of PCD's. >>>> >>>> Changes since v3: >>>> Correction on cover letter. >>>> >>>> Changes since v2: >>>> The intent of this revision is to maintain compatibility with existing >>>> packages. To that end, changes to OvmgfPkg and QuarkSocPkg are >>> reverted. >>>> Moreover, pertinent macros are replaced in the C code, rather than on >>>> header files that are shared globally. >>>> >>>> Changes since v1: >>>> Revision to Cc list for UefiCpuPkg. >>>> >>>> Leo Duran (2): >>>> UefiCpuPkg/SmmCpuFeaturesLib: Use global variables to replace macros >>>> UefiCpuPkg/PiSmmCpuDxeSmm: Use global variables to replace macros >>>> >>>> .../Library/SmmCpuFeaturesLib/Ia32/SmiEntry.S | 28 ++++++--- >>>> .../Library/SmmCpuFeaturesLib/Ia32/SmiEntry.asm | 29 ++++++--- >>>> .../Library/SmmCpuFeaturesLib/Ia32/SmiEntry.nasm | 43 +++++++++---- >>>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCommon.h | 48 >>>> +++++++++++++++ >>>> .../Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 59 >>>> +++++++++++++++--- >>>> .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf | 3 + >>>> .../SmmCpuFeaturesLib/SmmCpuFeaturesLibStm.inf | 3 + >>>> UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmStm.c | 39 ++++++++++- >>> - >>>> .../Library/SmmCpuFeaturesLib/X64/SmiEntry.S | 28 ++++++--- >>>> .../Library/SmmCpuFeaturesLib/X64/SmiEntry.asm | 30 ++++++--- >>>> .../Library/SmmCpuFeaturesLib/X64/SmiEntry.nasm | 47 ++++++++++---- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Semaphore.c | 22 ++++--- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 28 ++++++--- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 21 +++++-- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 43 >>>> +++++++++---- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 72 >>>> ++++++++++++++++++++-- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 17 ++++- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 18 >>> +++--- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 20 +++--- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/Semaphore.c | 22 ++++--- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.S | 34 ++++++---- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm | 22 +++++-- >>>> UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 45 >>>> ++++++++++---- >>>> 23 files changed, 547 insertions(+), 174 deletions(-) create mode >>>> 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCommon.h >>>> >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >