subject:"\[edk2\-devel\] \[PATCH RFC v3 03\/22\] OvmfPkg\/MemEncryptSevLib\: extend the workarea to include SNP enabled field"

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-06-08 Thread Laszlo Ersek

On 06/08/21 15:51, Brijesh Singh wrote:
> 
> On 6/8/21 3:17 AM, Laszlo Ersek wrote:
>>
 (3) Actually, no.

 This patch should be reduced to the following files only:

 - OvmfPkg/PlatformPei/AmdSev.c
 - OvmfPkg/PlatformPei/PlatformPei.inf

 and the following changes should be dropped completely:

 - OvmfPkg/Include/Library/MemEncryptSevLib.h
 - OvmfPkg/ResetVector/Ia32/PageTables64.asm
 - OvmfPkg/ResetVector/ResetVector.nasmb

 Specifically, the "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" field should
 never be introduced.

 The reason is apparent only from patch #10 -- "OvmfPkg/PlatformPei:
 register GHCB gpa for the SEV-SNP guest".

 The core idea is that in patch#10, in the SEC module, you can implement
 SevSnpIsEnabled() by just reading MSR_SEV_STATUS, and checking the SNP
 bit. Namely, while the SevSnpIsEnabled() call is made in
 SevEsProtocolCheck(), i.e., before exception handling is set up in the
 SEC module -- and so you indeed cannot call CPUID --, you don't *have*
 to call CPUID at that call site. Where you call SevSnpIsEnabled() in
 SevEsProtocolCheck(), you already know that SEV-ES is enabled, so it's
 safe to just read the exact same SEV status MSR that the SEV-ES status
 comes from in the first place, without any CPUID safety check.
>>> We must check the SNP Enabled inside the assembly code for the page
>>> invalidate functions, and I decided to cache the value. A similar
>>> SNP-enabled check is required in SEC phase before the
>>> ProcessLibraryConstrctorList() is called. There are two options on how
>>> we can go about doing the SNP enabled check inside the SEC phase
>>> 1. Call the SEV_STATUS MSR after reading the
>>> SEC_SEV_ES_WORK_AREA.SevEnabled. As you said, we need to be sure that ES
>>> is enabled before calling the SEV_STATUS MSR.
>>> 2. SEV_STATUS MSR is read in Reset vector for the SNP enabled check
>>> purpose. Extend the SevEsWorkArea to cache the state.
>>>
>>>  I chose #2 because it avoids checking for ES enabled before checking
>>> the SNP enabled. I understand that in the current code path, SNP check
>>> is called inside the SevEsProtocolCheck() -- ES is already enabled, and
>>> its safe to call SEV_STATUS MSR. What if we need to check for the SNP
>>> state outside the ES-specific code block in the future? Then we will
>>> need to extend the SevEsWorkArea.
>> What would be the reason for this, ever?
> 
> One reason I can think of is if we ever decided validate the pages
> before the SevEsProtocolCheck(). The version 2 of GHCB spec adds few new
> NAE's that are SNP specific such as Page State Change. They are not
> applicable to the ES guests. Currently, we do the page validation much
> later and by then ProcessorConstructList() is called. Anyway, this is
> not an important thing to consider right now. As I said, I will drop the
> extending workarea to cache the SNP enable and Hypervisor feature values.

Thanks.

I agree there are conflicting goals here (and by that I don't mean my
goals conflict with yours). One goal is to avoid speculative generality,
as (in my experience) actual usage of such generality rarely
materializes, but we keep paying the price of a more complex data flow
in maintenance. The conflicting goal is to lay a future-proof foundation
(also known as "let's not code ourselves into a corner").

Personally, I'm really bad at predicting the future, while (I feel)
more-complex-than-necessary data flow tends to sound my alarm quickly.
(Perhaps because that can make debugging difficult.) I know I'm going
out on a limb here and that I might eat my words later, but right now, I
think we should not extend the work area.

Thanks for putting up with me.


> 
> 
>>
>> I think this ties in with another point (or question) I raised
>> elsewhere: the assembly code in the reset vector suggests *anyway* that
>> SNP is only available if ES is available, but I couldn't verify that
>> from any specs. If this dependency is an architectural fact (that is, if
>> ES is absent, then SNP may never be present), then I wouldn't like to
>> introduce a separate field for SNP presence in the SEC_SEV_ES_WORK_AREA
>> structure.
> 
> The SEV-SNP builds upon existing SEV and SEV-ES support and provides an
> additional protection from the hypervisor. The SEV-SNP feature requires
> both the SEV and SEV-ES must be enabled. There is some text about it in
> APM volume 2 [1] chapter  15.36.
> 
> 
> [1] https://www.amd.com/system/files/TechDocs/24593.pdf

Perfect, the first paragraph is exactly what I needed:

The SEV-SNP features enable additional protection for encrypted VMs
designed to achieve stronger isolation from the hypervisor. SEV-SNP
is used with the SEV and SEV-ES features described in Section 15.34
and Section 15.35 respectively and requires the enablement and use
of these features.

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-06-08 Thread Brijesh Singh via groups.io



On 6/8/21 3:17 AM, Laszlo Ersek wrote:
>
>>> (3) Actually, no.
>>>
>>> This patch should be reduced to the following files only:
>>>
>>> - OvmfPkg/PlatformPei/AmdSev.c
>>> - OvmfPkg/PlatformPei/PlatformPei.inf
>>>
>>> and the following changes should be dropped completely:
>>>
>>> - OvmfPkg/Include/Library/MemEncryptSevLib.h
>>> - OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> - OvmfPkg/ResetVector/ResetVector.nasmb
>>>
>>> Specifically, the "SEC_SEV_ES_WORK_AREA.SevSnpEnabled" field should
>>> never be introduced.
>>>
>>> The reason is apparent only from patch #10 -- "OvmfPkg/PlatformPei:
>>> register GHCB gpa for the SEV-SNP guest".
>>>
>>> The core idea is that in patch#10, in the SEC module, you can implement
>>> SevSnpIsEnabled() by just reading MSR_SEV_STATUS, and checking the SNP
>>> bit. Namely, while the SevSnpIsEnabled() call is made in
>>> SevEsProtocolCheck(), i.e., before exception handling is set up in the
>>> SEC module -- and so you indeed cannot call CPUID --, you don't *have*
>>> to call CPUID at that call site. Where you call SevSnpIsEnabled() in
>>> SevEsProtocolCheck(), you already know that SEV-ES is enabled, so it's
>>> safe to just read the exact same SEV status MSR that the SEV-ES status
>>> comes from in the first place, without any CPUID safety check.
>> We must check the SNP Enabled inside the assembly code for the page
>> invalidate functions, and I decided to cache the value. A similar
>> SNP-enabled check is required in SEC phase before the
>> ProcessLibraryConstrctorList() is called. There are two options on how
>> we can go about doing the SNP enabled check inside the SEC phase
>> 1. Call the SEV_STATUS MSR after reading the
>> SEC_SEV_ES_WORK_AREA.SevEnabled. As you said, we need to be sure that ES
>> is enabled before calling the SEV_STATUS MSR.
>> 2. SEV_STATUS MSR is read in Reset vector for the SNP enabled check
>> purpose. Extend the SevEsWorkArea to cache the state.
>>
>>  I chose #2 because it avoids checking for ES enabled before checking
>> the SNP enabled. I understand that in the current code path, SNP check
>> is called inside the SevEsProtocolCheck() -- ES is already enabled, and
>> its safe to call SEV_STATUS MSR. What if we need to check for the SNP
>> state outside the ES-specific code block in the future? Then we will
>> need to extend the SevEsWorkArea.
> What would be the reason for this, ever?

One reason I can think of is if we ever decided validate the pages
before the SevEsProtocolCheck(). The version 2 of GHCB spec adds few new
NAE's that are SNP specific such as Page State Change. They are not
applicable to the ES guests. Currently, we do the page validation much
later and by then ProcessorConstructList() is called. Anyway, this is
not an important thing to consider right now. As I said, I will drop the
extending workarea to cache the SNP enable and Hypervisor feature values.


>
> I think this ties in with another point (or question) I raised
> elsewhere: the assembly code in the reset vector suggests *anyway* that
> SNP is only available if ES is available, but I couldn't verify that
> from any specs. If this dependency is an architectural fact (that is, if
> ES is absent, then SNP may never be present), then I wouldn't like to
> introduce a separate field for SNP presence in the SEC_SEV_ES_WORK_AREA
> structure.

The SEV-SNP builds upon existing SEV and SEV-ES support and provides an
additional protection from the hypervisor. The SEV-SNP feature requires
both the SEV and SEV-ES must be enabled. There is some text about it in
APM volume 2 [1] chapter  15.36.


[1] https://www.amd.com/system/files/TechDocs/24593.pdf


thanks



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#76220): https://edk2.groups.io/g/devel/message/76220
Mute This Topic: https://groups.io/mt/83113764/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-06-08 Thread Laszlo Ersek

On 06/07/21 15:00, Brijesh Singh wrote:
> Hi Laszlo,
> 
> 
> On 6/7/21 6:20 AM, Laszlo Ersek via groups.io wrote:
>> On 06/04/21 16:15, Laszlo Ersek wrote:
>>> On 05/27/21 01:10, Brijesh Singh wrote:
 BZ: 
 https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275data=04%7C01%7Cbrijesh.singh%40amd.com%7C4b1c71cc4cef4dd250f608d929a640cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586616293720447%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=Wulen9mRG3lzBHDOwrwP6%2BnV6F95LgnsgDxuEadi9Ng%3Dreserved=0

 Extend the workarea to include the SEV-SNP enabled fields. This will be set
 when SEV-SNP is active in the guest VM.

 Cc: James Bottomley 
 Cc: Min Xu 
 Cc: Jiewen Yao 
 Cc: Tom Lendacky 
 Cc: Jordan Justen 
 Cc: Ard Biesheuvel 
 Cc: Laszlo Ersek 
 Cc: Erdem Aktas 
 Signed-off-by: Brijesh Singh 
 ---
  OvmfPkg/PlatformPei/PlatformPei.inf|  1 +
  OvmfPkg/Include/Library/MemEncryptSevLib.h |  3 ++-
  OvmfPkg/PlatformPei/AmdSev.c   | 26 ++
  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 12 ++
  OvmfPkg/ResetVector/ResetVector.nasmb  |  1 +
  5 files changed, 42 insertions(+), 1 deletion(-)
>>> (1) Please split this in two patches -- the PlatformPei changes should
>>> be a separate patch. And, I think those should come second, the
>>> ResetVector + header file change should come first.
>>>
 diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
 b/OvmfPkg/PlatformPei/PlatformPei.inf
 index 6ef77ba7bb21..bc1dcac48343 100644
 --- a/OvmfPkg/PlatformPei/PlatformPei.inf
 +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
 @@ -110,6 +110,7 @@ [Pcd]
gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
 +  gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled

  [FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
 diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h 
 b/OvmfPkg/Include/Library/MemEncryptSevLib.h
 index 2425d8ba0a36..24507de55c5d 100644
 --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
 +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
 @@ -49,7 +49,8 @@ typedef struct {
  //
  typedef struct _SEC_SEV_ES_WORK_AREA {
UINT8SevEsEnabled;
 -  UINT8Reserved1[7];
 +  UINT8SevSnpEnabled;
 +  UINT8Reserved2[6];

UINT64   RandomData;

 diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
 index a8bf610022ba..67b78fd5fa36 100644
 --- a/OvmfPkg/PlatformPei/AmdSev.c
 +++ b/OvmfPkg/PlatformPei/AmdSev.c
 @@ -22,6 +22,27 @@

  #include "Platform.h"

 +/**
 +
 +  Initialize SEV-SNP support if running as an SEV-SNP guest.
 +
 +  **/
 +STATIC
 +VOID
 +AmdSevSnpInitialize (
 +  VOID
 +  )
 +{
 +  RETURN_STATUSPcdStatus;
 +
 +  if (!MemEncryptSevSnpIsEnabled ()) {
 +return;
 +  }
 +
 +  PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
 +  ASSERT_RETURN_ERROR (PcdStatus);
 +}
 +
  /**

Initialize SEV-ES support if running as an SEV-ES guest.
 @@ -209,4 +230,9 @@ AmdSevInitialize (
// Check and perform SEV-ES initialization if required.
//
AmdSevEsInitialize ();
 +
 +  //
 +  // Check and perform SEV-SNP initialization if required.
 +  //
 +  AmdSevSnpInitialize ();
  }
 diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
 b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
 index 5fae8986d9da..6838cdeec9c3 100644
 --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
 +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
 @@ -81,6 +81,11 @@ CheckSevFeatures:
  ; the MSR check below will set the first byte of the workarea to one.
  mov byte[SEV_ES_WORK_AREA], 0

 +; Set the SevSnpEnabled field in workarea to zero to communicate to 
 the SEC
 +; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this 
 function
 +; will set it to 1.
 +mov   byte[SEV_ES_WORK_AREA_SNP], 0
 +
  ;
  ; Set up exception handlers to check for SEV-ES
  ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
 @@ -136,6 +141,13 @@ CheckSevFeatures:
  ; phase that SEV-ES is enabled.
  mov   byte[SEV_ES_WORK_AREA], 1

 +bteax, 2
 +jnc   GetSevEncBit
 +
 +; Set the second byte of the workarea to one to communicate to the SEC
 +; phase that the SEV-SNP is enabled
 +mov   byte[SEV_ES_WORK_AREA_SNP], 1
 +
  GetSevEncBit:
  ; Get pte bit

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-06-07 Thread Brijesh Singh via groups.io

Hi Laszlo,


On 6/7/21 6:20 AM, Laszlo Ersek via groups.io wrote:
> On 06/04/21 16:15, Laszlo Ersek wrote:
>> On 05/27/21 01:10, Brijesh Singh wrote:
>>> BZ: 
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275data=04%7C01%7Cbrijesh.singh%40amd.com%7C4b1c71cc4cef4dd250f608d929a640cb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637586616293720447%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000sdata=Wulen9mRG3lzBHDOwrwP6%2BnV6F95LgnsgDxuEadi9Ng%3Dreserved=0
>>>
>>> Extend the workarea to include the SEV-SNP enabled fields. This will be set
>>> when SEV-SNP is active in the guest VM.
>>>
>>> Cc: James Bottomley 
>>> Cc: Min Xu 
>>> Cc: Jiewen Yao 
>>> Cc: Tom Lendacky 
>>> Cc: Jordan Justen 
>>> Cc: Ard Biesheuvel 
>>> Cc: Laszlo Ersek 
>>> Cc: Erdem Aktas 
>>> Signed-off-by: Brijesh Singh 
>>> ---
>>>  OvmfPkg/PlatformPei/PlatformPei.inf|  1 +
>>>  OvmfPkg/Include/Library/MemEncryptSevLib.h |  3 ++-
>>>  OvmfPkg/PlatformPei/AmdSev.c   | 26 ++
>>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 12 ++
>>>  OvmfPkg/ResetVector/ResetVector.nasmb  |  1 +
>>>  5 files changed, 42 insertions(+), 1 deletion(-)
>> (1) Please split this in two patches -- the PlatformPei changes should
>> be a separate patch. And, I think those should come second, the
>> ResetVector + header file change should come first.
>>
>>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
>>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> index 6ef77ba7bb21..bc1dcac48343 100644
>>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>>> @@ -110,6 +110,7 @@ [Pcd]
>>>gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>>>gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>>>gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
>>> +  gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
>>>
>>>  [FixedPcd]
>>>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>>> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h 
>>> b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>>> index 2425d8ba0a36..24507de55c5d 100644
>>> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
>>> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>>> @@ -49,7 +49,8 @@ typedef struct {
>>>  //
>>>  typedef struct _SEC_SEV_ES_WORK_AREA {
>>>UINT8SevEsEnabled;
>>> -  UINT8Reserved1[7];
>>> +  UINT8SevSnpEnabled;
>>> +  UINT8Reserved2[6];
>>>
>>>UINT64   RandomData;
>>>
>>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>>> index a8bf610022ba..67b78fd5fa36 100644
>>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>>> @@ -22,6 +22,27 @@
>>>
>>>  #include "Platform.h"
>>>
>>> +/**
>>> +
>>> +  Initialize SEV-SNP support if running as an SEV-SNP guest.
>>> +
>>> +  **/
>>> +STATIC
>>> +VOID
>>> +AmdSevSnpInitialize (
>>> +  VOID
>>> +  )
>>> +{
>>> +  RETURN_STATUSPcdStatus;
>>> +
>>> +  if (!MemEncryptSevSnpIsEnabled ()) {
>>> +return;
>>> +  }
>>> +
>>> +  PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
>>> +  ASSERT_RETURN_ERROR (PcdStatus);
>>> +}
>>> +
>>>  /**
>>>
>>>Initialize SEV-ES support if running as an SEV-ES guest.
>>> @@ -209,4 +230,9 @@ AmdSevInitialize (
>>>// Check and perform SEV-ES initialization if required.
>>>//
>>>AmdSevEsInitialize ();
>>> +
>>> +  //
>>> +  // Check and perform SEV-SNP initialization if required.
>>> +  //
>>> +  AmdSevSnpInitialize ();
>>>  }
>>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
>>> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> index 5fae8986d9da..6838cdeec9c3 100644
>>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>>> @@ -81,6 +81,11 @@ CheckSevFeatures:
>>>  ; the MSR check below will set the first byte of the workarea to one.
>>>  mov byte[SEV_ES_WORK_AREA], 0
>>>
>>> +; Set the SevSnpEnabled field in workarea to zero to communicate to 
>>> the SEC
>>> +; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this 
>>> function
>>> +; will set it to 1.
>>> +mov   byte[SEV_ES_WORK_AREA_SNP], 0
>>> +
>>>  ;
>>>  ; Set up exception handlers to check for SEV-ES
>>>  ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
>>> @@ -136,6 +141,13 @@ CheckSevFeatures:
>>>  ; phase that SEV-ES is enabled.
>>>  mov   byte[SEV_ES_WORK_AREA], 1
>>>
>>> +bteax, 2
>>> +jnc   GetSevEncBit
>>> +
>>> +; Set the second byte of the workarea to one to communicate to the SEC
>>> +; phase that the SEV-SNP is enabled
>>> +mov   byte[SEV_ES_WORK_AREA_SNP], 1
>>> +
>>>  GetSevEncBit:
>>>  ; Get pte bit position to enable memory encryption
>>>  ; CPUID Fn8000_001F[EBX] - Bits 5:0
>> (2) Please mention in the commit message (of the ResetVector patch),
>> and/or a comment here in

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-06-07 Thread Laszlo Ersek

On 06/04/21 16:15, Laszlo Ersek wrote:
> On 05/27/21 01:10, Brijesh Singh wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
>>
>> Extend the workarea to include the SEV-SNP enabled fields. This will be set
>> when SEV-SNP is active in the guest VM.
>>
>> Cc: James Bottomley 
>> Cc: Min Xu 
>> Cc: Jiewen Yao 
>> Cc: Tom Lendacky 
>> Cc: Jordan Justen 
>> Cc: Ard Biesheuvel 
>> Cc: Laszlo Ersek 
>> Cc: Erdem Aktas 
>> Signed-off-by: Brijesh Singh 
>> ---
>>  OvmfPkg/PlatformPei/PlatformPei.inf|  1 +
>>  OvmfPkg/Include/Library/MemEncryptSevLib.h |  3 ++-
>>  OvmfPkg/PlatformPei/AmdSev.c   | 26 ++
>>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 12 ++
>>  OvmfPkg/ResetVector/ResetVector.nasmb  |  1 +
>>  5 files changed, 42 insertions(+), 1 deletion(-)
>
> (1) Please split this in two patches -- the PlatformPei changes should
> be a separate patch. And, I think those should come second, the
> ResetVector + header file change should come first.
>
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
>> b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index 6ef77ba7bb21..bc1dcac48343 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -110,6 +110,7 @@ [Pcd]
>>gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>>gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>>gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
>> +  gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
>>
>>  [FixedPcd]
>>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
>> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h 
>> b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> index 2425d8ba0a36..24507de55c5d 100644
>> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> @@ -49,7 +49,8 @@ typedef struct {
>>  //
>>  typedef struct _SEC_SEV_ES_WORK_AREA {
>>UINT8SevEsEnabled;
>> -  UINT8Reserved1[7];
>> +  UINT8SevSnpEnabled;
>> +  UINT8Reserved2[6];
>>
>>UINT64   RandomData;
>>
>> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
>> index a8bf610022ba..67b78fd5fa36 100644
>> --- a/OvmfPkg/PlatformPei/AmdSev.c
>> +++ b/OvmfPkg/PlatformPei/AmdSev.c
>> @@ -22,6 +22,27 @@
>>
>>  #include "Platform.h"
>>
>> +/**
>> +
>> +  Initialize SEV-SNP support if running as an SEV-SNP guest.
>> +
>> +  **/
>> +STATIC
>> +VOID
>> +AmdSevSnpInitialize (
>> +  VOID
>> +  )
>> +{
>> +  RETURN_STATUSPcdStatus;
>> +
>> +  if (!MemEncryptSevSnpIsEnabled ()) {
>> +return;
>> +  }
>> +
>> +  PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>> +}
>> +
>>  /**
>>
>>Initialize SEV-ES support if running as an SEV-ES guest.
>> @@ -209,4 +230,9 @@ AmdSevInitialize (
>>// Check and perform SEV-ES initialization if required.
>>//
>>AmdSevEsInitialize ();
>> +
>> +  //
>> +  // Check and perform SEV-SNP initialization if required.
>> +  //
>> +  AmdSevSnpInitialize ();
>>  }
>> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
>> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> index 5fae8986d9da..6838cdeec9c3 100644
>> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
>> @@ -81,6 +81,11 @@ CheckSevFeatures:
>>  ; the MSR check below will set the first byte of the workarea to one.
>>  mov byte[SEV_ES_WORK_AREA], 0
>>
>> +; Set the SevSnpEnabled field in workarea to zero to communicate to the 
>> SEC
>> +; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this 
>> function
>> +; will set it to 1.
>> +mov   byte[SEV_ES_WORK_AREA_SNP], 0
>> +
>>  ;
>>  ; Set up exception handlers to check for SEV-ES
>>  ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
>> @@ -136,6 +141,13 @@ CheckSevFeatures:
>>  ; phase that SEV-ES is enabled.
>>  mov   byte[SEV_ES_WORK_AREA], 1
>>
>> +bteax, 2
>> +jnc   GetSevEncBit
>> +
>> +; Set the second byte of the workarea to one to communicate to the SEC
>> +; phase that the SEV-SNP is enabled
>> +mov   byte[SEV_ES_WORK_AREA_SNP], 1
>> +
>>  GetSevEncBit:
>>  ; Get pte bit position to enable memory encryption
>>  ; CPUID Fn8000_001F[EBX] - Bits 5:0
>
> (2) Please mention in the commit message (of the ResetVector patch),
> and/or a comment here in the code, that SEV-SNP is never enabled if
> SEV-ES is disabled.
>
> Section "15.34.10 SEV_STATUS MSR" in the APM (doc#24593 v3.37) does not
> spell out this dependency.
>
> Furthermore, the mSevStatus / mSevEsStatus / mSevSnpStatus variable
> assignments in patch#2 do not form a "dependency cascade" like the one
> seen here in the reset vector code.
>
> While "SEV-ES depends on SEV" seems obvious to me (and so the related,
> existent jumps in the assembly code are not surprising), the statement
> "SEV-SNP depends on SEV-ES" is not *that* obvious to me.

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-06-04 Thread Laszlo Ersek

On 05/27/21 01:10, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> Extend the workarea to include the SEV-SNP enabled fields. This will be set
> when SEV-SNP is active in the guest VM.
> 
> Cc: James Bottomley 
> Cc: Min Xu 
> Cc: Jiewen Yao 
> Cc: Tom Lendacky 
> Cc: Jordan Justen 
> Cc: Ard Biesheuvel 
> Cc: Laszlo Ersek 
> Cc: Erdem Aktas 
> Signed-off-by: Brijesh Singh 
> ---
>  OvmfPkg/PlatformPei/PlatformPei.inf|  1 +
>  OvmfPkg/Include/Library/MemEncryptSevLib.h |  3 ++-
>  OvmfPkg/PlatformPei/AmdSev.c   | 26 ++
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 12 ++
>  OvmfPkg/ResetVector/ResetVector.nasmb  |  1 +
>  5 files changed, 42 insertions(+), 1 deletion(-)

(1) Please split this in two patches -- the PlatformPei changes should
be a separate patch. And, I think those should come second, the
ResetVector + header file change should come first.

> 
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6ef77ba7bb21..bc1dcac48343 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -110,6 +110,7 @@ [Pcd]
>gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
> +  gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
>  
>  [FixedPcd]
>gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h 
> b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 2425d8ba0a36..24507de55c5d 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -49,7 +49,8 @@ typedef struct {
>  //
>  typedef struct _SEC_SEV_ES_WORK_AREA {
>UINT8SevEsEnabled;
> -  UINT8Reserved1[7];
> +  UINT8SevSnpEnabled;
> +  UINT8Reserved2[6];
>  
>UINT64   RandomData;
>  
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index a8bf610022ba..67b78fd5fa36 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -22,6 +22,27 @@
>  
>  #include "Platform.h"
>  
> +/**
> +
> +  Initialize SEV-SNP support if running as an SEV-SNP guest.
> +
> +  **/
> +STATIC
> +VOID
> +AmdSevSnpInitialize (
> +  VOID
> +  )
> +{
> +  RETURN_STATUSPcdStatus;
> +
> +  if (!MemEncryptSevSnpIsEnabled ()) {
> +return;
> +  }
> +
> +  PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +}
> +
>  /**
>  
>Initialize SEV-ES support if running as an SEV-ES guest.
> @@ -209,4 +230,9 @@ AmdSevInitialize (
>// Check and perform SEV-ES initialization if required.
>//
>AmdSevEsInitialize ();
> +
> +  //
> +  // Check and perform SEV-SNP initialization if required.
> +  //
> +  AmdSevSnpInitialize ();
>  }
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
> b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 5fae8986d9da..6838cdeec9c3 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -81,6 +81,11 @@ CheckSevFeatures:
>  ; the MSR check below will set the first byte of the workarea to one.
>  mov byte[SEV_ES_WORK_AREA], 0
>  
> +; Set the SevSnpEnabled field in workarea to zero to communicate to the 
> SEC
> +; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this function
> +; will set it to 1.
> +mov   byte[SEV_ES_WORK_AREA_SNP], 0
> +
>  ;
>  ; Set up exception handlers to check for SEV-ES
>  ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
> @@ -136,6 +141,13 @@ CheckSevFeatures:
>  ; phase that SEV-ES is enabled.
>  mov   byte[SEV_ES_WORK_AREA], 1
>  
> +bteax, 2
> +jnc   GetSevEncBit
> +
> +; Set the second byte of the workarea to one to communicate to the SEC
> +; phase that the SEV-SNP is enabled
> +mov   byte[SEV_ES_WORK_AREA_SNP], 1
> +
>  GetSevEncBit:
>  ; Get pte bit position to enable memory encryption
>  ; CPUID Fn8000_001F[EBX] - Bits 5:0

(2) Please mention in the commit message (of the ResetVector patch),
and/or a comment here in the code, that SEV-SNP is never enabled if
SEV-ES is disabled.

Section "15.34.10 SEV_STATUS MSR" in the APM (doc#24593 v3.37) does not
spell out this dependency.

Furthermore, the mSevStatus / mSevEsStatus / mSevSnpStatus variable
assignments in patch#2 do not form a "dependency cascade" like the one
seen here in the reset vector code.

While "SEV-ES depends on SEV" seems obvious to me (and so the related,
existent jumps in the assembly code are not surprising), the statement
"SEV-SNP depends on SEV-ES" is not *that* obvious to me. Thus a comment
would be welcome.

For *both* patches split out of this one:

Reviewed-by: Laszlo Ersek 

Thanks
Laszlo

> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb 
>

[edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

2021-05-26 Thread Brijesh Singh

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275

Extend the workarea to include the SEV-SNP enabled fields. This will be set
when SEV-SNP is active in the guest VM.

Cc: James Bottomley 
Cc: Min Xu 
Cc: Jiewen Yao 
Cc: Tom Lendacky 
Cc: Jordan Justen 
Cc: Ard Biesheuvel 
Cc: Laszlo Ersek 
Cc: Erdem Aktas 
Signed-off-by: Brijesh Singh 
---
 OvmfPkg/PlatformPei/PlatformPei.inf|  1 +
 OvmfPkg/Include/Library/MemEncryptSevLib.h |  3 ++-
 OvmfPkg/PlatformPei/AmdSev.c   | 26 ++
 OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 12 ++
 OvmfPkg/ResetVector/ResetVector.nasmb  |  1 +
 5 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf 
b/OvmfPkg/PlatformPei/PlatformPei.inf
index 6ef77ba7bb21..bc1dcac48343 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -110,6 +110,7 @@ [Pcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
   gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
+  gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
 
 [FixedPcd]
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h 
b/OvmfPkg/Include/Library/MemEncryptSevLib.h
index 2425d8ba0a36..24507de55c5d 100644
--- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
+++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
@@ -49,7 +49,8 @@ typedef struct {
 //
 typedef struct _SEC_SEV_ES_WORK_AREA {
   UINT8SevEsEnabled;
-  UINT8Reserved1[7];
+  UINT8SevSnpEnabled;
+  UINT8Reserved2[6];
 
   UINT64   RandomData;
 
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index a8bf610022ba..67b78fd5fa36 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -22,6 +22,27 @@
 
 #include "Platform.h"
 
+/**
+
+  Initialize SEV-SNP support if running as an SEV-SNP guest.
+
+  **/
+STATIC
+VOID
+AmdSevSnpInitialize (
+  VOID
+  )
+{
+  RETURN_STATUSPcdStatus;
+
+  if (!MemEncryptSevSnpIsEnabled ()) {
+return;
+  }
+
+  PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
+  ASSERT_RETURN_ERROR (PcdStatus);
+}
+
 /**
 
   Initialize SEV-ES support if running as an SEV-ES guest.
@@ -209,4 +230,9 @@ AmdSevInitialize (
   // Check and perform SEV-ES initialization if required.
   //
   AmdSevEsInitialize ();
+
+  //
+  // Check and perform SEV-SNP initialization if required.
+  //
+  AmdSevSnpInitialize ();
 }
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 5fae8986d9da..6838cdeec9c3 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -81,6 +81,11 @@ CheckSevFeatures:
 ; the MSR check below will set the first byte of the workarea to one.
 mov byte[SEV_ES_WORK_AREA], 0
 
+; Set the SevSnpEnabled field in workarea to zero to communicate to the SEC
+; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this function
+; will set it to 1.
+mov   byte[SEV_ES_WORK_AREA_SNP], 0
+
 ;
 ; Set up exception handlers to check for SEV-ES
 ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
@@ -136,6 +141,13 @@ CheckSevFeatures:
 ; phase that SEV-ES is enabled.
 mov   byte[SEV_ES_WORK_AREA], 1
 
+bteax, 2
+jnc   GetSevEncBit
+
+; Set the second byte of the workarea to one to communicate to the SEC
+; phase that the SEV-SNP is enabled
+mov   byte[SEV_ES_WORK_AREA_SNP], 1
+
 GetSevEncBit:
 ; Get pte bit position to enable memory encryption
 ; CPUID Fn8000_001F[EBX] - Bits 5:0
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb 
b/OvmfPkg/ResetVector/ResetVector.nasmb
index 5fbacaed5f9d..1971557b1c00 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -73,6 +73,7 @@
   %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
   %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
   %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
+  %define SEV_ES_WORK_AREA_SNP (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 1)
   %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
   %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
   %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + 
FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75719): https://edk2.groups.io/g/devel/message/75719
Mute This Topic: https://groups.io/mt/83113764/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

Re: [edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

[edk2-devel] [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

7 matches

Site Navigation

Mail list logo

Footer information