[edk2-devel] [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check
Laszlo Ersek
lersek at redhat.com
Mon May 25 16:50:07 UTC 2020
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 <jordan.l.justen at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Reviewed-by: Laszlo Ersek <lersek at redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky at amd.com>
> ---
> 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 <https://bugzilla.tianocore.org/show_bug.cgi?id=2198#c10>.
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: https://groups.io/mt/74336598/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