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

[Kinney, Michael D]

Hi Laszlo,

A couple responses below.

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
>
>Hi Mike,
>
>On 10/08/15 02:54, Kinney, Michael D wrote:
>> Hi Laszlo,
>>
>> Thanks for the feedback!
>>
>> Please let me know if I missed any of your questions.
>
>I'm very happy with your answers, thank you for them.
>
>I'll comment on S3 below:
>
>> 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 
>>>
>>> (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
>> FAL

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 re

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

[Laszlo Ersek]

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 
> ---

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/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf

OVMF uses the following non-null instances (for the appropriate module
types):

- UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
- 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.inf

and why does it use a NULL instance instead?

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.

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

[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 
>> ---
>
>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

[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

[Jordan Justen]

On 2015-10-12 09:23:28, 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:

Can you submit your patch following?

https://raw.githubusercontent.com/tianocore/edk2/master/MdePkg/Contributions.txt

The key missing part being Contributed-under and Signed-off-by...

So, maybe just reply adding something like this, so Mike can easily
fold it into his patch's commit message:

[your-email: What you changed]
Contributed-under: 
Signed-off-by: 

-Jordan

> 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
___
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 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

[Laszlo Ersek]

On 10/13/15 12:32, Paolo Bonzini wrote:
> 
> 
> 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
> 

For now I'm picking this up as well, as a separate patch, between Mike's
original series and my upcoming v3.

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/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 

(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.

  (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?

* [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?

* [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.

* [PATCH 33/58] OvmfPkg: add skeleton QuarkPort/CpuS3DataDxe
  http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=357

  This patch in my SMM series (and a few later ones that relate to
  CpuS3DataDxe) make me ask my most important question here.

  The PiSmmCpuDxeSmm module in this series -- same as in Quark -- only
  *consumes* PcdCpuS3DataAddress, to save the ACPI_CPU_DATA structure,
  collected by "some driver", into SMRAM.

  In Quark, PiSmmCpuDxeSmm insisted on this, therefore I had to extract
  the "collection" portions of CpuMpDxe, and I named it CpuS3DataDxe.

  However, in this patch, PiSmmCpuDxeSmm seems to be able to handle
  a zero PcdCpuS3DataAddress:

  - in SmmReadyToLockEventNotify(), on normal boot, no collected data is
saved into SMRAM,

  - in SmmRestoreCpu(), at S3 resume, the restoration of said data is
also skipped.

  My questions are:

  (5) Why is this safe? For OVMF's "QuarkPort", identifying and porting
  the required subset of CpuMpDxe (under the name CpuS3DataDxe), in
  order to populate PcdCpuS3DataAddress and ACPI_CPU_DATA, was a
  *major* headache. (Each field in that structure required separate
  analysis.)

  If PcdCpuS3DataAddress doesn't get populated though, then none of
  the ACPI_CPU_DATA fields are restored at S3 resume either, in
  EarlyInitializeCpu() and InitializeCpu().

  (6) When you tested this series on physical hardware, did a different
  driver (not included in this series) exist there in the firmware,
  populating PcdCpuS3DataAddress, and the pointed-to ACPI_CPU_DATA
  structure? Did you test S3?

  - If you think it is safe to leave PcdCpuS3DataAddress and the
pointed-to ACPI_CPU_DATA struct empty (and you tested such a
config), then I'll just drop my CpuS3DataDxe driver. Meaning [PATCH
33/58] referenced above, plus *all* of these patches (note that
there's one UefiCpuPkg/CpuDxe patch among them):

[PATCH 34/58] OvmfPkg: QuarkPort/CpuS3DataDxe: fill in
  ACPI_CPU_DATA.StartupVector
[PATCH 35/58] OvmfPkg: QuarkPor

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 
>
>(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/CpuS3Dat

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

[Laszlo Ersek]

Hi Mike,

On 10/08/15 02:54, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> Thanks for the feedback!
> 
> Please let me know if I missed any of your questions.

I'm very happy with your answers, thank you for them.

I'll comment on S3 below:

> 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 
>>
>> (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: Quark