[edk2-devel] [PATCH 2/3] OvmfPkg/ResetVector: update SEV support to use new work area format
Brijesh Singh via groups.io
brijesh.singh=amd.com at groups.io
Mon Aug 9 17:21:20 UTC 2021
On 8/9/21 11:54 AM, Tom Lendacky wrote:
> On 8/4/21 3:20 PM, Brijesh Singh wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3429
>>
>> Update the SEV support to switch to using the newer work area format.
>>
>> Cc: James Bottomley <jejb at linux.ibm.com>
>> Cc: Min Xu <min.m.xu at intel.com>
>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>> Cc: Erdem Aktas <erdemaktas at google.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh at amd.com>
>> ---
>> OvmfPkg/ResetVector/ResetVector.inf | 1 +
>> OvmfPkg/Sec/SecMain.inf | 1 +
>> OvmfPkg/Sec/SecMain.c | 25 ++++++++++++++++++++++-
>> OvmfPkg/ResetVector/Ia32/AmdSev.asm | 8 ++++++++
>> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 4 ++++
>> OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
>> 6 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>> index d028c92d8cfa..6ec9cca40c3a 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>> @@ -34,6 +34,7 @@ [BuildOptions]
>> *_*_X64_NASMB_FLAGS = -I$(WORKSPACE)/UefiCpuPkg/ResetVector/Vtf0/
>>
>> [Pcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
>
> Laszlo was trying to keep things sorted, so you should move this down to
> the end of the list.
Yes, I will try to keep it sorted.
>
>> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
>> index 7f78dcee2772..82910dcbd5c2 100644
>> --- a/OvmfPkg/Sec/SecMain.inf
>> +++ b/OvmfPkg/Sec/SecMain.inf
>> @@ -56,6 +56,7 @@ [Ppis]
>> gEfiTemporaryRamSupportPpiGuid # PPI ALWAYS_PRODUCED
>>
>> [Pcd]
>> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase
>
> Ditto here, even though the list isn't truly sorted to begin with.
Noted.
>
>> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
>> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
>> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
>> index 9db67e17b2aa..dda572c7ad7d 100644
>> --- a/OvmfPkg/Sec/SecMain.c
>> +++ b/OvmfPkg/Sec/SecMain.c
>> @@ -807,6 +807,29 @@ SevEsProtocolCheck (
>> Ghcb->GhcbUsage = GHCB_STANDARD_USAGE;
>> }
>>
>> +/**
>> + Determine if the SEV is active.
>> +
>> + During the early booting, GuestType is set in the work area. Verify that it
>> + is an SEV guest.
>> +
>> + @retval TRUE SEV is enabled
>> + @retval FALSE SEV is not enabled
>> +
>> +**/
>> +STATIC
>> +BOOLEAN
>> +IsSevGuest (
>> + VOID
>> + )
>> +{
>> + OVMF_WORK_AREA *WorkArea;
>> +
>> + WorkArea = (OVMF_WORK_AREA *) FixedPcdGet32 (PcdOvmfWorkAreaBase);
>> +
>> + return ((WorkArea != NULL) && (WorkArea->GuestType == GUEST_TYPE_AMD_SEV));
>> +}
>> +
>> /**
>> Determine if SEV-ES is active.
>>
>> @@ -828,7 +851,7 @@ SevEsIsEnabled (
>>
>> SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
>>
>> - return ((SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
>> + return (((IsSevGuest()) && SevEsWorkArea != NULL) && (SevEsWorkArea->SevEsEnabled != 0));
>
> The IsSevGuest() function checks for a NULL work area, so there's no need
> to check for SevEsWorkArea being non-NULL now. I think it would read
> better, though, to do:
>
> if (!IsSevGuest ()) {
> return FALSE;
> }
>
> SevEsWorkArea = ...
>
> return (SevEsWorkArea->SevEsEnabled != 0);
>
>> }
>>
Sure, it makes it a bit more readiable and avoids unessary checks.
>> VOID
>> diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> index aa95d06eaddb..87d81b01e263 100644
>> --- a/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
>> @@ -171,6 +171,9 @@ CheckSevFeatures:
>> bt eax, 0
>> jnc NoSev
>>
>> + ; Set the work area header to indicate that the SEV is enabled
>
> s/the SEV/SEV/
Noted.
>
>> + mov byte[WORK_AREA_GUEST_TYPE], 1
>
> The "1" should probably be defined in ResetVector.nasmb as a %define.
Sure, I will define the constant
>
>> +
>> ; Check for SEV-ES memory encryption feature:
>> ; CPUID Fn8000_001F[EAX] - Bit 3
>> ; CPUID raises a #VC exception if running as an SEV-ES guest
>> @@ -257,6 +260,11 @@ SevExit:
>> IsSevEsEnabled:
>> xor eax, eax
>>
>> + ; During CheckSevFeatures, the WORK_AREA_GUEST_TYPE is set
>> + ; to 1 if SEV is enabled.
>> + cmp byte[WORK_AREA_GUEST_TYPE], 1
>> + jne SevEsDisabled
>> +
>> ; During CheckSevFeatures, the SEV_ES_WORK_AREA was set to 1 if
>> ; SEV-ES is enabled.
>> cmp byte[SEV_ES_WORK_AREA], 1
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index eacdb69ddb9f..f688909f1c7d 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -42,6 +42,10 @@ BITS 32
>> ;
>> SetCr3ForPageTables64:
>>
>> + ; Clear the WorkArea header. The SEV probe routines will populate the
>
> How about:
> ; Initialize the WorkArea header to indicate a legacy guest. The ...
>
>> + ; work area when detected.
>> + mov byte[WORK_AREA_GUEST_TYPE], 0
>
> And then use a %define here for the '0'
>
I will make the required changes.
>> +
>> OneTimeCall CheckSevFeatures
>> xor edx, edx
>> test eax, eax
>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>> index acec46a32450..d1d800c56745 100644
>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>> @@ -72,6 +72,7 @@
>> %define GHCB_PT_ADDR (FixedPcdGet32 (PcdOvmfSecGhcbPageTableBase))
>> %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>> %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>> + %define WORK_AREA_GUEST_TYPE (FixedPcdGet32 (PcdOvmfWorkAreaBase))
>
> Create %defines for each of the defined enum types from the first patch.
>
Got it. thanks
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78982): https://edk2.groups.io/g/devel/message/78982
Mute This Topic: https://groups.io/mt/84670985/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list