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.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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198%23c10&data=02%7C01%7Cthomas.lendacky%40amd.com%7C4fe7191ef9fe43793eb408d800cbbb44%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260222303712873&sdata=bK4imTQl9UvEJ9Y96QR7CuBkLSwi14MqSacXwh7JzhY%3D&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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#60292): https://edk2.groups.io/g/devel/message/60292
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]
-=-=-=-=-=-=-=-=-=-=-=-
