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

[Laszlo Ersek <lersek@redhat.com>]

On 10/16/17 19:31, Duran, Leo wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 16, 2017 12:06 PM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; Duran, Leo
>> <leo.duran@amd.com>
>> Cc: edk2-devel@lists.01.org; Paolo Bonzini <pbonzini@redhat.com>
>> Subject: Re: [edk2] [PATCH v5 0/2] Enhanced SMM support for AMD-based
>> x86 systems.
>>
>> 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 :) )
>>
> 
> Lazlo,
> I purposely left out changes to OVMF and Quark, consistent with previous feedback.

I've found my previous comments:

http://mid.mail-archive.com/2d3efa5a-ad72-bb35-1e6a-b9b78379337c@redhat.com

There I only suggested a different (more telling) subject for the
OvmfPkg patch, and wrote,

> (Of course I realize the patch might entirely be replaced in the next
> version, based on Jiewen's and Mike's feedback -- that's OK with me, I
> just wanted to give an example.)

I didn't try to validate Jiewen's / Mike's feedback; I just stated *if*,
according to them, patching OvmfPkg was not necessary, I'd be OK with that.

Since we're talking anyway, can you (and/or Jiewen & Mike) please state
the problem being solved here, and explain why patching the
SmmCpuFeaturesLib instance in OvmfPkg is, or is not, necessary to update?

Hmmm... Is it the case that

  UefiCpuPkg/Library/SmmCpuFeaturesLib

runs correctly on Intel *hosts* only at the moment (so it needs fixing,
for AMD *hosts*), while

  OvmfPkg/Library/SmmCpuFeaturesLib

deals with AMD-looking *guests* anyway, so it needs no fixing, for AMD
compatibility?

If this is correct, then I agree patch #1 does not need to be duplicated
for OvmfPkg.

*However*, in turn, patch #2 (for PiSmmCpuDxeSmm) might be necessary to
update for QEMU. PiSmmCpuDxeSmm runs on both bare metal and on QEMU.
And, as Paolo says, a pure CPUID / manufacturer check (for determining
the state save layout) is wrong on QEMU, even if the same would work on
bare metal:

@@ -547,6 +595,20 @@ PiCpuSmmEntry (
     );

   //
+  // Override SMRAM offsets for AMD
+  //
+  if (StandardSignatureIsAuthenticAMD ()) {
+    gSmmSmramStateMapOffset = AMD_SMRAM_SAVE_STATE_MAP_OFFSET;
+    gSmmPsdOffset = AMD_SMM_PSD_OFFSET;
+  }
+

If patch v5 2/2 is merely a refactoring (i.e., it causes PiSmmCpuDxeSmm
to behave exactly the same as before, just with an improved
implementation), then I agree a CPUID-based check is not necessarily a
bug (regression). Instead, it might be called a missed opportunity (or,
more nicely put, a "basis") for bringing PiSmmCpuDxeSmm closer to QEMU.

My apologies if I'm confused.

Thanks
Laszlo