Re: [edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase
On 8/21/19 9:25 AM, Laszlo Ersek via Groups.Io wrote: > On 08/19/19 23:35, Lendacky, Thomas wrote: >> From: Tom Lendacky >> >> A GHCB page is needed during the Sec phase, so this new page must be >> created. Since the GHCB must be marked as an un-encrypted, or shared, >> page, an additional pagetable page is required so break down the 2MB >> region where the GHCB page lives into 4K pagetable entries. >> >> Signed-off-by: Tom Lendacky >> --- >> OvmfPkg/OvmfPkg.dec| 5 +++ >> OvmfPkg/OvmfPkgX64.fdf | 11 --- >> OvmfPkg/PlatformPei/PlatformPei.inf| 2 ++ >> OvmfPkg/ResetVector/ResetVector.inf| 2 ++ >> UefiCpuPkg/Include/Register/Amd/Fam17Msr.h | 28 >> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 37 +- >> OvmfPkg/ResetVector/ResetVector.nasmb | 2 +- >> 7 files changed, 81 insertions(+), 6 deletions(-) > > I've skipped patches 02 and 03 for now, because I'll have to go through > them with a fine toothed comb -- in a subsequent submission, most > probably. I'm just trying to provide formal comments, so that I do the > actual review more easily, later. > > As I requested under the blurb, this patch should be split in at least > three parts, if possible -- OvmfPkg/PlatformPei, OvmfPkg/ResetVector, > UefiCpuPkg. (The DEC and FDF changes can be kept squashed with the > OvmfPkg patch that seems more suitable for that.) Ok. > > ... Having said that, why do you add PCDs to the PlatformPei INF file? > The code in PlatformPei doesn't change. Could be a leftover from an > earlier (abandoned) approach. Yeah, most likely. I'll remove that. Thanks, Tom > > Thanks > Laszlo > >> >> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >> index 9640360f6245..2ead9a944af4 100644 >> --- a/OvmfPkg/OvmfPkg.dec >> +++ b/OvmfPkg/OvmfPkg.dec >> @@ -218,6 +218,11 @@ [PcdsFixedAtBuild] >># The value should be a multiple of 4KB. >>gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x40|UINT32|0x31 >> >> + ## Specify the GHCB base address and size. >> + # The value should be a multiple of 4KB for each. >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x32 >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x33 >> + >> [PcdsDynamic, PcdsDynamicEx] >>gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10 >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >> index 74407072563b..2a2427092382 100644 >> --- a/OvmfPkg/OvmfPkgX64.fdf >> +++ b/OvmfPkg/OvmfPkgX64.fdf >> @@ -67,13 +67,16 @@ [FD.MEMFD] >> BlockSize = 0x1 >> NumBlocks = 0xC0 >> >> -0x00|0x006000 >> +0x00|0x007000 >> >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >> >> -0x006000|0x001000 >> -gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize >> - >> 0x007000|0x001000 >> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >> + >> +0x008000|0x001000 >> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize >> + >> +0x009000|0x001000 >> >> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize >> >> 0x01|0x01 >> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf >> b/OvmfPkg/PlatformPei/PlatformPei.inf >> index d9fd9c8f05b3..aed1f64b7c93 100644 >> --- a/OvmfPkg/PlatformPei/PlatformPei.inf >> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf >> @@ -72,6 +72,8 @@ [Pcd] >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize >>gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize >> diff --git a/OvmfPkg/ResetVector/ResetVector.inf >> b/OvmfPkg/ResetVector/ResetVector.inf >> index 960b47cd0797..d66f4dc29737 100644 >> --- a/OvmfPkg/ResetVector/ResetVector.inf >> +++ b/OvmfPkg/ResetVector/ResetVector.inf >> @@ -37,3 +37,5 @@ [Pcd] >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase >>gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >> diff --git a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h >> b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h >> index 37b935dcdb30..55a5723e164e 100644 >> --- a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h >> +++ b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h >> @@ -17,6 +17,34 @@ >>
Re: [edk2-devel] [RFC PATCH 04/28] OvmfPkg: Create a GHCB page for use during Sec phase
On 08/19/19 23:35, Lendacky, Thomas wrote: > From: Tom Lendacky > > A GHCB page is needed during the Sec phase, so this new page must be > created. Since the GHCB must be marked as an un-encrypted, or shared, > page, an additional pagetable page is required so break down the 2MB > region where the GHCB page lives into 4K pagetable entries. > > Signed-off-by: Tom Lendacky > --- > OvmfPkg/OvmfPkg.dec| 5 +++ > OvmfPkg/OvmfPkgX64.fdf | 11 --- > OvmfPkg/PlatformPei/PlatformPei.inf| 2 ++ > OvmfPkg/ResetVector/ResetVector.inf| 2 ++ > UefiCpuPkg/Include/Register/Amd/Fam17Msr.h | 28 > OvmfPkg/ResetVector/Ia32/PageTables64.asm | 37 +- > OvmfPkg/ResetVector/ResetVector.nasmb | 2 +- > 7 files changed, 81 insertions(+), 6 deletions(-) I've skipped patches 02 and 03 for now, because I'll have to go through them with a fine toothed comb -- in a subsequent submission, most probably. I'm just trying to provide formal comments, so that I do the actual review more easily, later. As I requested under the blurb, this patch should be split in at least three parts, if possible -- OvmfPkg/PlatformPei, OvmfPkg/ResetVector, UefiCpuPkg. (The DEC and FDF changes can be kept squashed with the OvmfPkg patch that seems more suitable for that.) ... Having said that, why do you add PCDs to the PlatformPei INF file? The code in PlatformPei doesn't change. Could be a leftover from an earlier (abandoned) approach. Thanks Laszlo > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 9640360f6245..2ead9a944af4 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -218,6 +218,11 @@ [PcdsFixedAtBuild] ># The value should be a multiple of 4KB. >gUefiOvmfPkgTokenSpaceGuid.PcdHighPmmMemorySize|0x40|UINT32|0x31 > > + ## Specify the GHCB base address and size. > + # The value should be a multiple of 4KB for each. > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|0x0|UINT32|0x32 > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize|0x0|UINT32|0x33 > + > [PcdsDynamic, PcdsDynamicEx] >gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10 > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 74407072563b..2a2427092382 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -67,13 +67,16 @@ [FD.MEMFD] > BlockSize = 0x1 > NumBlocks = 0xC0 > > -0x00|0x006000 > +0x00|0x007000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > > -0x006000|0x001000 > -gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > - > 0x007000|0x001000 > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > + > +0x008000|0x001000 > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > + > +0x009000|0x001000 > > gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > > 0x01|0x01 > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf > b/OvmfPkg/PlatformPei/PlatformPei.inf > index d9fd9c8f05b3..aed1f64b7c93 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -72,6 +72,8 @@ [Pcd] >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize >gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > diff --git a/OvmfPkg/ResetVector/ResetVector.inf > b/OvmfPkg/ResetVector/ResetVector.inf > index 960b47cd0797..d66f4dc29737 100644 > --- a/OvmfPkg/ResetVector/ResetVector.inf > +++ b/OvmfPkg/ResetVector/ResetVector.inf > @@ -37,3 +37,5 @@ [Pcd] >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase >gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize > diff --git a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h > b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h > index 37b935dcdb30..55a5723e164e 100644 > --- a/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h > +++ b/UefiCpuPkg/Include/Register/Amd/Fam17Msr.h > @@ -17,6 +17,34 @@ > #ifndef __FAM17_MSR_H__ > #define __FAM17_MSR_H__ > > +/** > + Secure Encrypted Virtualization - Encrypted State (SEV-ES) GHCB register > + > +**/ > +#define MSR_SEV_ES_GHCB0xc0010130 > + > +/** > + MSR information
