Re: [PATCH 2/9] OvmfPkg/VmgExitLib: Set the SW exit fields when performing VMGEXIT

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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 <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).

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.

Thanks,
Tom

> 
> Thanks,
> Laszlo
>