Re: [edk2-devel] [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check

2020-05-27 Thread Laszlo Ersek 
On 05/26/20 18:31, Tom Lendacky wrote:
> On 5/25/20 11:50 AM, Laszlo Ersek wrote:

>> Now I'm reviewing the differences (incrementally from v6 to v8), and I'm
>> shocked how many changes you incorporated into preexistent patches,
>> while keeping my R-b's.
> 
> My apologies for this. I was experimenting with cleaning things up and
> making the code more readable and I guess I forgot to either remove it
> or note it as a change to be re-reviewed - thinking I had done one or
> the other.

I agree that it's not easy to track previously given R-b's across
rebases (reordering patches, moving hunks between them, etc), especially
if there's no huge need to touch up the commit messages themselves.

What helps IME is to review the full set (just about to be posted)
before sending, with "git-range-diff", against the last posted version.
If code changes are shown for any patch (not just context differences),
take notes about those patches, then perform a final rebase, dropping
all the previously given tags from the affected patches.

(Clearly such a "git-range-diff" is incomparably faster on the author's
end than on the reviewer's end, as the author has it all in their
"working set".)

>> (1) Unless there was an actual bug in the v6 version of this patch,
>> please let's go back to that. IOW, if the v6->v8 changes are only
>> cleanups or optimizations, let's please postpone them.
> 
> Yes, I'll revert it back to the original version. Again, sorry for all
> of this churn.

Thank you!
Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#60340): https://edk2.groups.io/g/devel/message/60340
Mute This Topic: https://groups.io/mt/74336598/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check

2020-05-26 Thread Lendacky, Thomas 

On 5/25/20 11:50 AM, Laszlo Ersek wrote:

Tom,

On 05/19/20 23:51, Lendacky, Thomas wrote:

BZ: 
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198data=02%7C01%7Cthomas.lendacky%40amd.com%7C4fe7191ef9fe43793eb408d800cbbb44%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260222303712873sdata=DJP9%2Fbe1ttZ%2FGEwqZ2Flv5z4dTV0T8QkYdeSnaIGWcY%3Dreserved=0

During BSP startup, the reset vector code will issue a CPUID instruction
while in 32-bit mode. When running as an SEV-ES guest, this will trigger
a #VC exception.

Add exception handling support to the early reset vector code to catch
these exceptions.  Also, since the guest is in 32-bit mode at this point,
writes to the GHCB will be encrypted and thus not able to be read by the
hypervisor, so use the GHCB CPUID request/response protocol to obtain the
requested CPUID function values and provide these to the guest.

The exception handling support is active during the SEV check and uses the
OVMF temporary RAM space for a stack. After the SEV check is complete, the
exception handling support is removed and the stack pointer cleared.

Cc: Jordan Justen 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Reviewed-by: Laszlo Ersek 
Signed-off-by: Tom Lendacky 
---
  OvmfPkg/ResetVector/ResetVector.inf   |   2 +
  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 329 +++---
  OvmfPkg/ResetVector/ResetVector.nasmb |   1 +
  3 files changed, 294 insertions(+), 38 deletions(-)


this doesn't work for me.

Under your v5 posting, I reviewed those OvmfPkg patches that still
needed my review.

The v6 posting carried all my R-b's; all OvmfPkg patches had been
reviewed. I trusted you and I only verified the commit messages for my
R-b's. I thought the OvmfPkg state was final.

The v7 posting again carried my R-b's; I briefly checked the v6->v7
changes in the blurb, and re-checked my R-b's on the OvmfPkg patches.
This was in the v7 blurb:


Changes since v6:
- Add function comments to all functions, including local functions
- Add function parameter direction to all functions (in/out)
- Add support for MMIO MOVZX/MOVSX instructions
- Ensure the per-CPU variable page remains encrypted
- Coding-style fixes as identified by Ecc


This summary didn't indicate I'd have to go through the OvmfPkg patches
again -- and the presence of my R-b's on all the OvmfPkg patches
supported that impression.

I commented on v7 only later, independently; namely on two topics:

- on one of the S3 reservation aspects,
- on the upcoming / requested movement of VmgExitLib to OvmfPkg.

These were the two updates I was going to expect in v8.

So, in order to "page in" your work again, in preparation for reviewing
v8, I decided to review the v5->v6 changes in more detail -- the code
too (incrementally), not just the picking up of my R-b's, like I had
originally done under v6. I was happy with v6, after performing this
review; see 
.

Now I'm reviewing the differences (incrementally from v6 to v8), and I'm
shocked how many changes you incorporated into preexistent patches,
while keeping my R-b's.


My apologies for this. I was experimenting with cleaning things up and 
making the code more readable and I guess I forgot to either remove it or 
note it as a change to be re-reviewed - thinking I had done one or the other.




On this patch, you significantly changed the logic from v6 to v7, and I
don't have the slightest clue why. I don't feel inclined to
reverse-engineer the logic change from the v6->v7 interdiff. The right
way to present a significant change is to (a) drop the existent R-b's
from the patch, and (b) spell out the news in the blurb and/or in the
"notes" section of the individual patch. If you had dropped the R-b in
v7, then I would have known to review the changes in v7 at once (rather
than let it accumulate to v8). And if you had explained the updates, I
may have started with a re-review of the patch from scratch (and
wouldn't be stuck with an incremental one / interdiff now, between v6
and v8).

Then, the patch changed *again*, from v7 to v8; and my R-b (which only
applied to v6) got carried forward again.

Consider the v7->v8 changes noted in the blurb:


Changes since v7:
- Reserve the SEV-ES workarea when S3 is enabled
- Fix warnings issued by the Visual Studio compiler
- Create a NULL VmgExitLib instance that is used for VMGEXIT
   related operations as well as #VC handling. Then create the full
   VmgExitLib support only in OvmfPkg - where it will be used. This
   removes a bunch of implementation code from platforms that will
   not be using the functionality.
- Remove single use interfaces from

Re: [edk2-devel] [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check

2020-05-25 Thread Laszlo Ersek 
Tom,

On 05/19/20 23:51, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> During BSP startup, the reset vector code will issue a CPUID instruction
> while in 32-bit mode. When running as an SEV-ES guest, this will trigger
> a #VC exception.
>
> Add exception handling support to the early reset vector code to catch
> these exceptions.  Also, since the guest is in 32-bit mode at this point,
> writes to the GHCB will be encrypted and thus not able to be read by the
> hypervisor, so use the GHCB CPUID request/response protocol to obtain the
> requested CPUID function values and provide these to the guest.
>
> The exception handling support is active during the SEV check and uses the
> OVMF temporary RAM space for a stack. After the SEV check is complete, the
> exception handling support is removed and the stack pointer cleared.
>
> Cc: Jordan Justen 
> Cc: Laszlo Ersek 
> Cc: Ard Biesheuvel 
> Reviewed-by: Laszlo Ersek 
> Signed-off-by: Tom Lendacky 
> ---
>  OvmfPkg/ResetVector/ResetVector.inf   |   2 +
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm | 329 +++---
>  OvmfPkg/ResetVector/ResetVector.nasmb |   1 +
>  3 files changed, 294 insertions(+), 38 deletions(-)

this doesn't work for me.

Under your v5 posting, I reviewed those OvmfPkg patches that still
needed my review.

The v6 posting carried all my R-b's; all OvmfPkg patches had been
reviewed. I trusted you and I only verified the commit messages for my
R-b's. I thought the OvmfPkg state was final.

The v7 posting again carried my R-b's; I briefly checked the v6->v7
changes in the blurb, and re-checked my R-b's on the OvmfPkg patches.
This was in the v7 blurb:

> Changes since v6:
> - Add function comments to all functions, including local functions
> - Add function parameter direction to all functions (in/out)
> - Add support for MMIO MOVZX/MOVSX instructions
> - Ensure the per-CPU variable page remains encrypted
> - Coding-style fixes as identified by Ecc

This summary didn't indicate I'd have to go through the OvmfPkg patches
again -- and the presence of my R-b's on all the OvmfPkg patches
supported that impression.

I commented on v7 only later, independently; namely on two topics:

- on one of the S3 reservation aspects,
- on the upcoming / requested movement of VmgExitLib to OvmfPkg.

These were the two updates I was going to expect in v8.

So, in order to "page in" your work again, in preparation for reviewing
v8, I decided to review the v5->v6 changes in more detail -- the code
too (incrementally), not just the picking up of my R-b's, like I had
originally done under v6. I was happy with v6, after performing this
review; see .

Now I'm reviewing the differences (incrementally from v6 to v8), and I'm
shocked how many changes you incorporated into preexistent patches,
while keeping my R-b's.

On this patch, you significantly changed the logic from v6 to v7, and I
don't have the slightest clue why. I don't feel inclined to
reverse-engineer the logic change from the v6->v7 interdiff. The right
way to present a significant change is to (a) drop the existent R-b's
from the patch, and (b) spell out the news in the blurb and/or in the
"notes" section of the individual patch. If you had dropped the R-b in
v7, then I would have known to review the changes in v7 at once (rather
than let it accumulate to v8). And if you had explained the updates, I
may have started with a re-review of the patch from scratch (and
wouldn't be stuck with an incremental one / interdiff now, between v6
and v8).

Then, the patch changed *again*, from v7 to v8; and my R-b (which only
applied to v6) got carried forward again.

Consider the v7->v8 changes noted in the blurb:

> Changes since v7:
> - Reserve the SEV-ES workarea when S3 is enabled
> - Fix warnings issued by the Visual Studio compiler
> - Create a NULL VmgExitLib instance that is used for VMGEXIT
>   related operations as well as #VC handling. Then create the full
>   VmgExitLib support only in OvmfPkg - where it will be used. This
>   removes a bunch of implementation code from platforms that will
>   not be using the functionality.
> - Remove single use interfaces from the VmgExitLib (VmgMmioWrite
>   and VmgSetApJumpTable)

Not a word on this patch, as far as I can see.

I don't even know what to do about this patch now. I'd be really unhappy
to review it from zero; it's a difficult one. The reset vector is also
shared with non-SEV X64, so it's not like I can just slap an Acked-by on
it.

(1) Unless there was an actual bug in the v6 version of this patch,
please let's go back to that. IOW, if the v6->v8 changes are only
cleanups or optimizations, let's please postpone them.

I'm going to take a walk now.

Laszlo


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#60215): https://edk2.groups.io/g/devel/message/60215
Mute This Topic:

[edk2-devel] [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check

2020-05-19 Thread Lendacky, Thomas 
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198

During BSP startup, the reset vector code will issue a CPUID instruction
while in 32-bit mode. When running as an SEV-ES guest, this will trigger
a #VC exception.

Add exception handling support to the early reset vector code to catch
these exceptions.  Also, since the guest is in 32-bit mode at this point,
writes to the GHCB will be encrypted and thus not able to be read by the
hypervisor, so use the GHCB CPUID request/response protocol to obtain the
requested CPUID function values and provide these to the guest.

The exception handling support is active during the SEV check and uses the
OVMF temporary RAM space for a stack. After the SEV check is complete, the
exception handling support is removed and the stack pointer cleared.

Cc: Jordan Justen 
Cc: Laszlo Ersek 
Cc: Ard Biesheuvel 
Reviewed-by: Laszlo Ersek 
Signed-off-by: Tom Lendacky 
---
 OvmfPkg/ResetVector/ResetVector.inf   |   2 +
 OvmfPkg/ResetVector/Ia32/PageTables64.asm | 329 +++---
 OvmfPkg/ResetVector/ResetVector.nasmb |   1 +
 3 files changed, 294 insertions(+), 38 deletions(-)

diff --git a/OvmfPkg/ResetVector/ResetVector.inf 
b/OvmfPkg/ResetVector/ResetVector.inf
index e94e1bfcce7e..a53ae6c194ae 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -41,3 +41,5 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
+  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm 
b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index 73a4eaadb1b6..8a24e7fd42f6 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -36,13 +36,56 @@ BITS32
PAGE_READ_WRITE + \
PAGE_PRESENT)
 
-; Check if Secure Encrypted Virtualization (SEV) feature is enabled
 ;
-; If SEV is enabled then EAX will be at least 32
+; SEV-ES #VC exception handler support
+;
+; #VC handler local variable locations
+;
+%define VC_CPUID_RESULT_EAX 0
+%define VC_CPUID_RESULT_EBX 4
+%define VC_CPUID_RESULT_ECX 8
+%define VC_CPUID_RESULT_EDX12
+%define VC_GHCB_MSR_EDX16
+%define VC_GHCB_MSR_EAX20
+%define VC_CPUID_REQUEST_REGISTER  24
+%define VC_CPUID_FUNCTION  28
+
+; #VC handler total local variable size
+;
+%define VC_VARIABLE_SIZE   32
+
+; #VC handler GHCB CPUID request/response protocol values
+;
+%define GHCB_CPUID_REQUEST  4
+%define GHCB_CPUID_RESPONSE 5
+%define GHCB_CPUID_REGISTER_SHIFT  30
+%define CPUID_INSN_LEN  2
+
+
+; Check if Secure Encrypted Virtualization (SEV) features are enabled
+;
+; Modified:  EAX, EBX, ECX, EDX, ESP
+;
+; If SEV is enabled then EAX will be at least 32.
 ; If SEV is disabled then EAX will be zero.
 ;
-CheckSevFeature:
+CheckSevFeatures:
+; Initialize the first byte of the workarea to zero to communicate to
+; the SEC phase that SEV-ES is not enabled.
+mov byte[SEV_ES_WORK_AREA], 0
+
+;
+; Set up exception handlers to check for SEV-ES
+;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
+;   stack usage)
+;   Establish exception handlers
+;
+mov   esp, SEV_ES_VC_TOP_OF_STACK
+mov   eax, ADDR_OF(Idtr)
+lidt  [cs:eax]
+
 ; Check if we have a valid (0x8000_001F) CPUID leaf
+;   CPUID raises a #VC exception if running as an SEV-ES guest
 mov   eax, 0x8000
 cpuid
 
@@ -53,8 +96,8 @@ CheckSevFeature:
 jlNoSev
 
 ; Check for memory encryption feature:
-;  CPUID  Fn8000_001F[EAX] - Bit 1
-;
+; CPUID  Fn8000_001F[EAX] - Bit 1
+;   CPUID raises a #VC exception if running as an SEV-ES guest
 mov   eax,  0x801f
 cpuid
 bteax, 1
@@ -67,6 +110,16 @@ CheckSevFeature:
 bteax, 0
 jnc   NoSev
 
+; Check if SEV-ES is enabled
+;  MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
+bteax, 1
+jnc   NoSevEs
+
+; Set the first byte of the workarea to one to communicate to the SEC
+; phase that SEV-ES is enabled.
+mov   byte[SEV_ES_WORK_AREA], 1
+
+NoSevEs:
 ; Get pte bit position to enable memory encryption
 ; CPUID Fn8000_001F[EBX] - Bits 5:0
 ;
@@ -78,56 +131,44 @@ NoSev:
 xor   eax, eax
 
 SevExit:
-OneTimeCallRet CheckSevFeature
+;
+; Clear exception handlers and stack
+;
+push  eax
+mov   eax, ADDR_OF(IdtrClear)
+lidt  [cs:eax]
+pop   eax
+mov   esp, 0
+
+OneTimeCallRet CheckSevFeatures
 
 ; Check if Secure Encrypted Virtualization - Encrypted State (SEV-ES) feature
 ; is enabled.
 ;
-; Modified:  EAX, EBX, ECX
+; Modified:  EAX
 ;
 ; If