[edk2-devel] [PATCH v9 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO)
Lendacky, Thomas
thomas.lendacky at amd.com
Thu Jun 11 15:09:54 UTC 2020
On 6/11/20 3:30 AM, Laszlo Ersek wrote:
> On 06/05/20 15:27, Tom Lendacky 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%7C54ee9d68728e43f1e0b608d80de1bb4c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637274610447371952&sdata=4c9uyhXu269S2d9EPt%2Bt45eNMVipU8YDicGK24LOtUk%3D&reserved=0
>>
>> 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.justen at intel.com>
>> Cc: Laszlo Ersek <lersek at redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky at 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.
This is what happens when I switch back and forth between coding
standards, sorry. If there's a v10, this and the ones below will be fixed.
Thanks!
Tom
>
>> + 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 <lersek at 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 (#61163): https://edk2.groups.io/g/devel/message/61163
Mute This Topic: https://groups.io/mt/74692427/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list