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

["Laszlo Ersek" <lersek@redhat.com>]

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@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@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