[v3] Create page table by CpuPageTableLib in DxeIpl

[edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by duntan 1 year ago

In V3 patch set:
1. Add a new patch 'MdePkg: Move CpuPageTableLib defination to MdePkg' to Move CpuPageTableLib defination from UefiCpuPkg to MdePkg.
   So that MdeModulePkg doesn't need to depend on UefiCpuPkg.
2. Modify the patch 'MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib' to set GHCB page to be mapped as unencrypted for each CPU for AMD SEV feature.

Dun Tan (8):
  MdePkg: Move CpuPageTableLib defination to MdePkg
  EmulatorPkg: Add CpuPageTableLib required by DxeIpl in DSC
  IntelFsp2Pkg: Add CpuPageTableLib required by DxeIpl in DSC
  MdeModulePkg: Add CpuPageTableLib required by DxeIpl in DSC
  OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file
  MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib
  MdeModulePkg/DxeIpl: Remove duplicated code to enable NX
  MdeModulePkg/DxeIpl: Refinement to the code to set PageTable as RO

 EmulatorPkg/EmulatorPkg.dsc                              |   3 ++-
 IntelFsp2Pkg/Tools/Tests/QemuFspPkg.dsc                  |   3 ++-
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.h                    |   3 ++-
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                  |   5 ++++-
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c          | 112 ++++------------------------------------------------------------------------------------------------------------
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c           |   5 +++--
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c         | 720 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h         | 182 ++++++++++----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 MdeModulePkg/MdeModulePkg.dsc                            |   3 ++-
 {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h |   0
 MdePkg/MdePkg.dec                                        |   5 ++++-
 OvmfPkg/AmdSev/AmdSevX64.dsc                             |   2 +-
 OvmfPkg/Bhyve/BhyveX64.dsc                               |   3 ++-
 OvmfPkg/CloudHv/CloudHvX64.dsc                           |   2 +-
 OvmfPkg/Microvm/MicrovmX64.dsc                           |   2 +-
 OvmfPkg/OvmfPkgIa32.dsc                                  |   3 ++-
 OvmfPkg/OvmfPkgIa32X64.dsc                               |   2 +-
 OvmfPkg/OvmfPkgX64.dsc                                   |   2 +-
 OvmfPkg/OvmfXen.dsc                                      |   2 +-
 UefiCpuPkg/UefiCpuPkg.dec                                |   3 ---
 20 files changed, 211 insertions(+), 851 deletions(-)
 rename {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h (100%)

-- 
2.31.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103464): https://edk2.groups.io/g/devel/message/103464
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by Ard Biesheuvel 1 year ago

On Mon, 24 Apr 2023 at 12:06, duntan <dun.tan@intel.com> wrote:
>
> In V3 patch set:
> 1. Add a new patch 'MdePkg: Move CpuPageTableLib defination to MdePkg' to Move CpuPageTableLib defination from UefiCpuPkg to MdePkg.
>    So that MdeModulePkg doesn't need to depend on UefiCpuPkg.

As I replied to the other patch, I think adding CpuPageTableLib to
MdePkg in its current form (even only the interface) is not the right
approach here. The function protoypes and enums exposed by this
library are highly specific to a particular implementation.

There is a clear use case here, which is the DXE IPL code, and as has
been suggested in the other thread, it would be more appropriate to
define an abstraction regarding this use case, and define it as a
library class (with a NULL implementation) in MdeModulePkg. That way,
UefiCpuPkg can provide the x86 implementation based on
CpuPageTableLilb, and other architectures can do likewise.

Please refer to the patch below, which illustrates why there are other
requirements in this area:

https://edk2.groups.io/g/devel/message/101122

-- 
Ard.




> 2. Modify the patch 'MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib' to set GHCB page to be mapped as unencrypted for each CPU for AMD SEV feature.
>
> Dun Tan (8):
>   MdePkg: Move CpuPageTableLib defination to MdePkg
>   EmulatorPkg: Add CpuPageTableLib required by DxeIpl in DSC
>   IntelFsp2Pkg: Add CpuPageTableLib required by DxeIpl in DSC
>   MdeModulePkg: Add CpuPageTableLib required by DxeIpl in DSC
>   OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file
>   MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib
>   MdeModulePkg/DxeIpl: Remove duplicated code to enable NX
>   MdeModulePkg/DxeIpl: Refinement to the code to set PageTable as RO
>
>  EmulatorPkg/EmulatorPkg.dsc                              |   3 ++-
>  IntelFsp2Pkg/Tools/Tests/QemuFspPkg.dsc                  |   3 ++-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h                    |   3 ++-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                  |   5 ++++-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c          | 112 ++++------------------------------------------------------------------------------------------------------------
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c           |   5 +++--
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c         | 720 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h         | 182 ++++++++++----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>  MdeModulePkg/MdeModulePkg.dsc                            |   3 ++-
>  {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h |   0
>  MdePkg/MdePkg.dec                                        |   5 ++++-
>  OvmfPkg/AmdSev/AmdSevX64.dsc                             |   2 +-
>  OvmfPkg/Bhyve/BhyveX64.dsc                               |   3 ++-
>  OvmfPkg/CloudHv/CloudHvX64.dsc                           |   2 +-
>  OvmfPkg/Microvm/MicrovmX64.dsc                           |   2 +-
>  OvmfPkg/OvmfPkgIa32.dsc                                  |   3 ++-
>  OvmfPkg/OvmfPkgIa32X64.dsc                               |   2 +-
>  OvmfPkg/OvmfPkgX64.dsc                                   |   2 +-
>  OvmfPkg/OvmfXen.dsc                                      |   2 +-
>  UefiCpuPkg/UefiCpuPkg.dec                                |   3 ---
>  20 files changed, 211 insertions(+), 851 deletions(-)
>  rename {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h (100%)
>
> --
> 2.31.1.windows.1
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103489): https://edk2.groups.io/g/devel/message/103489
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by Michael D Kinney 1 year ago

Hi Ard,

Thanks for the feedback.  Let's work on this approach.

Are there similar needs for CPU related services in the DXE Core before
the CPU AP is loaded?

If we are going to define a new lib class to abstract DXE IPL requirements,
it would be good to cover DXE Core requirements too.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> Sent: Monday, April 24, 2023 10:24 AM
> To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> Subject: Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl
> 
> On Mon, 24 Apr 2023 at 12:06, duntan <dun.tan@intel.com> wrote:
> >
> > In V3 patch set:
> > 1. Add a new patch 'MdePkg: Move CpuPageTableLib defination to MdePkg' to Move CpuPageTableLib defination from UefiCpuPkg to
> MdePkg.
> >    So that MdeModulePkg doesn't need to depend on UefiCpuPkg.
> 
> As I replied to the other patch, I think adding CpuPageTableLib to
> MdePkg in its current form (even only the interface) is not the right
> approach here. The function protoypes and enums exposed by this
> library are highly specific to a particular implementation.
> 
> There is a clear use case here, which is the DXE IPL code, and as has
> been suggested in the other thread, it would be more appropriate to
> define an abstraction regarding this use case, and define it as a
> library class (with a NULL implementation) in MdeModulePkg. That way,
> UefiCpuPkg can provide the x86 implementation based on
> CpuPageTableLilb, and other architectures can do likewise.
> 
> Please refer to the patch below, which illustrates why there are other
> requirements in this area:
> 
> https://edk2.groups.io/g/devel/message/101122
> 
> --
> Ard.
> 
> 
> 
> 
> > 2. Modify the patch 'MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib' to set GHCB page to be mapped as unencrypted
> for each CPU for AMD SEV feature.
> >
> > Dun Tan (8):
> >   MdePkg: Move CpuPageTableLib defination to MdePkg
> >   EmulatorPkg: Add CpuPageTableLib required by DxeIpl in DSC
> >   IntelFsp2Pkg: Add CpuPageTableLib required by DxeIpl in DSC
> >   MdeModulePkg: Add CpuPageTableLib required by DxeIpl in DSC
> >   OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file
> >   MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib
> >   MdeModulePkg/DxeIpl: Remove duplicated code to enable NX
> >   MdeModulePkg/DxeIpl: Refinement to the code to set PageTable as RO
> >
> >  EmulatorPkg/EmulatorPkg.dsc                              |   3 ++-
> >  IntelFsp2Pkg/Tools/Tests/QemuFspPkg.dsc                  |   3 ++-
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h                    |   3 ++-
> >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                  |   5 ++++-
> >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c          | 112 ++++----------------------------------------------------------
> --------------------------------------------------
> >  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c           |   5 +++--
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c         | 720
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------
> --------------------------------------------------------------------------------------------------------------------------------
> --------------------------------------------------------------------------------------------------------------------------------
> --------------------------------------------------------------------------------------------------------------------------------
> --------------------------------------------------------------------------------
> >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h         | 182 ++++++++++----------------------------------------------------
> ------------------------------------------------------------------------------------------------------------------------
> >  MdeModulePkg/MdeModulePkg.dsc                            |   3 ++-
> >  {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h |   0
> >  MdePkg/MdePkg.dec                                        |   5 ++++-
> >  OvmfPkg/AmdSev/AmdSevX64.dsc                             |   2 +-
> >  OvmfPkg/Bhyve/BhyveX64.dsc                               |   3 ++-
> >  OvmfPkg/CloudHv/CloudHvX64.dsc                           |   2 +-
> >  OvmfPkg/Microvm/MicrovmX64.dsc                           |   2 +-
> >  OvmfPkg/OvmfPkgIa32.dsc                                  |   3 ++-
> >  OvmfPkg/OvmfPkgIa32X64.dsc                               |   2 +-
> >  OvmfPkg/OvmfPkgX64.dsc                                   |   2 +-
> >  OvmfPkg/OvmfXen.dsc                                      |   2 +-
> >  UefiCpuPkg/UefiCpuPkg.dec                                |   3 ---
> >  20 files changed, 211 insertions(+), 851 deletions(-)
> >  rename {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h (100%)
> >
> > --
> > 2.31.1.windows.1
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103492): https://edk2.groups.io/g/devel/message/103492
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by Ard Biesheuvel 1 year ago

On Mon, 24 Apr 2023 at 19:51, Kinney, Michael D
<michael.d.kinney@intel.com> wrote:
>
> Hi Ard,
>
> Thanks for the feedback.  Let's work on this approach.
>
> Are there similar needs for CPU related services in the DXE Core before
> the CPU AP is loaded?
>
> If we are going to define a new lib class to abstract DXE IPL requirements,
> it would be good to cover DXE Core requirements too.
>

Yeah, excellent point.

The problem I have had to work around in my strict permissions series
(which includes the linked patch) is that there is a window from the
moment DXE core is dispatched until the moment the CPU arch protocol
DXE driver is dispatched where we don't have an architectural means to
manipulate memory permissions.

So what we'd need here is a library version of the following method

typedef
EFI_STATUS
(EFIAPI *EFI_CPU_SET_MEMORY_ATTRIBUTES)(
  IN EFI_CPU_ARCH_PROTOCOL              *This,
  IN  EFI_PHYSICAL_ADDRESS              BaseAddress,
  IN  UINT64                            Length,
  IN  UINT64                            Attributes
  );

*However*, I am aware that the X86 DXE IPL code deviates from this, as
it needs to build long mode compatible page tables before switching
from IA32 to X64, right?
So whether that use case can be served with this prototype is
doubtful, but I guess it /would/ make sense to define a single library
abstraction that can accommodate all of these use cases.

Happy to discuss this in more detail,



>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard Biesheuvel
> > Sent: Monday, April 24, 2023 10:24 AM
> > To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> > Subject: Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl
> >
> > On Mon, 24 Apr 2023 at 12:06, duntan <dun.tan@intel.com> wrote:
> > >
> > > In V3 patch set:
> > > 1. Add a new patch 'MdePkg: Move CpuPageTableLib defination to MdePkg' to Move CpuPageTableLib defination from UefiCpuPkg to
> > MdePkg.
> > >    So that MdeModulePkg doesn't need to depend on UefiCpuPkg.
> >
> > As I replied to the other patch, I think adding CpuPageTableLib to
> > MdePkg in its current form (even only the interface) is not the right
> > approach here. The function protoypes and enums exposed by this
> > library are highly specific to a particular implementation.
> >
> > There is a clear use case here, which is the DXE IPL code, and as has
> > been suggested in the other thread, it would be more appropriate to
> > define an abstraction regarding this use case, and define it as a
> > library class (with a NULL implementation) in MdeModulePkg. That way,
> > UefiCpuPkg can provide the x86 implementation based on
> > CpuPageTableLilb, and other architectures can do likewise.
> >
> > Please refer to the patch below, which illustrates why there are other
> > requirements in this area:
> >
> > https://edk2.groups.io/g/devel/message/101122
> >
> > --
> > Ard.
> >
> >
> >
> >
> > > 2. Modify the patch 'MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib' to set GHCB page to be mapped as unencrypted
> > for each CPU for AMD SEV feature.
> > >
> > > Dun Tan (8):
> > >   MdePkg: Move CpuPageTableLib defination to MdePkg
> > >   EmulatorPkg: Add CpuPageTableLib required by DxeIpl in DSC
> > >   IntelFsp2Pkg: Add CpuPageTableLib required by DxeIpl in DSC
> > >   MdeModulePkg: Add CpuPageTableLib required by DxeIpl in DSC
> > >   OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file
> > >   MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib
> > >   MdeModulePkg/DxeIpl: Remove duplicated code to enable NX
> > >   MdeModulePkg/DxeIpl: Refinement to the code to set PageTable as RO
> > >
> > >  EmulatorPkg/EmulatorPkg.dsc                              |   3 ++-
> > >  IntelFsp2Pkg/Tools/Tests/QemuFspPkg.dsc                  |   3 ++-
> > >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h                    |   3 ++-
> > >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                  |   5 ++++-
> > >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c          | 112 ++++----------------------------------------------------------
> > --------------------------------------------------
> > >  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c           |   5 +++--
> > >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c         | 720
> > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > ++++++++++++++++++++++++++++++++++++++++----------------------------------------------------------------------------------------
> > --------------------------------------------------------------------------------------------------------------------------------
> > --------------------------------------------------------------------------------------------------------------------------------
> > --------------------------------------------------------------------------------------------------------------------------------
> > --------------------------------------------------------------------------------
> > >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h         | 182 ++++++++++----------------------------------------------------
> > ------------------------------------------------------------------------------------------------------------------------
> > >  MdeModulePkg/MdeModulePkg.dsc                            |   3 ++-
> > >  {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h |   0
> > >  MdePkg/MdePkg.dec                                        |   5 ++++-
> > >  OvmfPkg/AmdSev/AmdSevX64.dsc                             |   2 +-
> > >  OvmfPkg/Bhyve/BhyveX64.dsc                               |   3 ++-
> > >  OvmfPkg/CloudHv/CloudHvX64.dsc                           |   2 +-
> > >  OvmfPkg/Microvm/MicrovmX64.dsc                           |   2 +-
> > >  OvmfPkg/OvmfPkgIa32.dsc                                  |   3 ++-
> > >  OvmfPkg/OvmfPkgIa32X64.dsc                               |   2 +-
> > >  OvmfPkg/OvmfPkgX64.dsc                                   |   2 +-
> > >  OvmfPkg/OvmfXen.dsc                                      |   2 +-
> > >  UefiCpuPkg/UefiCpuPkg.dec                                |   3 ---
> > >  20 files changed, 211 insertions(+), 851 deletions(-)
> > >  rename {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h (100%)
> > >
> > > --
> > > 2.31.1.windows.1
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> > 
> >
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103493): https://edk2.groups.io/g/devel/message/103493
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by Ni, Ray 1 year ago


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> Sent: Tuesday, April 25, 2023 2:07 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> Subject: Re: [edk2-devel] [Patch V3 0/8] Create page table by
> CpuPageTableLib in DxeIpl
> 
> On Mon, 24 Apr 2023 at 19:51, Kinney, Michael D
> <michael.d.kinney@intel.com> wrote:
> >
> > Hi Ard,
> >
> > Thanks for the feedback.  Let's work on this approach.
> >
> > Are there similar needs for CPU related services in the DXE Core before
> > the CPU AP is loaded?
> >
> > If we are going to define a new lib class to abstract DXE IPL requirements,
> > it would be good to cover DXE Core requirements too.
> >
> 
> Yeah, excellent point.
> 
> The problem I have had to work around in my strict permissions series
> (which includes the linked patch) is that there is a window from the
> moment DXE core is dispatched until the moment the CPU arch protocol
> DXE driver is dispatched where we don't have an architectural means to
> manipulate memory permissions.
> 
> So what we'd need here is a library version of the following method
> 
> typedef
> EFI_STATUS
> (EFIAPI *EFI_CPU_SET_MEMORY_ATTRIBUTES)(
>   IN EFI_CPU_ARCH_PROTOCOL              *This,
>   IN  EFI_PHYSICAL_ADDRESS              BaseAddress,
>   IN  UINT64                            Length,
>   IN  UINT64                            Attributes
>   );

What's your idea here?
Besides HandOffToDxeCore(), you require a 2nd lib API as above for
early DXE phase before CPU AP is available?

Why do we want to combine two APIs into one lib class?
If combined, what lib class name do you think is proper to describe the lib purpose?

It seems to me lacking of CPU AP in early DXE phase is acknowledged by PI spec.
Having the 2nd API for DXE early phase is like a workaround to fix PI spec gap, do you think so?

> 
> *However*, I am aware that the X86 DXE IPL code deviates from this, as
> it needs to build long mode compatible page tables before switching
> from IA32 to X64, right?

DXEIPL creates long mode page table with following characteristics:
* 1:1 mapping to cover the entire memory space
* Set the bottom 4K of BSP stack as not-present - prevent stack overflow
* Set the entire BSP stack as NX - prevent buffer overflow attack
* Set the [0-4k] region as not-present - null protection

But it doesn't set DxeCore code region as RO, or data region as NX.

I describe the X86 DXEIPL page table behavior as above. Because I hope
you could explain a bit more on your thoughts. I don't quite understand
your above wordings.

> So whether that use case can be served with this prototype is
> doubtful, but I guess it /would/ make sense to define a single library
> abstraction that can accommodate all of these use cases.
> 
> Happy to discuss this in more detail,
> 
> 
> 
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> Biesheuvel
> > > Sent: Monday, April 24, 2023 10:24 AM
> > > To: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> > > Subject: Re: [edk2-devel] [Patch V3 0/8] Create page table by
> CpuPageTableLib in DxeIpl
> > >
> > > On Mon, 24 Apr 2023 at 12:06, duntan <dun.tan@intel.com> wrote:
> > > >
> > > > In V3 patch set:
> > > > 1. Add a new patch 'MdePkg: Move CpuPageTableLib defination to
> MdePkg' to Move CpuPageTableLib defination from UefiCpuPkg to
> > > MdePkg.
> > > >    So that MdeModulePkg doesn't need to depend on UefiCpuPkg.
> > >
> > > As I replied to the other patch, I think adding CpuPageTableLib to
> > > MdePkg in its current form (even only the interface) is not the right
> > > approach here. The function protoypes and enums exposed by this
> > > library are highly specific to a particular implementation.
> > >
> > > There is a clear use case here, which is the DXE IPL code, and as has
> > > been suggested in the other thread, it would be more appropriate to
> > > define an abstraction regarding this use case, and define it as a
> > > library class (with a NULL implementation) in MdeModulePkg. That way,
> > > UefiCpuPkg can provide the x86 implementation based on
> > > CpuPageTableLilb, and other architectures can do likewise.
> > >
> > > Please refer to the patch below, which illustrates why there are other
> > > requirements in this area:
> > >
> > > https://edk2.groups.io/g/devel/message/101122
> > >
> > > --
> > > Ard.
> > >
> > >
> > >
> > >
> > > > 2. Modify the patch 'MdeModulePkg/DxeIpl: Create page table by
> CpuPageTableLib' to set GHCB page to be mapped as unencrypted
> > > for each CPU for AMD SEV feature.
> > > >
> > > > Dun Tan (8):
> > > >   MdePkg: Move CpuPageTableLib defination to MdePkg
> > > >   EmulatorPkg: Add CpuPageTableLib required by DxeIpl in DSC
> > > >   IntelFsp2Pkg: Add CpuPageTableLib required by DxeIpl in DSC
> > > >   MdeModulePkg: Add CpuPageTableLib required by DxeIpl in DSC
> > > >   OvmfPkg: Add CpuPageTableLib required by DxeIpl in DSC file
> > > >   MdeModulePkg/DxeIpl: Create page table by CpuPageTableLib
> > > >   MdeModulePkg/DxeIpl: Remove duplicated code to enable NX
> > > >   MdeModulePkg/DxeIpl: Refinement to the code to set PageTable as
> RO
> > > >
> > > >  EmulatorPkg/EmulatorPkg.dsc                              |   3 ++-
> > > >  IntelFsp2Pkg/Tools/Tests/QemuFspPkg.dsc                  |   3 ++-
> > > >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.h                    |   3 ++-
> > > >  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf                  |   5 ++++-
> > > >  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c          | 112
> ++++----------------------------------------------------------
> > > --------------------------------------------------
> > > >  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c           |   5 +++--
> > > >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c         | 720
> > >
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++
> > > ++++++++++++++++++++++++++++++++++++++++-------------------------
> ---------------------------------------------------------------
> > > -----------------------------------------------------------------------------------------
> ---------------------------------------
> > > -----------------------------------------------------------------------------------------
> ---------------------------------------
> > > -----------------------------------------------------------------------------------------
> ---------------------------------------
> > > --------------------------------------------------------------------------------
> > > >  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h         | 182
> ++++++++++----------------------------------------------------
> > > -----------------------------------------------------------------------------------------
> -------------------------------
> > > >  MdeModulePkg/MdeModulePkg.dsc                            |   3 ++-
> > > >  {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h |   0
> > > >  MdePkg/MdePkg.dec                                        |   5 ++++-
> > > >  OvmfPkg/AmdSev/AmdSevX64.dsc                             |   2 +-
> > > >  OvmfPkg/Bhyve/BhyveX64.dsc                               |   3 ++-
> > > >  OvmfPkg/CloudHv/CloudHvX64.dsc                           |   2 +-
> > > >  OvmfPkg/Microvm/MicrovmX64.dsc                           |   2 +-
> > > >  OvmfPkg/OvmfPkgIa32.dsc                                  |   3 ++-
> > > >  OvmfPkg/OvmfPkgIa32X64.dsc                               |   2 +-
> > > >  OvmfPkg/OvmfPkgX64.dsc                                   |   2 +-
> > > >  OvmfPkg/OvmfXen.dsc                                      |   2 +-
> > > >  UefiCpuPkg/UefiCpuPkg.dec                                |   3 ---
> > > >  20 files changed, 211 insertions(+), 851 deletions(-)
> > > >  rename {UefiCpuPkg => MdePkg}/Include/Library/CpuPageTableLib.h
> (100%)
> > > >
> > > > --
> > > > 2.31.1.windows.1
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103506): https://edk2.groups.io/g/devel/message/103506
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by Ard Biesheuvel 1 year ago

On Tue, 25 Apr 2023 at 03:45, Ni, Ray <ray.ni@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ard
> > Biesheuvel
> > Sent: Tuesday, April 25, 2023 2:07 AM
> > To: Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: devel@edk2.groups.io; Tan, Dun <dun.tan@intel.com>
> > Subject: Re: [edk2-devel] [Patch V3 0/8] Create page table by
> > CpuPageTableLib in DxeIpl
> >
> > On Mon, 24 Apr 2023 at 19:51, Kinney, Michael D
> > <michael.d.kinney@intel.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > Thanks for the feedback.  Let's work on this approach.
> > >
> > > Are there similar needs for CPU related services in the DXE Core before
> > > the CPU AP is loaded?
> > >
> > > If we are going to define a new lib class to abstract DXE IPL requirements,
> > > it would be good to cover DXE Core requirements too.
> > >
> >
> > Yeah, excellent point.
> >
> > The problem I have had to work around in my strict permissions series
> > (which includes the linked patch) is that there is a window from the
> > moment DXE core is dispatched until the moment the CPU arch protocol
> > DXE driver is dispatched where we don't have an architectural means to
> > manipulate memory permissions.
> >
> > So what we'd need here is a library version of the following method
> >
> > typedef
> > EFI_STATUS
> > (EFIAPI *EFI_CPU_SET_MEMORY_ATTRIBUTES)(
> >   IN EFI_CPU_ARCH_PROTOCOL              *This,
> >   IN  EFI_PHYSICAL_ADDRESS              BaseAddress,
> >   IN  UINT64                            Length,
> >   IN  UINT64                            Attributes
> >   );
>
> What's your idea here?
> Besides HandOffToDxeCore(), you require a 2nd lib API as above for
> early DXE phase before CPU AP is available?
>
> Why do we want to combine two APIs into one lib class?
> If combined, what lib class name do you think is proper to describe the lib purpose?
>
> It seems to me lacking of CPU AP in early DXE phase is acknowledged by PI spec.
> Having the 2nd API for DXE early phase is like a workaround to fix PI spec gap, do you think so?
>

Perhaps. Maybe the problem here is that there setting memory
permissions is not part of the PEI CPU arch protocol. It would make
sense for shadowed PEIMs as well as the DXE core to be mapped with
strict permissions at dispatch time (if the section alignment permits
it). For XIP PEIMs, nothing would change, and if PEI executes in place
from DRAM, the whole FV can be mapped read-only.

Or perhaps this should be a separate PPI altogether, and we could
define it as one that is callable from DXE core if the CPU arch
protocol has not been dispatched yet.

I don't really care whether or not we add this to the PI spec tbh

> >
> > *However*, I am aware that the X86 DXE IPL code deviates from this, as
> > it needs to build long mode compatible page tables before switching
> > from IA32 to X64, right?
>
> DXEIPL creates long mode page table with following characteristics:
> * 1:1 mapping to cover the entire memory space
> * Set the bottom 4K of BSP stack as not-present - prevent stack overflow
> * Set the entire BSP stack as NX - prevent buffer overflow attack
> * Set the [0-4k] region as not-present - null protection
>
> But it doesn't set DxeCore code region as RO, or data region as NX.
>
> I describe the X86 DXEIPL page table behavior as above. Because I hope
> you could explain a bit more on your thoughts. I don't quite understand
> your above wordings.
>

I guess the long mode switch is sufficiently special that it will be
very hard to define a sane API that covers all of this. OTOH, it seems
like a missed opportunity to rely on DXE IPL to create all these
restricted mappings, and invent something completely new just to remap
the DXE core text and data sections RO / XP. And note that, for arm64,
this should occur before the code is actually called, since the
restricted mode we would like to enable for EDK2 does not permit
memory that is both writable and executable at all.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103586): https://edk2.groups.io/g/devel/message/103586
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [Patch V3 0/8] Create page table by CpuPageTableLib in DxeIpl

Posted by Ni, Ray 1 year ago

I can think of 3 options:
1. Create MdeModulePkg/HandOffToDxeCoreLib lib class. UefiCpuPkg implements the two instances supporting 32/64bit PEI.
2. Create MdeModulePkg/Ppi/EdkiiMemoryAttribute.h. UefiCpuPkg/CpuMpPei implements the X86 version of MemoryAttribute PPI.
     (As what Ard did in ArmCpuDxe driver.)
     The MemoryAttribute PPI only supports to modify the memory attribute in the active page table. It cannot modify the "future" page table
     which is the case DxeIpl/Ia32 creates long-mode page table.
      So, this option cannot help on DxeIpl/Ia32. I will have to keep DxeIpl/Ia32 code unchanged and only cleanup the DxeIpl/X64 by calling the new PPI.
3. A slight different version of option #2: create MdeModulePkg/MemoryAttributeLib lib class instead of PPI. I would prefer #2.


Option #1 helps to create single DxeIpl PEIM but doesn't help to abstract memory attributes changing in whole PEI phase.
With Option #2, multiple DxeIpl PEIMs still exist for different archs.

I am fine with either option #1 or #2 because either can avoid MdeModulePkg depending on UefiCpuPkg.

Thoughts?

> > >
> > > The problem I have had to work around in my strict permissions series
> > > (which includes the linked patch) is that there is a window from the
> > > moment DXE core is dispatched until the moment the CPU arch protocol
> > > DXE driver is dispatched where we don't have an architectural means to
> > > manipulate memory permissions.
> > >
> > > So what we'd need here is a library version of the following method
> > >
> > > typedef
> > > EFI_STATUS
> > > (EFIAPI *EFI_CPU_SET_MEMORY_ATTRIBUTES)(
> > >   IN EFI_CPU_ARCH_PROTOCOL              *This,
> > >   IN  EFI_PHYSICAL_ADDRESS              BaseAddress,
> > >   IN  UINT64                            Length,
> > >   IN  UINT64                            Attributes
> > >   );
> >
> > What's your idea here?
> > Besides HandOffToDxeCore(), you require a 2nd lib API as above for
> > early DXE phase before CPU AP is available?
> >
> > Why do we want to combine two APIs into one lib class?
> > If combined, what lib class name do you think is proper to describe the lib
> purpose?
> >
> > It seems to me lacking of CPU AP in early DXE phase is acknowledged by PI
> spec.
> > Having the 2nd API for DXE early phase is like a workaround to fix PI spec
> gap, do you think so?
> >
> 
> Perhaps. Maybe the problem here is that there setting memory
> permissions is not part of the PEI CPU arch protocol. It would make
> sense for shadowed PEIMs as well as the DXE core to be mapped with
> strict permissions at dispatch time (if the section alignment permits
> it). For XIP PEIMs, nothing would change, and if PEI executes in place
> from DRAM, the whole FV can be mapped read-only.
> 
> Or perhaps this should be a separate PPI altogether, and we could
> define it as one that is callable from DXE core if the CPU arch
> protocol has not been dispatched yet.
> 
> I don't really care whether or not we add this to the PI spec tbh
> 
> > >
> > > *However*, I am aware that the X86 DXE IPL code deviates from this, as
> > > it needs to build long mode compatible page tables before switching
> > > from IA32 to X64, right?
> >
> > DXEIPL creates long mode page table with following characteristics:
> > * 1:1 mapping to cover the entire memory space
> > * Set the bottom 4K of BSP stack as not-present - prevent stack overflow
> > * Set the entire BSP stack as NX - prevent buffer overflow attack
> > * Set the [0-4k] region as not-present - null protection
> >
> > But it doesn't set DxeCore code region as RO, or data region as NX.
> >
> > I describe the X86 DXEIPL page table behavior as above. Because I hope
> > you could explain a bit more on your thoughts. I don't quite understand
> > your above wordings.
> >
> 
> I guess the long mode switch is sufficiently special that it will be
> very hard to define a sane API that covers all of this. OTOH, it seems
> like a missed opportunity to rely on DXE IPL to create all these
> restricted mappings, and invent something completely new just to remap
> the DXE core text and data sections RO / XP. And note that, for arm64,
> this should occur before the code is actually called, since the
> restricted mode we would like to enable for EDK2 does not permit
> memory that is both writable and executable at all.
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#103610): https://edk2.groups.io/g/devel/message/103610
Mute This Topic: https://groups.io/mt/98466782/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-