From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.8244.1602753549260653945 for ; Thu, 15 Oct 2020 02:19:09 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=dFn4nvv3; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1602753548; 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=vBuJtqqqNGRn+L15v2cimYNS6cNY5JOAZppJLXvMcmo=; b=dFn4nvv3ePTdCI3DJmlEoQDrAvlegQNOMgatqJ6sZssieRRGZMkxmTy4Y0ZEMS4yAlLVr1 zbBxwByyvupNF8LlaWv9esVtCVfW3N5X4GqC1M+3u1Qn8yT68tfQSdidhVgULHMNLaN3fH nOpd6k0RvZXFGAwWf9ubpP/5MTCG9VA= 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-185-WhuqKNefOf6M-9pSeh9Whg-1; Thu, 15 Oct 2020 05:19:06 -0400 X-MC-Unique: WhuqKNefOf6M-9pSeh9Whg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8E15664082; Thu, 15 Oct 2020 09:19:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-119.ams2.redhat.com [10.36.113.119]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2867D50B44; Thu, 15 Oct 2020 09:19:02 +0000 (UTC) Subject: Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT From: "Laszlo Ersek" To: Tom Lendacky , devel@edk2.groups.io Cc: Brijesh Singh , Jordan Justen , Ard Biesheuvel References: <34bc7f77-2b5a-2e74-e78e-ddf606b6305b@redhat.com> <71b855ac-8381-4767-798c-2fafd2b9bd56@redhat.com> Message-ID: <78000bae-da10-970c-af49-0e484493f47c@redhat.com> Date: Thu, 15 Oct 2020 11:19:02 +0200 MIME-Version: 1.0 In-Reply-To: <71b855ac-8381-4767-798c-2fafd2b9bd56@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/15/20 10:50, Laszlo Ersek wrote: > On 10/15/20 10:47, Laszlo Ersek wrote: >> On 10/10/20 18:07, Tom Lendacky wrote: >>> From: Tom Lendacky >>> >>> All fields that are set in the GHCB should have their associated bit in >>> the GHCB ValidBitmap field set. Add support to set the bits for the >>> software exit information fields when performing a VMGEXIT (SwExitCode, >>> SwExitInfo1, SwExitInfo2). >>> >>> Fixes: 61bacc0fa16f ("OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF") >>> Cc: Jordan Justen >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Cc: Tom Lendacky >>> Cc: Brijesh Singh >>> Signed-off-by: Tom Lendacky >>> --- >>> OvmfPkg/Library/VmgExitLib/VmgExitLib.c | 30 ++++++++++++++++++++ >>> 1 file changed, 30 insertions(+) >>> >>> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c >>> index 53040cc6f649..6cf649c6101b 100644 >>> --- a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c >>> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c >>> @@ -78,6 +78,32 @@ VmgExitErrorCheck ( >>> return Status; >>> } >>> >>> +/** >>> + Marks a field at the specified offset as valid in the GHCB. >>> + >>> + The ValidBitmap area represents the areas of the GHCB that have been marked >>> + valid. Set the area of the GHCB at the specified offset as valid. >>> + >>> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication Block >>> + @param[in] Offset Offset in the GHCB to mark valid >>> + >>> +**/ >>> +STATIC >>> +VOID >>> +GhcbSetOffsetValid ( >>> + IN OUT GHCB *Ghcb, >>> + IN GHCB_QWORD_OFFSET Offset >>> + ) >>> +{ >>> + UINT32 OffsetIndex; >>> + UINT32 OffsetBit; >>> + >>> + OffsetIndex = Offset / 8; >>> + OffsetBit = Offset & 0x07; >> >> (1) I suggest improving the consistency of operators -- please either >> use division and remainder ("Offset / 8" and "Offset % 8"), or bit shift >> and masking ("Offset >> 3" and "Offset & 0x7") >> >> With that: >> >> Acked-by: Laszlo Ersek > > ... I realize I didn't make the same comment for GhcbIsRegValid(), so > I'm taking back the above -- consistency with GhcbIsRegValid() is more > important. No change needed here. > > Acked-by: Laszlo Ersek Wow, I'm needing *a lot* of time for getting back into this. Sorry about that. :/ So, as I'm slowly grasping the idea behind this series (--> wherever we make a VmgExit() call, having populated some fields in the GHCB, make sure the "valid bitmap" is set correctly too), it's becoming clear that we need a new "VmgExitLib.h" API. Because, (a) VmgExit() is declared in that lib class header anyway, and the new helper function effectively helps us set up the (thick) parameters for that call, (b) at the end of this v1 series, we have the "valid bitmap" setting logic triplicated (not counting the assembly code logic in patch #5): - GhcbSetRegValid() -- existent function in "OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c" - GhcbSetOffsetValid() -- function basically identical to GhcbSetRegValid(), added in this patch to "OvmfPkg/Library/VmgExitLib/VmgExitLib.c". (This means that the same library instance will have two copies of the same logic.) - QemuFlashPtrWrite() -- from patch #6 So I think we should first replace the GhcbSetRegValid() function with a public (UefiCpuPkg VmgExitLib) API called VmgSetOffsetValid(). This would likely take two patches -- first patch, introduce the API and add the VmgExitLibNull implementation under UefiCpuPkg; second patch, add the real implementation, replacing GhcbSetRegValid(), under OvmfPkg. Then, in the rest of the series, call VmgSetOffsetValid() wherever needed (in C code, of course; in assembly, the open-coded stuff is OK I guess). Thanks, Laszlo