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

["Laszlo Ersek" <lersek@redhat.com>]

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