On 06/05/20 15:27, Tom Lendacky wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> Under SEV-ES, a NPF intercept for an NPT entry with a reserved bit set
> generates a #VC exception. This condition is assumed to be an MMIO access.
> VMGEXIT must be used to allow the hypervisor to handle this intercept.
>
> Add support to construct the required GHCB values to support a NPF NAE
> event for MMIO. Parse the instruction that generated the #VC exception,
> setting the required register values in the GHCB and creating the proper
> SW_EXIT_INFO1, SW_EXITINFO2 and SW_SCRATCH values in the GHCB.
>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
> OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 484 ++++++++++++++++++++
> 1 file changed, 484 insertions(+)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> index 009eb48cd468..c2646d45506a 100644
> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c
> @@ -183,6 +183,279 @@ GhcbSetRegValid (
> Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
> }
>
> +/**
> + Return a pointer to the contents of the specified register.
> +
> + Based upon the input register, return a pointer to the registers contents
> + in the x86 processor context.
> +
> + @param[in] Regs x64 processor context
> + @param[in] Register Register to obtain pointer for
> +
> + @return Pointer to the contents of the requested register
> +
> +**/
> +STATIC
> +UINT64 *
> +GetRegisterPointer (
> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN UINT8 Register
> + )
> +{
> + UINT64 *Reg;
> +
> + switch (Register) {
> + case 0:
> + Reg = &Regs->Rax;
> + break;
> + case 1:
> + Reg = &Regs->Rcx;
> + break;
> + case 2:
> + Reg = &Regs->Rdx;
> + break;
> + case 3:
> + Reg = &Regs->Rbx;
> + break;
> + case 4:
> + Reg = &Regs->Rsp;
> + break;
> + case 5:
> + Reg = &Regs->Rbp;
> + break;
> + case 6:
> + Reg = &Regs->Rsi;
> + break;
> + case 7:
> + Reg = &Regs->Rdi;
> + break;
> + case 8:
> + Reg = &Regs->R8;
> + break;
> + case 9:
> + Reg = &Regs->R9;
> + break;
> + case 10:
> + Reg = &Regs->R10;
> + break;
> + case 11:
> + Reg = &Regs->R11;
> + break;
> + case 12:
> + Reg = &Regs->R12;
> + break;
> + case 13:
> + Reg = &Regs->R13;
> + break;
> + case 14:
> + Reg = &Regs->R14;
> + break;
> + case 15:
> + Reg = &Regs->R15;
> + break;
> + default:
> + Reg = NULL;
> + }
> + ASSERT (Reg != NULL);
> +
> + return Reg;
> +}
> +
> +/**
> + Update the instruction parsing context for displacement bytes.
> +
> + @param[in, out] InstructionData Instruction parsing context
> + @param[in] Size The instruction displacement size
> +
> +**/
> +STATIC
> +VOID
> +UpdateForDisplacement (
> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData,
> + IN UINTN Size
> + )
> +{
> + InstructionData->DisplacementSize = Size;
> + InstructionData->Immediate += Size;
> + InstructionData->End += Size;
> +}
> +
> +/**
> + Determine if an instruction address if RIP relative.
> +
> + Examine the instruction parsing context to determine if the address offset
> + is relative to the instruction pointer.
> +
> + @param[in] InstructionData Instruction parsing context
> +
> + @retval TRUE Instruction addressing is RIP relative
> + @retval FALSE Instruction addressing is not RIP relative
> +
> +**/
> +STATIC
> +BOOLEAN
> +IsRipRelative (
> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> +
> + Ext = &InstructionData->Ext;
> +
> + return ((InstructionData->Mode == LongMode64Bit) &&
> + (Ext->ModRm.Mod == 0) &&
> + (Ext->ModRm.Rm == 5) &&
> + (InstructionData->SibPresent == FALSE));
> +}
> +
> +/**
> + Return the effective address of a memory operand.
> +
> + Examine the instruction parsing context to obtain the effective memory
> + address of a memory operand.
> +
> + @param[in] Regs x64 processor context
> + @param[in] InstructionData Instruction parsing context
> +
> + @return The memory operand effective address
> +
> +**/
> +STATIC
> +UINT64
> +GetEffectiveMemoryAddress (
> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> + UINT64 EffectiveAddress;
> +
> + Ext = &InstructionData->Ext;
> + EffectiveAddress = 0;
> +
> + if (IsRipRelative (InstructionData)) {
> + //
> + // RIP-relative displacement is a 32-bit signed value
> + //
> + INT32 RipRelative;
> +
> + RipRelative = *(INT32 *) InstructionData->Displacement;
> +
> + UpdateForDisplacement (InstructionData, 4);
> +
> + //
> + // Negative displacement is handled by standard UINT64 wrap-around.
> + //
> + return Regs->Rip + (UINT64) RipRelative;
> + }
> +
> + switch (Ext->ModRm.Mod) {
> + case 1:
> + UpdateForDisplacement (InstructionData, 1);
> + EffectiveAddress += (UINT64) (*(INT8 *) (InstructionData->Displacement));
> + break;
> + case 2:
> + switch (InstructionData->AddrSize) {
> + case Size16Bits:
> + UpdateForDisplacement (InstructionData, 2);
> + EffectiveAddress += (UINT64) (*(INT16 *)
> (InstructionData->Displacement));
> + break;
> + default:
> + UpdateForDisplacement (InstructionData, 4);
> + EffectiveAddress += (UINT64) (*(INT32 *)
> (InstructionData->Displacement));
> + break;
> + }
> + break;
> + }
> +
> + if (InstructionData->SibPresent) {
> + INT64 Displacement;
> +
> + if (Ext->Sib.Index != 4) {
> + CopyMem (&Displacement,
> + GetRegisterPointer (Regs, Ext->Sib.Index),
> + sizeof (Displacement));
(1) The indentation is not idiomatic. Please either use the style I
proposed in the v8 review, or the more verbose
CopyMem (
&Displacement,
GetRegisterPointer (Regs, Ext->Sib.Index),
sizeof (Displacement)
);
Anyway, this alone does not justify a v10.
> + Displacement *= (1 << Ext->Sib.Scale);
> +
> + //
> + // Negative displacement is handled by standard UINT64 wrap-around.
> + //
> + EffectiveAddress += (UINT64) Displacement;
> + }
> +
> + if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) {
> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base);
> + } else {
> + UpdateForDisplacement (InstructionData, 4);
> + EffectiveAddress += (UINT64) (*(INT32 *)
> (InstructionData->Displacement));
> + }
> + } else {
> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm);
> + }
> +
> + return EffectiveAddress;
> +}
> +
> +/**
> + Decode a ModRM byte.
> +
> + Examine the instruction parsing context to decode a ModRM byte and the SIB
> + byte, if present.
> +
> + @param[in] Regs x64 processor context
> + @param[in, out] InstructionData Instruction parsing context
> +
> +**/
> +STATIC
> +VOID
> +DecodeModRm (
> + IN EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext;
> + INSTRUCTION_REX_PREFIX *RexPrefix;
> + INSTRUCTION_MODRM *ModRm;
> + INSTRUCTION_SIB *Sib;
> +
> + RexPrefix = &InstructionData->RexPrefix;
> + Ext = &InstructionData->Ext;
> + ModRm = &InstructionData->ModRm;
> + Sib = &InstructionData->Sib;
> +
> + InstructionData->ModRmPresent = TRUE;
> + ModRm->Uint8 = *(InstructionData->End);
> +
> + InstructionData->Displacement++;
> + InstructionData->Immediate++;
> + InstructionData->End++;
> +
> + Ext->ModRm.Mod = ModRm->Bits.Mod;
> + Ext->ModRm.Reg = (RexPrefix->Bits.BitR << 3) | ModRm->Bits.Reg;
> + Ext->ModRm.Rm = (RexPrefix->Bits.BitB << 3) | ModRm->Bits.Rm;
> +
> + Ext->RegData = *GetRegisterPointer (Regs, Ext->ModRm.Reg);
> +
> + if (Ext->ModRm.Mod == 3) {
> + Ext->RmData = *GetRegisterPointer (Regs, Ext->ModRm.Rm);
> + } else {
> + if (ModRm->Bits.Rm == 4) {
> + InstructionData->SibPresent = TRUE;
> + Sib->Uint8 = *(InstructionData->End);
> +
> + InstructionData->Displacement++;
> + InstructionData->Immediate++;
> + InstructionData->End++;
> +
> + Ext->Sib.Scale = Sib->Bits.Scale;
> + Ext->Sib.Index = (RexPrefix->Bits.BitX << 3) | Sib->Bits.Index;
> + Ext->Sib.Base = (RexPrefix->Bits.BitB << 3) | Sib->Bits.Base;
> + }
> +
> + Ext->RmData = GetEffectiveMemoryAddress (Regs, InstructionData);
> + }
> +}
> +
> /**
> Decode instruction prefixes.
>
> @@ -374,6 +647,213 @@ UnsupportedExit (
> return Status;
> }
>
> +/**
> + Handle an MMIO event.
> +
> + Use the VMGEXIT instruction to handle either an MMIO read or an MMIO write.
> +
> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor
> Communication
> + Block
> + @param[in, out] Regs x64 processor context
> + @param[in, out] InstructionData Instruction parsing context
> +
> + @return 0 Event handled successfully
> + @return Others New exception value to propagate
> +
> +**/
> +STATIC
> +UINT64
> +MmioExit (
> + IN OUT GHCB *Ghcb,
> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs,
> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData
> + )
> +{
> + UINT64 ExitInfo1, ExitInfo2, Status;
> + UINTN Bytes;
> + UINT64 *Register;
> + UINT8 OpCode, SignByte;
> +
> + Bytes = 0;
> +
> + OpCode = *(InstructionData->OpCodes);
> + if (OpCode == TWO_BYTE_OPCODE_ESCAPE) {
> + OpCode = *(InstructionData->OpCodes + 1);
> + }
> +
> + switch (OpCode) {
> + //
> + // MMIO write (MOV reg/memX, regX)
> + //
> + case 0x88:
> + Bytes = 1;
> + //
> + // fall through
> + //
> + case 0x89:
> + DecodeModRm (Regs, InstructionData);
> + Bytes = ((Bytes != 0) ? Bytes :
> + (InstructionData->DataSize == Size16Bits) ? 2 :
> + (InstructionData->DataSize == Size32Bits) ? 4 :
> + (InstructionData->DataSize == Size64Bits) ? 8 :
> + 0);
(2) The final argument "0" should be un-indented ("out-dented"?) by 1
space character.
Address it only if a v10 becomes necessary for a more important reason.
> +
> + if (InstructionData->Ext.ModRm.Mod == 3) {
> + //
> + // NPF on two register operands???
> + //
> + return UnsupportedExit (Ghcb, Regs, InstructionData);
> + }
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> + CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes);
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
> + if (Status != 0) {
> + return Status;
> + }
> + break;
> +
> + //
> + // MMIO write (MOV reg/memX, immX)
> + //
> + case 0xC6:
> + Bytes = 1;
> + //
> + // fall through
> + //
> + case 0xC7:
> + DecodeModRm (Regs, InstructionData);
> + Bytes = ((Bytes != 0) ? Bytes :
> + (InstructionData->DataSize == Size16Bits) ? 2 :
> + (InstructionData->DataSize == Size32Bits) ? 4 :
> + 0);
(3) Same as (2). (No need to repost just because of this.)
> +
> + InstructionData->ImmediateSize = Bytes;
> + InstructionData->End += Bytes;
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> + CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes);
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2);
> + if (Status != 0) {
> + return Status;
> + }
> + break;
> +
> + //
> + // MMIO read (MOV regX, reg/memX)
> + //
> + case 0x8A:
> + Bytes = 1;
> + //
> + // fall through
> + //
> + case 0x8B:
> + DecodeModRm (Regs, InstructionData);
> + Bytes = ((Bytes != 0) ? Bytes :
> + (InstructionData->DataSize == Size16Bits) ? 2 :
> + (InstructionData->DataSize == Size32Bits) ? 4 :
> + (InstructionData->DataSize == Size64Bits) ? 8 :
> + 0);
(4) Same as (2). (No need to repost just because of this.)
Thank you very much for the updates in this patch!
Acked-by: Laszlo Ersek <ler...@redhat.com>
Laszlo
> + if (InstructionData->Ext.ModRm.Mod == 3) {
> + //
> + // NPF on two register operands???
> + //
> + return UnsupportedExit (Ghcb, Regs, InstructionData);
> + }
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> + if (Status != 0) {
> + return Status;
> + }
> +
> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> + if (Bytes == 4) {
> + //
> + // Zero-extend for 32-bit operation
> + //
> + *Register = 0;
> + }
> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> + //
> + // MMIO read w/ zero-extension ((MOVZX regX, reg/memX)
> + //
> + case 0xB6:
> + Bytes = 1;
> + //
> + // fall through
> + //
> + case 0xB7:
> + Bytes = (Bytes != 0) ? Bytes : 2;
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> + if (Status != 0) {
> + return Status;
> + }
> +
> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> + SetMem (Register, InstructionData->DataSize, 0);
> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> + //
> + // MMIO read w/ sign-extension (MOVSX regX, reg/memX)
> + //
> + case 0xBE:
> + Bytes = 1;
> + //
> + // fall through
> + //
> + case 0xBF:
> + Bytes = (Bytes != 0) ? Bytes : 2;
> +
> + ExitInfo1 = InstructionData->Ext.RmData;
> + ExitInfo2 = Bytes;
> +
> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer;
> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2);
> + if (Status != 0) {
> + return Status;
> + }
> +
> + if (Bytes == 1) {
> + UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer;
> +
> + SignByte = ((*Data & BIT7) != 0) ? 0xFF : 0x00;
> + } else {
> + UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer;
> +
> + SignByte = ((*Data & BIT15) != 0) ? 0xFF : 0x00;
> + }
> +
> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg);
> + SetMem (Register, InstructionData->DataSize, SignByte);
> + CopyMem (Register, Ghcb->SharedBuffer, Bytes);
> + break;
> +
> + default:
> + Status = GP_EXCEPTION;
> + ASSERT (FALSE);
> + }
> +
> + return Status;
> +}
> +
> /**
> Handle an MSR event.
>
> @@ -770,6 +1250,10 @@ VmgExitHandleVc (
> NaeExit = MsrExit;
> break;
>
> + case SVM_EXIT_NPF:
> + NaeExit = MmioExit;
> + break;
> +
> default:
> NaeExit = UnsupportedExit;
> }
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#61120): https://edk2.groups.io/g/devel/message/61120
Mute This Topic: https://groups.io/mt/74692427/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
