Re: [PATCH v5 0/2] Enhanced SMM support for AMD-based x86 systems.

[Laszlo Ersek <lersek@redhat.com>]

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 <leo.duran@amd.com> 写道：
>>
>>
>>
>>> -----Original Message-----
>>> From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
>>> Sent: Thursday, October 12, 2017 8:53 PM
>>> To: Duran, Leo <leo.duran@amd.com>; 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
>