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.web10.16577.1602779203262394134 for ; Thu, 15 Oct 2020 09:26:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Y/00r7LV; 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=1602779202; 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=6o3Hsme5l9THEOr++oo695NcH8F2OSXiFipKJ2Xx3kw=; b=Y/00r7LVbavoTp+fXTA9PjBZb0/lkkHrMouZzE+EcgpTNZKb8U+hgEuaZ/fc5TEtacnTuc +4rVIwglerDCvPPx8nVZ0s9Pc9Uh9gP4czcKCWoFqQIu09YFtKRZqFIz52xs40b4SZNUas e2x6laigKdacPHUHzXNPUfiSHe19qKA= 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-227-9iIspOC2ORG3HF3VAmTSxQ-1; Thu, 15 Oct 2020 12:26:38 -0400 X-MC-Unique: 9iIspOC2ORG3HF3VAmTSxQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 03FCA1800D42; Thu, 15 Oct 2020 16:26:37 +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 8CDD919C4F; Thu, 15 Oct 2020 16:26:35 +0000 (UTC) Subject: Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT 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> <78000bae-da10-970c-af49-0e484493f47c@redhat.com> <74720266-1b2a-8be1-2a3f-e0e9ee0e63ed@amd.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 15 Oct 2020 18:26:34 +0200 MIME-Version: 1.0 In-Reply-To: <74720266-1b2a-8be1-2a3f-e0e9ee0e63ed@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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: 8bit On 10/15/20 16:33, Tom Lendacky wrote: > On 10/15/20 9:10 AM, Tom Lendacky wrote: >> On 10/15/20 4:19 AM, Laszlo Ersek wrote: >>> 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). >> >> Yup, I was toying with the idea of adding a new function to the >> library, too. I'll do that, using the approach you outlined. > > Also, I'll add an interface, VmgIsOffsetValid(), so that all the > ValidBitmap manipulation and examination is contained in the library. > I'll replace the GhcbIsRegValid() implementation with calls to this > interface. Sounds good! Thanks Laszlo