Re: [edk2-devel] [PATCH v8 18/46] OvmfPkg/VmgExitLib: Add support for WBINVD NAE events

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

On 5/22/20 9:19 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cfc8d0918a2474d08139808d7fe5b2a93%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257539824804827&sdata=aiIeMfwp9GQCrgqOw2si6NzLZOZzRRV%2Bzu3dpT6S%2F5k%3D&reserved=0
>>
>> Under SEV-ES, a WBINVD intercept generates a #VC exception. VMGEXIT must be
>> used to allow the hypervisor to handle this intercept.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 36 +++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> index 50199845ceef..0cf26c052b32 100644
>> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
>> @@ -843,6 +843,38 @@ MmioExit (
>>     return Status;
>>   }
>>   
>> +/**
>> +  Handle a WBINVD event.
>> +
>> +  Use the VMGEXIT instruction to handle a WBINVD event.
>> +
>> +  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor Communication
>> +                                   Block
>> +  @param[in, out] Regs             x64 processor context
>> +  @param[in]      InstructionData  Instruction parsing context
>> +
>> +  @retval 0                        Event handled successfully
>> +  @retval Others                   New exception value to propagate
>> +
>> +**/
>> +STATIC
>> +UINT64
>> +WbinvdExit (
>> +  IN OUT GHCB                     *Ghcb,
>> +  IN OUT EFI_SYSTEM_CONTEXT_X64   *Regs,
>> +  IN     SEV_ES_INSTRUCTION_DATA  *InstructionData
>> +  )
>> +{
>> +  UINT64  Status;
>> +
>> +  Status = VmgExit (Ghcb, SVM_EXIT_WBINVD, 0, 0);
>> +  if (Status) {
>> +    return Status;
>> +  }
>> +
>> +  return 0;
> 
> (1) I'd simplify:
> 
>    return VmgExit (Ghcb, SVM_EXIT_WBINVD, 0, 0);
> 
> unless you want to keep the Status variable (maybe for inserting DEBUG
> statements more easily, if needed). Even in that case:
> 
>    Status = VmgExit (Ghcb, SVM_EXIT_WBINVD, 0, 0);
>    return Status;
> 
> With either option adopted:
> 
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> 
> (The real wart I'm trying to address here is the evaluation of "Status"
> in a logical context, as usual.)

Yup, I'm going through all of the patches.

Thanks!
Tom

> 
> Thanks,
> Laszlo
> 
>> +}
>> +
>>   /**
>>     Handle an MSR event.
>>   
>> @@ -1238,6 +1270,10 @@ VmgExitHandleVc (
>>       NaeExit = MsrExit;
>>       break;
>>   
>> +  case SVM_EXIT_WBINVD:
>> +    NaeExit = WbinvdExit;
>> +    break;
>> +
>>     case SVM_EXIT_NPF:
>>       NaeExit = MmioExit;
>>       break;
>>
>