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

["Duran, Leo" <leo.duran@amd.com>]

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, October 16, 2017 1:44 PM
> To: Duran, Leo <leo.duran@amd.com>; Yao, Jiewen
> <jiewen.yao@intel.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/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.

Lazlo,
Yes, that is my understanding.

> 
> *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.
Lazlo,

1) Per feedback from Yao:
I'm currently investigating if we *really*really* have to change the PSD offset.

2) Per feedback from Paolo:
And if the PSD offset change is indeed required, I will investigate an alternate detection mechanism.

Thanks.
Leo

> 
> My apologies if I'm confused.
> 
> Thanks
> Laszlo