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

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

On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> 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.)

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