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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4fe7191ef9fe43793eb408d800cbbb44%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260222303712873&sdata=DJP9%2Fbe1ttZ%2FGEwqZ2Flv5z4dTV0T8QkYdeSnaIGWcY%3D&reserved=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 <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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198%23c10&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C4fe7191ef9fe43793eb408d800cbbb44%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260222303712873&amp;sdata=bK4imTQl9UvEJ9Y96QR7CuBkLSwi14MqSacXwh7JzhY%3D&amp;reserved=0>.
> 
> 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 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.

Yes, I'll revert it back to the original version. Again, sorry for all of 
this churn.

Thanks,
Tom

> 
> I'm going to take a walk now.
> 
> Laszlo
>