public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Laszlo Ersek" <lersek@redhat.com>
To: Tom Lendacky <thomas.lendacky@amd.com>, devel@edk2.groups.io
Cc: Brijesh Singh <brijesh.singh@amd.com>,
	Jordan Justen <jordan.l.justen@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT
Date: Thu, 15 Oct 2020 11:19:02 +0200	[thread overview]
Message-ID: <78000bae-da10-970c-af49-0e484493f47c@redhat.com> (raw)
In-Reply-To: <71b855ac-8381-4767-798c-2fafd2b9bd56@redhat.com>

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 <thomas.lendacky@amd.com>
>>>
>>> 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 <jordan.l.justen@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>>> ---
>>>  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 <lersek@redhat.com>
> 
> ... 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 <lersek@redhat.com>

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


  reply	other threads:[~2020-10-15  9:19 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-10 16:06 [PATCH 0/9] SEV-ES guest support fixes and cleanup Lendacky, Thomas
2020-10-10 16:06 ` [PATCH 1/9] OvmfPkg/VmgExitLib: Update ValidBitmap settings Lendacky, Thomas
2020-10-15  8:40   ` Laszlo Ersek
2020-10-15 13:37     ` Lendacky, Thomas
2020-10-10 16:07 ` [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT Lendacky, Thomas
2020-10-15  8:47   ` Laszlo Ersek
2020-10-15  8:50     ` Laszlo Ersek
2020-10-15  9:19       ` Laszlo Ersek [this message]
2020-10-15 14:10         ` Lendacky, Thomas
2020-10-15 14:33           ` Lendacky, Thomas
2020-10-15 16:26             ` Laszlo Ersek
2020-10-15 15:27     ` Lendacky, Thomas
2020-10-15 16:28       ` Laszlo Ersek
2020-10-10 16:07 ` [PATCH 3/9] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for IOIO events Lendacky, Thomas
2020-10-15  8:47   ` Laszlo Ersek
2020-10-10 16:07 ` [PATCH 4/9] OvmfPkg/VmgExitLib: Set the SwScratch valid bit for MMIO events Lendacky, Thomas
2020-10-15  8:52   ` Laszlo Ersek
2020-10-10 16:07 ` [PATCH 5/9] UefiCpuPkg/MpInitLib: Set the SW exit fields when performing VMGEXIT Lendacky, Thomas
2020-10-12  5:11   ` Ni, Ray
2020-10-10 16:07 ` [PATCH 6/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Set the SwScratch valid bit Lendacky, Thomas
2020-10-15  9:25   ` Laszlo Ersek
2020-10-10 16:07 ` [PATCH 7/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Fix erase blocks for SEV-ES Lendacky, Thomas
2020-10-15  9:27   ` Laszlo Ersek
2020-10-10 16:07 ` [PATCH 8/9] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Disable interrupts when using GHCB Lendacky, Thomas
2020-10-15  9:45   ` Laszlo Ersek
2020-10-15 17:39     ` Lendacky, Thomas
2020-10-10 16:07 ` [PATCH 9/9] UefiCpuPkg/MpInitLib: For SEV-ES guest set stack based on processor number Lendacky, Thomas
2020-10-12  5:11   ` Ni, Ray
2020-10-15  9:49   ` Laszlo Ersek
2020-10-15  7:43 ` [PATCH 0/9] SEV-ES guest support fixes and cleanup Laszlo Ersek
2020-10-15 13:26   ` Lendacky, Thomas
2020-10-15 16:20     ` Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=78000bae-da10-970c-af49-0e484493f47c@redhat.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox