Thanks for the comments. It will be updated in the next version. > -----Original Message----- > From: Yao, Jiewen <[email protected]> > Sent: Friday, January 6, 2023 4:51 PM > To: Xu, Min M <[email protected]>; [email protected] > Cc: Aktas, Erdem <[email protected]>; James Bottomley > <[email protected]>; Gerd Hoffmann <[email protected]>; Tom > Lendacky <[email protected]>; Ryan Afranji <[email protected]> > Subject: RE: [PATCH V1 2/2] OvmfPkg/CcExitLib: Refactor TDX MmioExit > > Thanks for the cleanup. > Comments inline. > > In general, we need ensure CpuDeadLoop() for *any* parsing error in VE > handler. Just ASSERT or VmCall(HALT) is not enough, because ASSERT == > nothing in release, while VmCall(HALT) is not trusted. > > ASSERT can only be used to handle internal impossible logic, but not input > from VMM. > > Thank you > Yao, Jiewen > > > -----Original Message----- > > From: Xu, Min M <[email protected]> > > Sent: Thursday, December 29, 2022 4:56 PM > > To: [email protected] > > Cc: Xu, Min M <[email protected]>; Aktas, Erdem > > <[email protected]>; James Bottomley <[email protected]>; Yao, > > Jiewen <[email protected]>; Gerd Hoffmann <[email protected]>; > Tom > > Lendacky <[email protected]>; Ryan Afranji > <[email protected]> > > Subject: [PATCH V1 2/2] OvmfPkg/CcExitLib: Refactor TDX MmioExit > > > > From: Min M Xu <[email protected]> > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4169 > > > > The previous TDX MmioExit doesn't handle the Mmio instructions > > correctly in some scenarios. This patch refactors the implementation > > to fix the issues. > > > > Cc: Erdem Aktas <[email protected]> > > Cc: James Bottomley <[email protected]> > > Cc: Jiewen Yao <[email protected]> > > Cc: Gerd Hoffmann <[email protected]> > > Cc: Tom Lendacky <[email protected]> > > Cc: Ryan Afranji <[email protected]> > > Reported-by: Ryan Afranji <[email protected]> > > Signed-off-by: Min Xu <[email protected]> > > --- > > OvmfPkg/Library/CcExitLib/CcExitVeHandler.c | 498 > > ++++++++++++++------ > > 1 file changed, 347 insertions(+), 151 deletions(-) > > > > diff --git a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c > > b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c > > index 30d547d5fe55..e0998722af39 100644 > > --- a/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c > > +++ b/OvmfPkg/Library/CcExitLib/CcExitVeHandler.c > > @@ -13,6 +13,10 @@ > > #include <Library/BaseMemoryLib.h> > > #include <IndustryStandard/Tdx.h> > > #include <IndustryStandard/InstructionParsing.h> > > +#include "CcInstruction.h" > > + > > +#define TDX_MMIO_READ 0 > > +#define TDX_MMIO_WRITE 1 > > > > typedef union { > > struct { > > @@ -216,14 +220,15 @@ STATIC > > VOID > > EFIAPI > > TdxDecodeInstruction ( > > - IN UINT8 *Rip > > + IN UINT8 *Rip, > > + IN UINT32 Length > > ) > > { > > UINTN i; > > > > DEBUG ((DEBUG_INFO, "TDX: #TD[EPT] instruction (%p):", Rip)); > > - for (i = 0; i < 15; i++) { > > - DEBUG ((DEBUG_INFO, "%02x:", Rip[i])); > > + for (i = 0; i < MIN (15, Length); i++) { > > + DEBUG ((DEBUG_INFO, "%02x ", Rip[i])); > > } > > > > DEBUG ((DEBUG_INFO, "\n")); > > @@ -235,50 +240,324 @@ TdxDecodeInstruction ( > > TdVmCall(TDVMCALL_HALT, 0, 0, 0, 0, 0); \ > > [Jiewen] Please put CpuDeadLoop() here. > > > } > > > > +/** > > + * Tdx MMIO access via TdVmcall. > > + * > > + * @param MmioSize Size of the MMIO access > > + * @param ReadOrWrite Read or write operation > > + * @param GuestPA Guest physical address > > + * @param Val Pointer to the value which is read or written > > + > > + * @retval EFI_SUCCESS Successfully access the mmio > > + * @retval Others Other errors as indicated > > + */ > > STATIC > > -UINT64 * > > -EFIAPI > > -GetRegFromContext ( > > - IN EFI_SYSTEM_CONTEXT_X64 *Regs, > > - IN UINTN RegIndex > > +UINT64 > [Jiewen] According to the return code in the function, I think we need use > EFI_STATUS here. > > > > +TdxMmioReadWrite ( > > + IN UINT32 MmioSize, > > + IN UINT32 ReadOrWrite, > > + IN UINT64 GuestPA, > > + IN UINT64 *Val > > ) > > { > > - switch (RegIndex) { > > - case 0: return &Regs->Rax; > > - break; > > - case 1: return &Regs->Rcx; > > - break; > > - case 2: return &Regs->Rdx; > > - break; > > - case 3: return &Regs->Rbx; > > - break; > > - case 4: return &Regs->Rsp; > > - break; > > - case 5: return &Regs->Rbp; > > - break; > > - case 6: return &Regs->Rsi; > > - break; > > - case 7: return &Regs->Rdi; > > - break; > > - case 8: return &Regs->R8; > > - break; > > - case 9: return &Regs->R9; > > + UINT64 Status; > > + > > + if ((MmioSize != 1) && (MmioSize != 2) && (MmioSize != 4) && > > (MmioSize != 8)) { > > + ASSERT (FALSE); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (Val == NULL) { > > + ASSERT (FALSE); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (ReadOrWrite == TDX_MMIO_READ) { > > + Status = TdVmCall (TDVMCALL_MMIO, MmioSize, TDX_MMIO_READ, > > GuestPA, 0, Val); > > + } else if (ReadOrWrite == TDX_MMIO_WRITE) { > > + Status = TdVmCall (TDVMCALL_MMIO, MmioSize, TDX_MMIO_WRITE, > > GuestPA, *Val, 0); > > + } else { > > + ASSERT (FALSE); > > + Status = EFI_INVALID_PARAMETER; > > + } > > + > > + return Status; > > +} > > [Jiewen] I checked error state. > I think we need add CpuDeadLoo() after TdVmCall(TDVMCALL_HALT) below: > > if (Status) { > DEBUG (( > DEBUG_ERROR, > "#VE Error (0x%llx) returned from host, ExitReason is %d, > ExitQualification > = 0x%x.\n", > Status, > ReturnData.VeInfo.ExitReason, > ReturnData.VeInfo.ExitQualification.Val > )); > > TdVmCall (TDVMCALL_HALT, 0, 0, 0, 0, 0); > } > > In general, please CpuDeadLoop() for all TDVMCALL_HALT in CcExitHandleVe() > > > > + > > +typedef struct { > > + UINT8 OpCode; > > + UINT32 Bytes; > > + EFI_PHYSICAL_ADDRESS Address; > > + UINT64 Val; > > + UINT64 *Register; > > + UINT32 ReadOrWrite; > > +} MMIO_EXIT_PARSED_INSTRUCTION; > > + > > +/** > > + * Parse the MMIO instructions. > > + * > > + * @param Regs Pointer to the EFI_SYSTEM_CONTEXT_X64 which > > includes the instructions > > + * @param InstructionData Pointer to the CC_INSTRUCTION_DATA > > + * @param ParsedInstruction Pointer to the parsed instruction data > > + * > > + * @retval EFI_SUCCESS Successfully parsed the instructions > > + * @retval Others Other error as indicated > > + */ > > +STATIC > > +EFI_STATUS > > +ParseMmioExitInstructions ( > > + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs, > > + IN OUT CC_INSTRUCTION_DATA *InstructionData, > > + OUT MMIO_EXIT_PARSED_INSTRUCTION *ParsedInstruction > > + ) > > +{ > > + EFI_STATUS Status; > > + UINT8 OpCode; > > + UINT8 SignByte; > > + UINT32 Bytes; > > + EFI_PHYSICAL_ADDRESS Address; > > + UINT64 Val; > > + UINT64 *Register; > > + UINT32 ReadOrWrite; > > + > > + Address = 0; > > + Bytes = 0; > > + Register = NULL; > > + Status = EFI_SUCCESS; > > + Val = 0; > > + > > + Status = CcInitInstructionData (InstructionData, NULL, Regs); if > > + (Status != EFI_SUCCESS) { > > + DEBUG ((DEBUG_ERROR, "%a: Initialize InstructionData failed! > > + (%r)\n", > > __FUNCTION__, Status)); > > + ASSERT (FALSE); > > + return Status; > > + } > > + > > + 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: > > + CcDecodeModRm (Regs, InstructionData); > > + Bytes = ((Bytes != 0) ? Bytes : > > + (InstructionData->DataSize == Size16Bits) ? 2 : > > + (InstructionData->DataSize == Size32Bits) ? 4 : > > + (InstructionData->DataSize == Size64Bits) ? 8 : > > + 0); > > + > > + if (InstructionData->Ext.ModRm.Mod == 3) { > > + DEBUG ((DEBUG_ERROR, "%a: Parse Ext.ModRm.Mod error! (OpCode: > > 0x%x)\n", __FUNCTION__, OpCode)); > > + ASSERT (FALSE); > > + return EFI_UNSUPPORTED; > > + } > > + > > + Address = InstructionData->Ext.RmData; > > + Val = InstructionData->Ext.RegData; > > + ReadOrWrite = TDX_MMIO_WRITE; > > + > > break; > > - case 10: return &Regs->R10; > > + > > + // > > + // MMIO write (MOV moffsetX, aX) > > + // > > + case 0xA2: > > + Bytes = 1; > > + // > > + // fall through > > + // > > + case 0xA3: > > + Bytes = ((Bytes != 0) ? Bytes : > > + (InstructionData->DataSize == Size16Bits) ? 2 : > > + (InstructionData->DataSize == Size32Bits) ? 4 : > > + (InstructionData->DataSize == Size64Bits) ? 8 : > > + 0); > > + > > + InstructionData->ImmediateSize = (UINTN)(1 << InstructionData- > > >AddrSize); > > + InstructionData->End += InstructionData->ImmediateSize; > > + CopyMem (&Address, InstructionData->Immediate, InstructionData- > > >ImmediateSize); > > + > > + Val = Regs->Rax; > > + ReadOrWrite = TDX_MMIO_WRITE; > > break; > > - case 11: return &Regs->R11; > > + > > + // > > + // MMIO write (MOV reg/memX, immX) > > + // > > + case 0xC6: > > + Bytes = 1; > > + // > > + // fall through > > + // > > + case 0xC7: > > + CcDecodeModRm (Regs, InstructionData); > > + Bytes = ((Bytes != 0) ? Bytes : > > + (InstructionData->DataSize == Size16Bits) ? 2 : > > + (InstructionData->DataSize == Size32Bits) ? 4 : > > + (InstructionData->DataSize == Size64Bits) ? 8 : > > + 0); > > + > > + InstructionData->ImmediateSize = Bytes; > > + InstructionData->End += Bytes; > > + > > + Val = 0; > > + CopyMem (&Val, InstructionData->Immediate, InstructionData- > > >ImmediateSize); > > + > > + Address = InstructionData->Ext.RmData; > > + ReadOrWrite = TDX_MMIO_WRITE; > > + > > break; > > - case 12: return &Regs->R12; > > + > > + // > > + // MMIO read (MOV regX, reg/memX) > > + // > > + case 0x8A: > > + Bytes = 1; > > + // > > + // fall through > > + // > > + case 0x8B: > > + CcDecodeModRm (Regs, InstructionData); > > + Bytes = ((Bytes != 0) ? Bytes : > > + (InstructionData->DataSize == Size16Bits) ? 2 : > > + (InstructionData->DataSize == Size32Bits) ? 4 : > > + (InstructionData->DataSize == Size64Bits) ? 8 : > > + 0); > > + if (InstructionData->Ext.ModRm.Mod == 3) { > > + // > > + // NPF on two register operands??? > > + // > > + DEBUG ((DEBUG_ERROR, "%a: Parse Ext.ModRm.Mod error! (OpCode: > > 0x%x)\n", __FUNCTION__, OpCode)); > > + ASSERT (FALSE); > > [Jiewen] I am not sure if it is useful to put ASSERT here. > If this is possible case, but we don't want to support, just return > UNSUPPORTED without ASSERT. > It will cause CpuDeadLoop() later anyway. > > > > + return EFI_UNSUPPORTED; > > + } > > + > > + Address = InstructionData->Ext.RmData; > > + ReadOrWrite = TDX_MMIO_READ; > > + > > + Register = CcGetRegisterPointer (Regs, InstructionData- > >Ext.ModRm.Reg); > > + if (Bytes == 4) { > > + // > > + // Zero-extend for 32-bit operation > > + // > > + *Register = 0; > > + } > > + > > break; > > - case 13: return &Regs->R13; > > + > > + // > > + // MMIO read (MOV aX, moffsetX) > > + // > > + case 0xA0: > > + Bytes = 1; > > + // > > + // fall through > > + // > > + case 0xA1: > > + Bytes = ((Bytes != 0) ? Bytes : > > + (InstructionData->DataSize == Size16Bits) ? 2 : > > + (InstructionData->DataSize == Size32Bits) ? 4 : > > + (InstructionData->DataSize == Size64Bits) ? 8 : > > + 0); > > + > > + InstructionData->ImmediateSize = (UINTN)(1 << InstructionData- > > >AddrSize); > > + InstructionData->End += InstructionData->ImmediateSize; > > + > > + Address = 0; > > + CopyMem ( > > + &Address, > > + InstructionData->Immediate, > > + InstructionData->ImmediateSize > > + ); > > + > > + if (Bytes == 4) { > > + // > > + // Zero-extend for 32-bit operation > > + // > > + Regs->Rax = 0; > > + } > > + > > + Register = &Regs->Rax; > > + ReadOrWrite = TDX_MMIO_READ; > > + > > break; > > - case 14: return &Regs->R14; > > + > > + // > > + // MMIO read w/ zero-extension ((MOVZX regX, reg/memX) > > + // > > + case 0xB6: > > + Bytes = 1; > > + // > > + // fall through > > + // > > + case 0xB7: > > + CcDecodeModRm (Regs, InstructionData); > > + Bytes = (Bytes != 0) ? Bytes : 2; > > + Address = InstructionData->Ext.RmData; > > + > > + Register = CcGetRegisterPointer (Regs, InstructionData- > >Ext.ModRm.Reg); > > + SetMem (Register, (UINTN)(1 << InstructionData->DataSize), 0); > > + > > + ReadOrWrite = TDX_MMIO_READ; > > + > > break; > > - case 15: return &Regs->R15; > > + > > + // > > + // MMIO read w/ sign-extension (MOVSX regX, reg/memX) > > + // > > + case 0xBE: > > + Bytes = 1; > > + // > > + // fall through > > + // > > + case 0xBF: > > + CcDecodeModRm (Regs, InstructionData); > > + Bytes = (Bytes != 0) ? Bytes : 2; > > + > > + Address = InstructionData->Ext.RmData; > > + > > + if (Bytes == 1) { > > + UINT8 *Data; > > + Data = (UINT8 *)&Val; > > + SignByte = ((*Data & BIT7) != 0) ? 0xFF : 0x00; > > + } else { > > + UINT16 *Data; > > + Data = (UINT16 *)&Val; > > + SignByte = ((*Data & BIT15) != 0) ? 0xFF : 0x00; > > + } > > + > > + Register = CcGetRegisterPointer (Regs, InstructionData- > >Ext.ModRm.Reg); > > + SetMem (Register, (UINTN)(1 << InstructionData->DataSize), > > + SignByte); > > + > > + ReadOrWrite = TDX_MMIO_READ; > > + > > break; > > + > > + default: > > + DEBUG ((DEBUG_ERROR, "%a: Invalid MMIO opcode (%x)\n", > > __FUNCTION__, OpCode)); > > + Status = EFI_UNSUPPORTED; > > + ASSERT (FALSE); > > + } > > + > > + if (!EFI_ERROR (Status)) { > > + ParsedInstruction->OpCode = OpCode; > > + ParsedInstruction->Address = Address; > > + ParsedInstruction->Bytes = Bytes; > > + ParsedInstruction->Register = Register; > > + ParsedInstruction->Val = Val; > > + ParsedInstruction->ReadOrWrite = ReadOrWrite; > > } > > > > - return NULL; > > + return Status; > > } > > > > /** > > @@ -300,25 +579,13 @@ MmioExit ( > > IN TDCALL_VEINFO_RETURN_DATA *Veinfo > > ) > > { > > - UINT64 Status; > > - UINT32 MmioSize; > > - UINT32 RegSize; > > - UINT8 OpCode; > > - BOOLEAN SeenRex; > > - UINT64 *Reg; > > - UINT8 *Rip; > > - UINT64 Val; > > - UINT32 OpSize; > > - MODRM ModRm; > > - REX Rex; > > - TD_RETURN_DATA TdReturnData; > > - UINT8 Gpaw; > > - UINT64 TdSharedPageMask; > > - > > - Rip = (UINT8 *)Regs->Rip; > > - Val = 0; > > - Rex.Val = 0; > > - SeenRex = FALSE; > > + UINT64 Status; > > + TD_RETURN_DATA TdReturnData; > > + UINT8 Gpaw; > > + UINT64 Val; > > + UINT64 TdSharedPageMask; > > + CC_INSTRUCTION_DATA InstructionData; > > + MMIO_EXIT_PARSED_INSTRUCTION ParsedInstruction; > > > > Status = TdCall (TDCALL_TDINFO, 0, 0, 0, &TdReturnData); > > if (Status == TDX_EXIT_REASON_SUCCESS) { @@ -335,112 +602,41 @@ > > MmioExit ( > > CpuDeadLoop (); > > } > > > > - // > > - // Default to 32bit transfer > > - // > > - OpSize = 4; > > + Status = ParseMmioExitInstructions (Regs, &InstructionData, > > &ParsedInstruction); > > + if (Status != EFI_SUCCESS) { > > + return Status; > > + } > > > > - do { > > - OpCode = *Rip++; > > - if (OpCode == 0x66) { > > - OpSize = 2; > > - } else if ((OpCode == 0x64) || (OpCode == 0x65) || (OpCode == 0x67)) { > > - continue; > > - } else if ((OpCode >= 0x40) && (OpCode <= 0x4f)) { > > - SeenRex = TRUE; > > - Rex.Val = OpCode; > > - } else { > > - break; > > + if (Veinfo->GuestPA != (ParsedInstruction.Address | TdSharedPageMask)) > { > > + DEBUG (( > > + DEBUG_ERROR, > > + "%a: Address is not correct! (%d: 0x%llx != 0x%llx)\n", > > + __FUNCTION__, > > + ParsedInstruction.OpCode, > > + Veinfo->GuestPA, > > + ParsedInstruction.Address > > + )); > > + ASSERT (FALSE); > [Jiewen] Not sure if ASSERT here is useful. We should DeadLoop() later. > > > + return EFI_ABORTED; > > + } > > + > > + if (ParsedInstruction.ReadOrWrite == TDX_MMIO_WRITE ) { > > + Status = TdxMmioReadWrite (ParsedInstruction.Bytes, > > TDX_MMIO_WRITE, Veinfo->GuestPA, &ParsedInstruction.Val); > > + } else { > > + Val = 0; > > + Status = TdxMmioReadWrite (ParsedInstruction.Bytes, > > + TDX_MMIO_READ, > > Veinfo->GuestPA, &Val); > > + if (Status == 0) { > > + CopyMem (ParsedInstruction.Register, &Val, > > + ParsedInstruction.Bytes); > > } > > - } while (TRUE); > > - > > - // > > - // We need to have at least 2 more bytes for this instruction > > - // > > - TDX_DECODER_BUG_ON (((UINT64)Rip - Regs->Rip) > 13); > [Jiewen] We need make sure it will DeadLoop() if TDX_DECODER_BUG_ON is > ON. > > > - > > - OpCode = *Rip++; > > - // > > - // Two-byte opecode, get next byte > > - // > > - if (OpCode == 0x0F) { > > - OpCode = *Rip++; > > - } > > - > > - switch (OpCode) { > > - case 0x88: > > - case 0x8A: > > - case 0xB6: > > - MmioSize = 1; > > - break; > > - case 0xB7: > > - MmioSize = 2; > > - break; > > - default: > > - MmioSize = Rex.Bits.W ? 8 : OpSize; > > - break; > > - } > > - > > - /* Punt on AH/BH/CH/DH unless it shows up. */ > > - ModRm.Val = *Rip++; > > - TDX_DECODER_BUG_ON (MmioSize == 1 && ModRm.Bits.Reg > 4 > && !SeenRex > > && OpCode != 0xB6); > > - Reg = GetRegFromContext (Regs, ModRm.Bits.Reg | ((int)Rex.Bits.R << > > 3)); > > - TDX_DECODER_BUG_ON (!Reg); > > - > > - if (ModRm.Bits.Rm == 4) { > > - ++Rip; /* SIB byte */ > > - } > > - > > - if ((ModRm.Bits.Mod == 2) || ((ModRm.Bits.Mod == 0) && > > (ModRm.Bits.Rm == 5))) { > > - Rip += 4; /* DISP32 */ > > - } else if (ModRm.Bits.Mod == 1) { > > - ++Rip; /* DISP8 */ > > - } > > - > > - switch (OpCode) { > > - case 0x88: > > - case 0x89: > > - CopyMem ((void *)&Val, Reg, MmioSize); > > - Status = TdVmCall (TDVMCALL_MMIO, MmioSize, 1, Veinfo->GuestPA, > > Val, 0); > > - break; > > - case 0xC7: > > - CopyMem ((void *)&Val, Rip, OpSize); > > - Status = TdVmCall (TDVMCALL_MMIO, MmioSize, 1, Veinfo->GuestPA, > > Val, 0); > > - Rip += OpSize; > > - default: > > - // > > - // 32-bit write registers are zero extended to the full register > > - // Hence 'MOVZX r[32/64], r/m16' is > > - // hardcoded to reg size 8, and the straight MOV case has a reg > > - // size of 8 in the 32-bit read case. > > - // > > - switch (OpCode) { > > - case 0xB6: > > - RegSize = Rex.Bits.W ? 8 : OpSize; > > - break; > > - case 0xB7: > > - RegSize = 8; > > - break; > > - default: > > - RegSize = MmioSize == 4 ? 8 : MmioSize; > > - break; > > - } > > - > > - Status = TdVmCall (TDVMCALL_MMIO, MmioSize, 0, Veinfo->GuestPA, > 0, > > &Val); > > - if (Status == 0) { > [Jiewen] Status is EFI_STATUS. We need use !EFI_ERROR(Status) > > > > - ZeroMem (Reg, RegSize); > > - CopyMem (Reg, (void *)&Val, MmioSize); > > - } > > } > > > > if (Status == 0) { > [Jiewen] Status is EFI_STATUS. We need use !EFI_ERROR(Status) > > > > - TDX_DECODER_BUG_ON (((UINT64)Rip - Regs->Rip) > 15); > > - > > // > > // We change instruction length to reflect true size so handler can > > // bump rip > > // > > - Veinfo->ExitInstructionLength = (UINT32)((UINT64)Rip - Regs->Rip); > > + Veinfo->ExitInstructionLength = (UINT32)(CcInstructionLength > > (&InstructionData)); > > + TdxDecodeInstruction ((UINT8 *)Regs->Rip, Veinfo- > > >ExitInstructionLength); > > } > > > > return Status; > > -- > > 2.29.2.windows.2
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98158): https://edk2.groups.io/g/devel/message/98158 Mute This Topic: https://groups.io/mt/95934165/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
