Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Paolo Bonzini]

On 12/10/2015 18:23, Paolo Bonzini wrote:
> 
> 
> On 05/10/2015 01:57, Michael Kinney wrote:
>> Add module that initializes a CPU for the SMM envirnment and
>> installs the first level SMI handler.  This module along with the
>> SMM IPL and SMM Core provide the services required for
>> DXE_SMM_DRIVERS to register hardware and software SMI handlers.
>>
>> CPU specific features are abstracted through the SmmCpuFeaturesLib
>>
>> Platform specific features are abstracted through the
>> SmmCpuPlatformHookLib
>>
>> Several PCDs are added to enable/disable features and configure
>> settings for the PiSmmCpuDxeSmm module
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Kinney 
> 
> Hi Michael,
> 
> I'm happy to report the first bug! :)
> 
> InitPaging() is setting a page directory entry before initializing the
> corresponding page table.  This works on real hardware (including KVM),
> but the TLB of QEMU's emulation mode is different (possibly it has
> different  associativity, I don't really know) so at some point
> execution goes to nowhere's land.
> 
> The fix is really simple:

As suggested by Jordan, here's the patch again but with all the
standard signoffs.

[pbonz...@redhat.com: InitPaging: prepare PT before filling in PDE]
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Paolo Bonzini 

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 9463e97..6ee9256 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -555,12 +555,12 @@ InitPaging (
   Pt = AllocatePages (1);
   ASSERT (Pt != NULL);
   
-  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
-  
   // Split it
-  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++, Pt++) {
-*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P);
+  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) {
+Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P);
   } // end for PT
+
+  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
 } // end if IsAddressSplit
   } // end for PTE
 } // end for PDE
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Paolo Bonzini]

On 05/10/2015 01:57, Michael Kinney wrote:
> Add module that initializes a CPU for the SMM envirnment and
> installs the first level SMI handler.  This module along with the
> SMM IPL and SMM Core provide the services required for
> DXE_SMM_DRIVERS to register hardware and software SMI handlers.
> 
> CPU specific features are abstracted through the SmmCpuFeaturesLib
> 
> Platform specific features are abstracted through the
> SmmCpuPlatformHookLib
> 
> Several PCDs are added to enable/disable features and configure
> settings for the PiSmmCpuDxeSmm module
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Michael Kinney 

Hi Michael,

I'm happy to report the first bug! :)

InitPaging() is setting a page directory entry before initializing the
corresponding page table.  This works on real hardware (including KVM),
but the TLB of QEMU's emulation mode is different (possibly it has
different  associativity, I don't really know) so at some point
execution goes to nowhere's land.

The fix is really simple:

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index 9463e97..6ee9256 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -555,12 +555,12 @@ InitPaging (
   Pt = AllocatePages (1);
   ASSERT (Pt != NULL);

-  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
-
   // Split it
-  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++,
Pt++) {
-*Pt = Address + ((Level4 << 12) | IA32_PG_RW | IA32_PG_P);
+  for (Level4 = 0; Level4 < SIZE_4KB / sizeof(*Pt); Level4++) {
+Pt[Level4] = Address + ((Level4 << 12) | IA32_PG_RW |
IA32_PG_P);
   } // end for PT
+
+  *Pte = (UINTN)Pt | IA32_PG_RW | IA32_PG_P;
 } // end if IsAddressSplit
   } // end for PTE
 } // end for PDE

Thanks,

Paolo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Kinney, Michael D]

Laszlo,

Thanks for the feedback.  I agree I need to update UefiCpuPkg DSC file.

Mike

>-Original Message-
>From: Laszlo Ersek [mailto:ler...@redhat.com]
>Sent: Friday, October 09, 2015 6:57 AM
>To: Kinney, Michael D
>Cc: edk2-de...@ml01.01.org
>Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
>
>Mike,
>
>On 10/05/15 01:57, Michael Kinney wrote:
>> Add module that initializes a CPU for the SMM envirnment and
>> installs the first level SMI handler.  This module along with the
>> SMM IPL and SMM Core provide the services required for
>> DXE_SMM_DRIVERS to register hardware and software SMI handlers.
>>
>> CPU specific features are abstracted through the SmmCpuFeaturesLib
>>
>> Platform specific features are abstracted through the
>> SmmCpuPlatformHookLib
>>
>> Several PCDs are added to enable/disable features and configure
>> settings for the PiSmmCpuDxeSmm module
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Kinney <michael.d.kin...@intel.com>
>> ---
>
>I found that this version of PiSmmCpuDxeSmm depends on
>CpuExceptionHandlerLib (Quark's doesn't).
>
>The following interfaces from the library class are called:
>- InitializeCpuExceptionHandlers()
>- RegisterCpuInterruptHandler()
>
>I think this makes sense. The call sites are:
>
>  PiCpuSmmEntry
>InitializeSmmIdt
>  InitializeCpuExceptionHandlers
>
>  SmmRestoreCpu
>InitializeCpuExceptionHandlers
>
>  SmmRegisterExceptionHandler ==
>mSmmCpuService.RegisterExceptionHandler
>RegisterCpuInterruptHandler
>
>What I don't understand though is why the CpuExceptionHandlerLib class
>is resolved to a NULL instance, in "UefiCpuPkg/UefiCpuPkg.dsc":
>
>
>CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNul
>l/CpuExceptionHandlerLibNull.inf
>
>OVMF uses the following non-null instances (for the appropriate module
>types):
>
>-
>UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.i
>nf
>-
>UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf
>
>and as far as I remember, these exception handler library instances
>print, by default, those fault information dumps (to the serial port)
>that are very helpful for debugging incorrect pointer references and the
>like.
>
>So why doesn't this module use the corresponding SMM driver instance:
>
>-
>UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.in
>f
>
>and why does it use a NULL instance instead?

Good point.  The DSC file in the UefiCpuPkg is there to test the build of the
Modules/libraries, so the specific lib instance from another package is not
critical.  I agree it makes sense to put in the preferred lib mappings is 
possible
So I will update the DSC file for UefiCpuPkg to use the correct 
CpuExceptionHandlerLib for each module type.

>
>I realize that this module has its own fault info dumping facility
>(called SmiPFHandler(), specific to Ia32 vs. X64), similarly to the
>version in Quark. That handler calls DumpModuleInfoByIp(), which should
>do what I like to have -- helpful location info.
>
>However, SmiPFHandler() is registered with SmmRegisterExceptionHandler()
>-- since PcdCpuSmmProfileEnable defaults to FALSE --, and that call
>ultimately ends up in CpuExceptionHandlerLib. See the third call tree
>excerpt at the top -- in total, we have:
>
>SmmInitPageTable()
>  SmmRegisterExceptionHandler(SmiPFHandler)
>RegisterCpuInterruptHandler(SmiPFHandler)
>  // implemented by CpuExceptionHandlerLib
>
>Now, given that the CpuExceptionHandlerLib instance is the Null one here
>(according to UefiCpuPkg/UefiCpuPkg.dsc), this PF handler will actually
>*not* be installed. Is that your intent?
>
>I don't think so. Your patch doesn't seem to affect the
>CpuExceptionHandlerLib resolutions in any way; therefore I believe this
>is an omission in the patch.
>
>And, I think it doesn't only affect PiSmmCpuDxeSmm, but also SecCore
>(which you added in patch 4/7 of this series).
>
>On the other hand... I can see a preexistent client of
>CpuExceptionHandlerLib in UefiCpuPkg: CpuDxe. (Of type DXE_DRIVER.)
>Since the default Null resolution affects that module too, I'm now
>inclined to think that maybe this is all intentional *within* UefiCpuPkg.
>
>Either way, I'll go ahead and resolve CpuExceptionHandlerLib to
>SmmCpuExceptionHandlerLib, for DXE_SMM_DRIVER modules, in OVMF. That
>should be consistent with the current resolutions we have for other (SEC
>/ PEIM / DXE_DRIVER / ...) modules already.

Yes.  A platform specific DSC must use the right lib mapping based on
module type.  

>
>Thanks!
>Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Laszlo Ersek]

On 10/08/15 23:22, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> A couple responses below.

Thanks much (again!); I think I can rework CpuS3DataDxe based on your
advice:

> 
> Mike
> 
>> -Original Message-
>> From: Laszlo Ersek [mailto:ler...@redhat.com]
>> Sent: Thursday, October 08, 2015 11:37 AM
>> To: Kinney, Michael D
>> Cc: edk2-de...@ml01.01.org
>> Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[snip]

>> Let my try to go through the fields of ACPI_CPU_DATA again, and see how
>> my CpuS3DataDxe could interace with the PiSmmCpuDxeSmm driver in this
>> patch:
>>
>> - APState was one that I removed from both drivers. We've discussed
>>  this above -- even keeping it wouldn't cause CpuS3DataDxe problems.
>>
>> - StartupVector is a strange field. In Quark, CpuMpDxe allocated it and
>>  populated it, and PiSmmCpuDxeSmm overwrote its contents during S3
>>  resume (which was fine), but not with the original contents saved to,
>>  and restored from, SMRAM, but with its *own* code.
>>
>>  IOW, PiSmmCpuDxeSmm only relied, at S3 resume, on the reserved
>>  buffer's *allocation*, which had been made by CpuMpDxe previously
>>  (= during boot). For that reason in CpuS3DataDxe, I didn't bother to
>>  fill in the buffer at all, just to allocate it.
> 
> Correct.  The requirement is to allocate a 4KB aligned buffer that is
> 4KB is size in usable memory below 1MB.
> 
>>
>>  *Plus* I added an ASSERT() to PiSmmCpuDxeSmm (function
>>  PrepareApStartupVector()) about the size of the pre-allocated buffer
>>  (constant 4KB) being big enough for the actual communications area,
>>  and the executable code, that PiSmmCpuDxeSmm would copy into it.
>>
>>  As far as I can see, in the PrepareApStartupVector() function in your
>>  patch 6/7, this silent size assumption (i.e., without an explicit
>>  assert) still exist. Am I correct?
> 
> Yes.  I agree that is a bad assumption.  We either need to state that
> 4KB is always enough, or we need a way for the module that allocates
> the buffer to know the size of the buffer required.  
> 
>>
>>  This is not necessarily a bug, but it should be spelled out as one of
>>  the requirements between CpuS3DataDxe and PiSmmCpuDxeSmm, so that
>> the
>>  former can accommodate the latter -- and the latter should preferably
>>  *enforce* that somehow.
>>
> 
> Agreed

For now I'll just stick with the current allocation in CpuS3DataDxe.

> 
>> - The (a) GdtrProfile, IdtrProfile, ApMachineCheckHandlerBase,
>>  ApMachineCheckHandlerSize; (b) StackAddress and StackSize; and
>>  (c) NumberOfCpus fields were filled in in three more patches. I
>>  assume the CpuS3DataDxe code would work with *this* PiSmmCpuDxeSmm
>>  too.
>>
>> Now, the trouble starts with the following fields:
>>
>> - The MTRR settings (in the MtrrTable field) are saved in a delayed
>>  manner, that is, not when CpuS3DataDxe collects the other data. This
>>  is why I had to write patches for UefiCpuPkg/CpuDxe too, so that it
>>  would reflect the most recent MTRR settings to a spot where
>>  PiSmmCpuDxeSmm could find them at SMM-ready-to-lock. Again I assume
>>  the (minimal) code currently in CpuS3DataDxe would work with *this*
>>  PiSmmCpuDxeSmm, however the functionality ultimately depends on
>>  UefiCpuPkg/CpuDxe changes too.
> 
> You could add an event to CpuS3DataDxe that save MTRR settings
> Into the structure at end of DXE.  On simple platforms, there may
> Not be any different in MTRR settings between the time the CPU driver
> runs and the end of DXE.  In that case, the MTRR settings can be 
> captured when the CPU driver runs and fills in the structure.

Saving MTRR settings and End-of-DXE is a great idea!

OVMF's BDS triggers End-of-DXE not much before it kicks
DxeSmmReadyToLock too (which in turn gets reflected as SmmReadyToLock to
SMM drivers, and then PiSmmCpuDxeSmm fetches and stashes ACPI_CPU_DATA
into SMRAM), and I think it's reasonable *not* to expect MTRR changes
between those two actions, within BDS.

With regard to my other UefiCpuPkg/CpuDxe patches, I think I'll keep
some of them, independently:
- "broadcast MTRR changes to APs"
- "sync MTRR settings to APs at MP startup"
- "provide EFI_MP_SERVICES_PROTOCOL when there's no AP"

because they seem useful. (I'll have to adapt them to your patches of
course.)

>> - PreSmmInitRegisterTable. The handling of this field in Quark's
>>  PiSmmCpuDxeSmm had a security bug (the registers weren't stashed in
>>  SMRAM between boot and S3 resume), but I can see that it is fixed in
>>  your 

Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module

[Kinney, Michael D]

Hi Laszlo,

Thanks for the feedback!

Please let me know if I missed any of your questions.

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>Laszlo Ersek
>Sent: Wednesday, October 07, 2015 10:32 AM
>To: Kinney, Michael D
>Cc: edk2-de...@ml01.01.org
>Subject: Re: [edk2] [PATCH 6/7] UefiCpiPkg: Add PiSmmCpuDxeSmm module
>
>On 10/05/15 01:57, Michael Kinney wrote:
>> Add module that initializes a CPU for the SMM envirnment and
>> installs the first level SMI handler.  This module along with the
>> SMM IPL and SMM Core provide the services required for
>> DXE_SMM_DRIVERS to register hardware and software SMI handlers.
>>
>> CPU specific features are abstracted through the SmmCpuFeaturesLib
>>
>> Platform specific features are abstracted through the
>> SmmCpuPlatformHookLib
>>
>> Several PCDs are added to enable/disable features and configure
>> settings for the PiSmmCpuDxeSmm module
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Michael Kinney <michael.d.kin...@intel.com>
>
>(I'm snipping the patch because it is extremely long.)
>
>I'm going through my "QuarkPort" patches, evaluating for each one if I
>should keep it, drop it, or adapt it.
>
>* [PATCH 27/58] OvmfPkg: import PCDs from
>  Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg
>  http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=350
>
>  Going through all the PCDs I had to import there, and checking if each
>  was present in this patch too, I found the following difference:
>
>  PcdCpuSmmUncacheCpuSyncData is absent from this patch. In my
>  "QuarkPort", the PCD is used by the InitializeMpServiceData() function
>  only, gating a call to SetCacheability(). And that call to
>  SetCacheability() is the *only* such call.
>
>  Now, this patch provides SetCacheability(), but no calls are made to
>  it, ever (in accordance with the fact that the PCD that would control
>  the call is also absent).
>
>  (1) Therefore I recommend to delete the SetCacheability() function
>  from this patch.

I agree.  I will remove that function.  I missed that function when I removed
PcdCpuSmmUncacheCpuSyncData.

>
>  (2) I have a question wrt. PcdCpuSmmEnableBspElection. In the Quark
>  package, the "IA32FamilyCpuBasePkg.dsc" file overrides the default
>  TRUE value (from the .dec) for this PCD, with FALSE.
>
>  The .dec default is similarly TRUE here. In my "QuarkPort" for
>  OVMF, I flipped the PCD to FALSE as well. Does that make sense?
>  Should I stick with that override, when rebasing the OVMF SMM
>  series on top of this series? I don't know why this PCD matters.
>  What are the benefits vs. drawbacks of dynamic BSP-for-SMI
>  election?

For OVMF you will likely not see any difference in behavior.  
This PCD allows a platform to provide PlatformSmmBspElection() 
in a  platform specific SmmCpuPlatformHookLib instance to 
decide which CPU gets elected to be the BSP in each SMI.

The SmmCpuPlatformHookLibNull always returns 
EFI_NOT_READY for that function, which makes the 
module behave the same as the PCD being set to FALSE.

The default is TRUE, so the platform lib is always called,
so a platform developer can implement the hook 
function and does not have to also change a PCD
setting for the hook function to be active.

A platform that wants to eliminate the call to the 
hook function all together can set the PCD to FALSE.

So for OVMF, I think it makes sense to set this PCD to
FALSE in the DSC file.

>
>* [PATCH 28/58] OvmfPkg: import three protocols from
>  Quark_EDKII_v1.1.0/IA32FamilyCpuBasePkg
>  http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=351
>
>  (3) This patch doesn't install gSmmCpuSyncProtocolGuid or
>  gSmmCpuSync2ProtocolGuid in PiCpuSmmEntry(), whereas Quark does.
>
>  I don't know what those protocols good for, but can you please
>  summarize why this patch doesn't need them?

I did not find any consumers of the Sync protocols, so I removed them.

>
>* [PATCH 32/58] OvmfPkg: QuarkPort: drop ACPI_CPU_DATA.APState
>  http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=362
>
>  (4) Patch 5/7 in this series introduces the ACPI_CPU_DATA structure.
>  That structure has a field APState. I think it should be dropped,
>  it is never used in 6/7.

I agree it is not used.  However, this structure is shared between modules
through a buffer address in a PCD, and I am concerned that there may be 
other modules that depend on this layout.  I will need to do more
investigation before I can consider removing it.

>
>* [PATCH 33/58] OvmfPkg: add skeleton QuarkPort/CpuS