On 6/10/20 7:26 AM, Laszlo Ersek wrote: > 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.jus...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheu...@arm.com> >> Acked-by: Laszlo Ersek <ler...@redhat.com> >> Signed-off-by: Tom Lendacky <thomas.lenda...@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.
Will do. > >> 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: > > > https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmid.mail-archive.com%2F23c7dfb3-56ac-b080-2736-687cd11d51a0%40redhat.com&data=02%7C01%7Cthomas.lendacky%40amd.com%7C23a76c28475f4b34b57208d80d39989b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637273888322294989&sdata=bgpq7w3kRE4ZVgnu9vgSDxvbvrHmBUio3urYWP%2B5qWE%3D&reserved=0 > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F60047&data=02%7C01%7Cthomas.lendacky%40amd.com%7C23a76c28475f4b34b57208d80d39989b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637273888322294989&sdata=i7lzz2cYm3bdyC06bLBqwOguzMi%2BPHSeJV%2BU%2BzDqSOM%3D&reserved=0 > > 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. Ok, I've made the changes throughout the series in the OvmfPkg library and in the UefiCpuPkg NULL library. If there's a v10, then they'll be incorporated. Thanks, Tom > > 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; >> +} >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#61080): https://edk2.groups.io/g/devel/message/61080 Mute This Topic: https://groups.io/mt/74692417/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
