From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.42086.1683659505409062650 for ; Tue, 09 May 2023 12:11:45 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=oOtLPOBZ; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id 233BF20AECB2; Tue, 9 May 2023 12:11:42 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 233BF20AECB2 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1683659504; bh=yw50Jwi+rmCD+W3U7XwbfvfB5Lb3H8NDeBLOjsfU0jI=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=oOtLPOBZ9YaJh93I8LCVnLFN0a10j6ROzK32+ydsTquNLtS/62YzfViJW0x8LrwXO jSjSiV59LPk5ZCVsSGa/Cdh+9lHJ16YYFvm6NfZeJ38xWM+ajCNbly6oDLMKniDn48 +YpPnywF1f3H0BKDTm3PU8DvMVTffx6ziz/dYJJ0= Message-ID: <330175a0-22a9-7a26-8d99-f6b87d3c70d6@linux.microsoft.com> Date: Tue, 9 May 2023 15:11:41 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [edk2-devel] [PATCH v11 0/8] Adds AmdSmmCpuFeaturesLib and MmSaveStateLib To: devel@edk2.groups.io, abdattar@amd.com Cc: Paul Grimes , Abner Chang , Eric Dong , Ray Ni , Rahul Kumar , Gerd Hoffmann , Michael D Kinney , Liming Gao , Zhiguang Liu , Ard Biesheuvel , Jiewen Yao , Jordan Justen References: From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit I haven't followed the whole thread but looking at v11, I wanted to check on the following. 1. The library source files (for AmdMmSaveStateLib and IntelMmSaveStateLib) include several library headers (particularly in MmSaveStateLib/MmSaveState.h) but do not have a [LibraryClasses] section in their INF files. Is there a reason? 2. AmdMmSaveStateLib and IntelMmSaveStateLib depend on SmmServicesTableLib. Can they depend on MmServicesTableLib instead? 3. It seems resources like the EFER register LMA bit should be defined somewhere agnostic to the MmSaveState.h file in MmSaveStateLib. It is referenced in MdePkg/Include/Register/Intel/ArchitecturalMsr.h: https://github.com/tianocore/edk2/blob/5215cd5baf6609e54050c69909273b7f5161c59e/MdePkg/Include/Register/Intel/ArchitecturalMsr.h#L6342 In any case, can the bit be defined in one location? 4. Your change to the SmmCpuFeaturesLib.h interface is not backward compatible and a build breaking change. I think that should be called out in your cover letter. Thanks, Michael On 5/6/2023 12:06 AM, Abdul Lateef Attar via groups.io wrote: > PR: https://github.com/tianocore/edk2/pull/4341 > > V11: Delta changes > Drop the OVMF implementation of MmSaveStateLib > V10: Delta changes: > Addressed review comments from Abner. > V9: Delta changes: > Addressed review comments. > Rename to MmSaveStateLib. > Also rename SMM_ defines to MM_. > Implemented OVMF MmSaveStateLib. > Removes SmmCpuFeaturesReadSaveStateRegister and SmmCpuFeaturesWriteSaveStateRegister > function interface. > V8 delta changes: > Addressed review comments from Abner, > Fix the whitespace error. > Seperate the Ovmf changes to another patch > V7 delta changes: > Adds SmmSmramSaveStateLib for Intel processor. > Integrate SmmSmramSaveStateLib library. > V6 delta changes: > Addressed review comments for Ray NI. > removed unnecessary EFIAPI. > V5 delta changes: > rebase to master branch. > updated Reviewed-by > V4 delta changes: > rebase to master branch. > added reviewed-by. > V3 delta changes: > Addressed review comments from Abner chang. > Re-arranged patch order. > > Cc: Paul Grimes > Cc: Abner Chang > Cc: Eric Dong > Cc: Ray Ni > Cc: Rahul Kumar > Cc: Gerd Hoffmann > Cc: Michael D Kinney > Cc: Liming Gao > Cc: Zhiguang Liu > Cc: Ard Biesheuvel > Cc: Jiewen Yao > Cc: Jordan Justen > Cc: Abdul Lateef Attar > > Abdul Lateef Attar (8): > MdePkg: Adds AMD SMRAM save state map > UefiCpuPkg: Adds MmSaveStateLib library class > UefiCpuPkg: Implements MmSaveStateLib library instance > UefiCpuPkg/SmmCpuFeaturesLib: Restructure arch-dependent code > UefiCpuPkg: Implements SmmCpuFeaturesLib for AMD Family > UefiCpuPkg: Implements MmSaveStateLib for Intel > UefiCpuPkg: Removes SmmCpuFeaturesReadSaveStateRegister > OvmfPkg: Uses MmSaveStateLib library > > UefiCpuPkg/UefiCpuPkg.dec | 4 + > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 3 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > UefiCpuPkg/UefiCpuPkg.dsc | 14 + > .../MmSaveStateLib/AmdMmSaveStateLib.inf | 28 + > .../MmSaveStateLib/IntelMmSaveStateLib.inf | 28 + > .../AmdSmmCpuFeaturesLib.inf | 38 + > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf | 2 + > .../Include/Register/Amd/SmramSaveStateMap.h | 194 +++++ > UefiCpuPkg/Include/Library/MmSaveStateLib.h | 70 ++ > .../Include/Library/SmmCpuFeaturesLib.h | 52 -- > .../Library/MmSaveStateLib/MmSaveState.h | 102 +++ > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 56 +- > .../SmmCpuFeaturesLib/SmmCpuFeaturesLib.c | 767 ------------------ > .../Library/MmSaveStateLib/AmdMmSaveState.c | 309 +++++++ > .../Library/MmSaveStateLib/IntelMmSaveState.c | 413 ++++++++++ > .../MmSaveStateLib/MmSaveStateCommon.c | 138 ++++ > .../SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c | 387 +++++++++ > .../IntelSmmCpuFeaturesLib.c | 70 ++ > .../SmmCpuFeaturesLibCommon.c | 128 --- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 11 +- > UefiCpuPkg/PiSmmCpuDxeSmm/SmramSaveState.c | 500 +----------- > MdePkg/MdePkg.ci.yaml | 4 +- > 24 files changed, 1812 insertions(+), 1508 deletions(-) > create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveStateLib.inf > create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveStateLib.inf > create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.inf > create mode 100644 MdePkg/Include/Register/Amd/SmramSaveStateMap.h > create mode 100644 UefiCpuPkg/Include/Library/MmSaveStateLib.h > create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveState.h > create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/AmdMmSaveState.c > create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/IntelMmSaveState.c > create mode 100644 UefiCpuPkg/Library/MmSaveStateLib/MmSaveStateCommon.c > create mode 100644 UefiCpuPkg/Library/SmmCpuFeaturesLib/AmdSmmCpuFeaturesLib.c >