From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web12.34675.1590425428259482068 for ; Mon, 25 May 2020 09:50:29 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=MwBvsnnR; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590425427; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=eMVFFRHFX+QLyXF7oQakhfWXzAkO8ca+64S4122gBTw=; b=MwBvsnnR44Yxft25X4uEkwFTIoin+18zsDE/hy5Xf9G4ZExj+xwRGKn6Ob+YiqJVvIs1Mw IbpIXD72Mu9Ik5Q//iLAWfm5cI/YhU3itZO3mkK96UnNCE10V2mD62T3eV6V/6jWaVA4c8 N5wDYTr+FtMpPXMI5sPtFTrOoOtkewE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-441-i2uaOtcENvu6RBmTuY7vnw-1; Mon, 25 May 2020 12:50:12 -0400 X-MC-Unique: i2uaOtcENvu6RBmTuY7vnw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6AB58800688; Mon, 25 May 2020 16:50:10 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-168.ams2.redhat.com [10.36.114.168]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4A6D66EDA4; Mon, 25 May 2020 16:50:08 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: From: "Laszlo Ersek" Message-ID: <337fcec3-28bc-6498-1b73-0b4c7409d01d@redhat.com> Date: Mon, 25 May 2020 18:50:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Reviewed-by: Laszlo Ersek > Signed-off-by: Tom Lendacky > --- > 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 . 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