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.jus...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> > Reviewed-by: Laszlo Ersek <ler...@redhat.com> > Signed-off-by: Tom Lendacky <thomas.lenda...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-