Re: [PATCH v9 12/46] OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF

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

Hi Tom,

On 06/05/20 15:27, Tom Lendacky wrote:
> The base VmgExitLib library provides a default limited interface. As it
> does not provide full support, create an OVMF version of this library to
> begin the process of providing full support of SEV-ES within OVMF.
>
> SEV-ES support is only provided for X64 builds, so only OvmfPkgX64.dsc is
> updated to make use of the OvmfPkg version of the library.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkgX64.dsc                        |   2 +-
>  OvmfPkg/Library/VmgExitLib/VmgExitLib.inf     |  36 +++++
>  OvmfPkg/Library/VmgExitLib/VmgExitLib.c       | 159 ++++++++++++++++++++
>  OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c |  81 ++++++++++
>  4 files changed, 277 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 0b9189ab1e38..b5f3859420d0 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -232,7 +232,7 @@ [LibraryClasses]
>
>  [LibraryClasses.common]
>    BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf
> -  VmgExitLib|UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.inf
> +  VmgExitLib|OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
>
>  [LibraryClasses.common.SEC]
>    TimerLib|OvmfPkg/Library/AcpiTimerLib/BaseRomAcpiTimerLib.inf
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
> new file mode 100644
> index 000000000000..8acdee2349b4
> --- /dev/null
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.inf
> @@ -0,0 +1,36 @@
> +## @file
> +#  VMGEXIT Support Library.
> +#
> +#  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = VmgExitLib
> +  FILE_GUID                      = 0e923c25-13cd-430b-8714-ffe85652a97b
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = VmgExitLib
> +
> +#
> +# The following information is for reference only and not required by the build tools.
> +#
> +#  VALID_ARCHITECTURES           = X64
> +#
> +
> +[Sources.common]
> +  VmgExitLib.c
> +  VmgExitVcHandler.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  UefiCpuPkg/UefiCpuPkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  BaseMemoryLib
> +

(1) Please keep the individual sections sorted alphabetically.

> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitLib.c b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> new file mode 100644
> index 000000000000..0ca164a33eb4
> --- /dev/null
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitLib.c
> @@ -0,0 +1,159 @@
> +/** @file
> +  VMGEXIT Support Library.
> +
> +  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/VmgExitLib.h>
> +#include <Register/Amd/Msr.h>
> +
> +/**
> +  Check for VMGEXIT error
> +
> +  Check if the hypervisor has returned an error after completion of the VMGEXIT
> +  by examining the SwExitInfo1 field of the GHCB.
> +
> +  @param[in]  Ghcb       A pointer to the GHCB
> +
> +  @return  0             VMGEXIT succeeded.
> +  @return  Others        VMGEXIT processing did not succeed. Exception number to
> +                         be propagated.

(2) You misunderstood my @return / @retval explanation from v8:

  http://mid.mail-archive.com/23c7dfb3-56ac-b080-2736-687cd11d51a0@redhat.com
  https://edk2.groups.io/g/devel/message/60047

We have two formats.

(2a) "@return  Natural language explanation"

In this format, there is *NO CONSTANT* in the documentation. The comment
only explains in natural language what the function returns. In the
message linked above, I brought the example:

  @return  IOIO event information values.

That was *not* a typo.

(2b) The other format is "@retval CONSTANT  Natural language
explanation".

In this case, the exact return value is given, and then explained.


In the v8 review, I mentioned that "@retval Others" is wrong, even
though it is used in several places in the edk2 tree. That's because
"Others" is not a constant. The right way to put "@retval Others" is to
replace it with *just* "@return". In other words, *drop* the "Others"
non-constant, replace "@retval" with "@return", and reformulate the
natural language text.

In the v8 review, I also mentioned that "@retval Natural language
explanation" is also wrong, because it's either missing a constant, or
it should use "@return".

Unfortunately, in response, you seem to have replaced

  @retval  0
  @retval  Others

with

  @return  0
  @return  Others

(i.e., just replaced "@retval" with "@return"). And that update is
wrong, on *both* lines:

- "@retval 0" was correct. It should not have been updated.

- "@retval  Others" was indeed wrong, but "@return  Others" is *also*
wrong. It should be just "@return", not "@return Others".

Specifically, the right comment would be:

  @retval 0  VMGEXIT succeeded.

  @return    Exception number to be propagated. VMGEXIT processing did
             not succeed.

If you read it aloud, it will make sense.

Anyway, I'm exhausted. We can address this in a follow-up series, if we
want to. Keep the ACK, and no v10 is needed just for these comments.

Thanks
Laszlo


> +
> +**/
> +STATIC
> +UINT64
> +VmgExitErrorCheck (
> +  IN GHCB                *Ghcb
> +  )
> +{
> +  GHCB_EVENT_INJECTION  Event;
> +  GHCB_EXIT_INFO        ExitInfo;
> +  UINT64                Status;
> +
> +  ExitInfo.Uint64 = Ghcb->SaveArea.SwExitInfo1;
> +  ASSERT ((ExitInfo.Elements.Lower32Bits == 0) ||
> +          (ExitInfo.Elements.Lower32Bits == 1));
> +
> +  Status = 0;
> +  if (ExitInfo.Elements.Lower32Bits == 0) {
> +    return Status;
> +  }
> +
> +  if (ExitInfo.Elements.Lower32Bits == 1) {
> +    ASSERT (Ghcb->SaveArea.SwExitInfo2 != 0);
> +
> +    //
> +    // Check that the return event is valid
> +    //
> +    Event.Uint64 = Ghcb->SaveArea.SwExitInfo2;
> +    if (Event.Elements.Valid &&
> +        Event.Elements.Type == GHCB_EVENT_INJECTION_TYPE_EXCEPTION) {
> +      switch (Event.Elements.Vector) {
> +      case GP_EXCEPTION:
> +      case UD_EXCEPTION:
> +        //
> +        // Use returned event as return code
> +        //
> +        Status = Event.Uint64;
> +      }
> +    }
> +  }
> +
> +  if (Status == 0) {
> +    GHCB_EVENT_INJECTION  GpEvent;
> +
> +    GpEvent.Uint64 = 0;
> +    GpEvent.Elements.Vector = GP_EXCEPTION;
> +    GpEvent.Elements.Type   = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
> +    GpEvent.Elements.Valid  = 1;
> +
> +    Status = GpEvent.Uint64;
> +  }
> +
> +  return Status;
> +}
> +
> +/**
> +  Perform VMGEXIT.
> +
> +  Sets the necessary fields of the GHCB, invokes the VMGEXIT instruction and
> +  then handles the return actions.
> +
> +  @param[in, out]  Ghcb       A pointer to the GHCB
> +  @param[in]       ExitCode   VMGEXIT code to be assigned to the SwExitCode
> +                              field of the GHCB.
> +  @param[in]       ExitInfo1  VMGEXIT information to be assigned to the
> +                              SwExitInfo1 field of the GHCB.
> +  @param[in]       ExitInfo2  VMGEXIT information to be assigned to the
> +                              SwExitInfo2 field of the GHCB.
> +
> +  @return  0                  VMGEXIT succeeded.
> +  @return  Others             VMGEXIT processing did not succeed. Exception
> +                              event to be propagated.
> +
> +**/
> +UINT64
> +EFIAPI
> +VmgExit (
> +  IN OUT GHCB                *Ghcb,
> +  IN     UINT64              ExitCode,
> +  IN     UINT64              ExitInfo1,
> +  IN     UINT64              ExitInfo2
> +  )
> +{
> +  Ghcb->SaveArea.SwExitCode = ExitCode;
> +  Ghcb->SaveArea.SwExitInfo1 = ExitInfo1;
> +  Ghcb->SaveArea.SwExitInfo2 = ExitInfo2;
> +
> +  //
> +  // Guest memory is used for the guest-hypervisor communication, so fence
> +  // the invocation of the VMGEXIT instruction to ensure GHCB accesses are
> +  // synchronized properly.
> +  //
> +  MemoryFence ();
> +  AsmVmgExit ();
> +  MemoryFence ();
> +
> +  return VmgExitErrorCheck (Ghcb);
> +}
> +
> +/**
> +  Perform pre-VMGEXIT initialization/preparation.
> +
> +  Performs the necessary steps in preparation for invoking VMGEXIT. Must be
> +  called before setting any fields within the GHCB.
> +
> +  @param[in, out]  Ghcb       A pointer to the GHCB
> +
> +**/
> +VOID
> +EFIAPI
> +VmgInit (
> +  IN OUT GHCB                *Ghcb
> +  )
> +{
> +  SetMem (&Ghcb->SaveArea, sizeof (Ghcb->SaveArea), 0);
> +}
> +
> +/**
> +  Perform post-VMGEXIT cleanup.
> +
> +  Performs the necessary steps to cleanup after invoking VMGEXIT. Must be
> +  called after obtaining needed fields within the GHCB.
> +
> +  @param[in, out]  Ghcb       A pointer to the GHCB
> +
> +**/
> +VOID
> +EFIAPI
> +VmgDone (
> +  IN OUT GHCB                *Ghcb
> +  )
> +{
> +}
> +
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> new file mode 100644
> index 000000000000..b6a955ed8088
> --- /dev/null
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -0,0 +1,81 @@
> +/** @file
> +  X64 #VC Exception Handler functon.
> +
> +  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Uefi.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/VmgExitLib.h>
> +#include <Register/Amd/Msr.h>
> +
> +/**
> +  Handle a #VC exception.
> +
> +  Performs the necessary processing to handle a #VC exception.
> +
> +  @param[in, out]  ExceptionType  Pointer to an EFI_EXCEPTION_TYPE to be set
> +                                  as value to use on error.
> +  @param[in, out]  SystemContext  Pointer to EFI_SYSTEM_CONTEXT
> +
> +  @retval  EFI_SUCCESS            Exception handled
> +  @retval  EFI_UNSUPPORTED        #VC not supported, (new) exception value to
> +                                  propagate provided
> +  @retval  EFI_PROTOCOL_ERROR     #VC handling failed, (new) exception value to
> +                                  propagate provided
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +VmgExitHandleVc (
> +  IN OUT EFI_EXCEPTION_TYPE  *ExceptionType,
> +  IN OUT EFI_SYSTEM_CONTEXT  SystemContext
> +  )
> +{
> +  MSR_SEV_ES_GHCB_REGISTER  Msr;
> +  EFI_SYSTEM_CONTEXT_X64    *Regs;
> +  GHCB                      *Ghcb;
> +  UINT64                    ExitCode, Status;
> +  EFI_STATUS                VcRet;
> +
> +  VcRet = EFI_SUCCESS;
> +
> +  Msr.GhcbPhysicalAddress = AsmReadMsr64 (MSR_SEV_ES_GHCB);
> +  ASSERT (Msr.GhcbInfo.Function == 0);
> +  ASSERT (Msr.Ghcb != 0);
> +
> +  Regs = SystemContext.SystemContextX64;
> +  Ghcb = Msr.Ghcb;
> +
> +  VmgInit (Ghcb);
> +
> +  ExitCode = Regs->ExceptionData;
> +  switch (ExitCode) {
> +  default:
> +    Status = VmgExit (Ghcb, SVM_EXIT_UNSUPPORTED, ExitCode, 0);
> +    if (Status == 0) {
> +      Regs->ExceptionData = 0;
> +      *ExceptionType = GP_EXCEPTION;
> +    } else {
> +      GHCB_EVENT_INJECTION  Event;
> +
> +      Event.Uint64 = Status;
> +      if (Event.Elements.ErrorCodeValid != 0) {
> +        Regs->ExceptionData = Event.Elements.ErrorCode;
> +      } else {
> +        Regs->ExceptionData = 0;
> +      }
> +
> +      *ExceptionType = Event.Elements.Vector;
> +    }
> +
> +    VcRet = EFI_PROTOCOL_ERROR;
> +  }
> +
> +  VmgDone (Ghcb);
> +
> +  return VcRet;
> +}
>