Re: [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

["Yao, Jiewen" <jiewen.yao@intel.com>]

Comments below:

From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Duran, Leo
Sent: Wednesday, February 8, 2017 10:31 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; Gao, Liming <liming.gao@intel.com>; edk2-devel@ml01.01.org
Cc: Laszlo Ersek <lersek@redhat.com>; Tian, Feng <feng.tian@intel.com>; Singh, Brijesh <brijesh.singh@amd.com>; Zeng, Star <star.zeng@intel.com>
Subject: Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

[Jiewen] The IA32 capsule code creates X64 page tables, then switch to X64.
So the page table is for X64. Would you please double check if this PCD is needed?

Regarding: MedModelePkg/Universal/CapsulePei/UefiCapsule.c
Create4GPageTables() explicitly sets PhysicalAddressBits = 32;

So it seems like the address space is restricted to 4GB's even after switching to LongMode.
[Jiewen] We use page fault to handler above 4GiB access. :)

However, to your point, SEV just requires LongMode... so I'll make the change.
[Jiewen] Thank you.

Leo.


From: Yao, Jiewen [mailto:jiewen.yao@intel.com]
Sent: Wednesday, February 08, 2017 11:30 AM
To: Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>
Cc: Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com>>; Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com>>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
Subject: RE: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

Comments below:


From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Duran, Leo
Sent: Wednesday, February 8, 2017 9:12 AM
To: Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com<mailto:liming.gao@intel.com%3cmailto:liming.gao@intel.com>>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>>
Cc: Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>; Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com<mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com>>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com<mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>
Subject: Re: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD PcdPteMemoryEncryptionAddressOrMask

Please see replies below.
Thanks,
Leo

> -----Original Message-----
> From: Gao, Liming [mailto:liming.gao@intel.com]
> Sent: Wednesday, February 08, 2017 9:19 AM
> To: Duran, Leo <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>; Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com<mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com>>>;
> Singh, Brijesh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com<mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>
> Subject: RE: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> PcdPteMemoryEncryptionAddressOrMask
>
> Leo:
>   MdeModulePkg CapsulePei and UefiCpuPkg S3Resume2 also create
> PageTable to run X64 code. Do they require this change?
>
> Thanks
> Liming
[Duran, Leo]
1) MedModelePkg/Universal/CapsulePei:
Does not seem applicable for MDE_XPU_X64 compile-time option, which is required for SEV.
- ModeSwitch() calls Thunk32To64(), which in turn may call Create4GPageTables()
- However, ModeSwitch() is called only under #ifdef MDE_CPU_IA32
[Jiewen] The IA32 capsule code creates X64 page tables, then switch to X64.
So the page table is for X64. Would you please double check if this PCD is needed?


2)  UefiCpuPkg/Universal/Acpi/S3Resume2Pei:
Agreed. Will incorporate changes in 'v2' of the patch.

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Leo Duran
> Sent: Wednesday, February 8, 2017 3:54 AM
> To: edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org%3cmailto:edk2-devel@ml01.01.org>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>; Tian, Feng <feng.tian@intel.com<mailto:feng.tian@intel.com<mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com>>>;
> Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>; Zeng, Star <star.zeng@intel.com<mailto:star.zeng@intel.com<mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>;
> Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>
> Subject: [edk2] [PATCH] MdeModulePkg: Add dynamic PCD
> PcdPteMemoryEncryptionAddressOrMask
>
> From: Brijesh Singh <brijesh.singh@amd.com<mailto:brijesh.singh@amd.com<mailto:brijesh.singh@amd.com%3cmailto:brijesh.singh@amd.com>>>
>
> This dynamic PCD holds the address mask for page table entries when
> memory encryption is enabled on AMD processors supporting the Secure
> Encrypted Virtualization (SEV) feature.
>
> Cc: Feng Tian <feng.tian@intel.com<mailto:feng.tian@intel.com<mailto:feng.tian@intel.com%3cmailto:feng.tian@intel.com>>>
> Cc: Star Zeng <star.zeng@intel.com<mailto:star.zeng@intel.com<mailto:star.zeng@intel.com%3cmailto:star.zeng@intel.com>>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com<mailto:lersek@redhat.com%3cmailto:lersek@redhat.com>>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Leo Duran <leo.duran@amd.com<mailto:leo.duran@amd.com<mailto:leo.duran@amd.com%3cmailto:leo.duran@amd.com>>>
> ---
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  5 ++++-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 18 ++++++++++--
> ------
>  MdeModulePkg/MdeModulePkg.dec                    |  8 ++++++++
>  3 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> index 2bc41be..d62bd9b 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
> @@ -6,6 +6,8 @@
>  #  needed to run the DXE Foundation.
>  #
>  #  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
>  #  This program and the accompanying materials  #  are licensed and made
> available under the terms and conditions of the BSD License  #  which
> accompanies this distribution.  The full text of the license may be found at
> @@ -111,7 +113,8 @@ [FeaturePcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSupportUefiDecompress ##
> CONSUMES
>
>  [Pcd.IA32,Pcd.X64]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable              ##
> SOMETIMES_CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable                      ##
> SOMETIMES_CONSUMES
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask    ## CONSUMES
>
>  [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ##
> SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> index 790f6ab..2c52389 100644
> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
> @@ -16,6 +16,8 @@
>      3) IA-32 Intel(R) Architecture Software Developer's Manual Volume
> 3:System Programmer's Guide, Intel
>
>  Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> +
>  This program and the accompanying materials  are licensed and made
> available under the terms and conditions of the BSD License  which
> accompanies this distribution.  The full text of the license may be found at
> @@ -71,14 +73,14 @@ Split2MPageTo4K (
>    //
>    // Fill in 2M page entry.
>    //
> -  *PageEntry2M = (UINT64) (UINTN) PageTableEntry | IA32_PG_P |
> IA32_PG_RW;
> +  *PageEntry2M = (UINT64) (UINTN) PageTableEntry | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask) | IA32_PG_P | IA32_PG_RW;
>
>    PhysicalAddress4K = PhysicalAddress;
>    for (IndexOfPageTableEntries = 0; IndexOfPageTableEntries < 512;
> IndexOfPageTableEntries++, PageTableEntry++, PhysicalAddress4K +=
> SIZE_4KB) {
>      //
>      // Fill in the Page Table entries
>      //
> -    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K;
> +    PageTableEntry->Uint64 = (UINT64) PhysicalAddress4K | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask);
>      PageTableEntry->Bits.ReadWrite = 1;
>      PageTableEntry->Bits.Present = 1;
>      if ((PhysicalAddress4K >= StackBase) && (PhysicalAddress4K < StackBase +
> StackSize)) { @@ -116,7 +118,7 @@ Split1GPageTo2M (
>    //
>    // Fill in 1G page entry.
>    //
> -  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | IA32_PG_P |
> IA32_PG_RW;
> +  *PageEntry1G = (UINT64) (UINTN) PageDirectoryEntry | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask) | IA32_PG_P | IA32_PG_RW;
>
>    PhysicalAddress2M = PhysicalAddress;
>    for (IndexOfPageDirectoryEntries = 0; IndexOfPageDirectoryEntries < 512;
> IndexOfPageDirectoryEntries++, PageDirectoryEntry++, PhysicalAddress2M
> += SIZE_2MB) { @@ -129,7 +131,7 @@ Split1GPageTo2M (
>        //
>        // Fill in the Page Directory entries
>        //
> -      PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M;
> +      PageDirectoryEntry->Uint64 = (UINT64) PhysicalAddress2M |
> + PcdGet64 (PcdPteMemoryEncryptionAddressOrMask);
>        PageDirectoryEntry->Bits.ReadWrite = 1;
>        PageDirectoryEntry->Bits.Present = 1;
>        PageDirectoryEntry->Bits.MustBe1 = 1; @@ -248,7 +250,7 @@
> CreateIdentityMappingPageTables (
>      //
>      // Make a PML4 Entry
>      //
> -    PageMapLevel4Entry->Uint64 =
> (UINT64)(UINTN)PageDirectoryPointerEntry;
> +    PageMapLevel4Entry->Uint64 =
> + (UINT64)(UINTN)PageDirectoryPointerEntry | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask);
>      PageMapLevel4Entry->Bits.ReadWrite = 1;
>      PageMapLevel4Entry->Bits.Present = 1;
>
> @@ -262,7 +264,7 @@ CreateIdentityMappingPageTables (
>            //
>            // Fill in the Page Directory entries
>            //
> -          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress;
> +          PageDirectory1GEntry->Uint64 = (UINT64)PageAddress | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask);
>            PageDirectory1GEntry->Bits.ReadWrite = 1;
>            PageDirectory1GEntry->Bits.Present = 1;
>            PageDirectory1GEntry->Bits.MustBe1 = 1; @@ -280,7 +282,7 @@
> CreateIdentityMappingPageTables (
>          //
>          // Fill in a Page Directory Pointer Entries
>          //
> -        PageDirectoryPointerEntry->Uint64 =
> (UINT64)(UINTN)PageDirectoryEntry;
> +        PageDirectoryPointerEntry->Uint64 =
> + (UINT64)(UINTN)PageDirectoryEntry | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask);
>          PageDirectoryPointerEntry->Bits.ReadWrite = 1;
>          PageDirectoryPointerEntry->Bits.Present = 1;
>
> @@ -294,7 +296,7 @@ CreateIdentityMappingPageTables (
>              //
>              // Fill in the Page Directory entries
>              //
> -            PageDirectoryEntry->Uint64 = (UINT64)PageAddress;
> +            PageDirectoryEntry->Uint64 = (UINT64)PageAddress | PcdGet64
> + (PcdPteMemoryEncryptionAddressOrMask);
>              PageDirectoryEntry->Bits.ReadWrite = 1;
>              PageDirectoryEntry->Bits.Present = 1;
>              PageDirectoryEntry->Bits.MustBe1 = 1; diff --git
> a/MdeModulePkg/MdeModulePkg.dec
> b/MdeModulePkg/MdeModulePkg.dec index 273cd7e..207384f 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -6,6 +6,8 @@
>  # Copyright (c) 2007 - 2017, Intel Corporation. All rights reserved.<BR>  #
> Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>  # (C) Copyright 2016
> Hewlett Packard Enterprise Development LP<BR>
> +# Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> #
>  # This program and the accompanying materials are licensed and made
> available under  # the terms and conditions of the BSD License that
> accompanies this distribution.
>  # The full text of the license may be found at @@ -1738,5 +1740,11 @@
> [PcdsDynamic, PcdsDynamicEx]
>    # @Prompt If there is any test key used by the platform.
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdTestKeyUsed|FALSE|BOOLEAN|0x0
> 0030003
>
> +  ## This dynamic PCD holds the address mask for page table entries
> + when memory encryption is  #  enabled on AMD processors supporting the
> Secure Encrypted Virtualization (SEV) feature.
> +  #  This mask should be applied when creating 1:1 virtual to physical
> mapping tables.
> +  #
> +
> +
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrM
> ask|0x0
> + |UINT64|0x00030004
> +
>  [UserExtensions.TianoCore."ExtraFiles"]
>    MdeModulePkgExtra.uni
> --
> 1.9.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>
https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
https://lists.01.org/mailman/listinfo/edk2-devel