From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.3139.1591864242882283176 for ; Thu, 11 Jun 2020 01:30:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=cBB6Ueji; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1591864242; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=g4jopmiFq6qK/3MVB2VI3jyf/eQ5TStHRyFUVyG+51o=; b=cBB6UejivY9vP/yPhtFRWn4znGul1Wja2PB6p5mtGgE1+aZ8AbXiloU5Y/85SiGbuQj/Ud 6ndTNziEmXpgW1SCagWHp7lAbIbBRn3gAFjhdmmblhIdv/LN0RzTwL/DzF5GAOOpOPKlUy H1ZJOKksKUVvUBhYuYixb7XbIxIKiWg= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-476-PPkl2HYpMgi190DIexNXxg-1; Thu, 11 Jun 2020 04:30:33 -0400 X-MC-Unique: PPkl2HYpMgi190DIexNXxg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CC38CC7440; Thu, 11 Jun 2020 08:30:31 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-21.ams2.redhat.com [10.36.114.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 2D2952B4DD; Thu, 11 Jun 2020 08:30:28 +0000 (UTC) Subject: Re: [PATCH v9 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO) To: Tom Lendacky , devel@edk2.groups.io Cc: Brijesh Singh , Ard Biesheuvel , Eric Dong , Jordan Justen , Liming Gao , Michael D Kinney , Ray Ni References: <2eeee73d6f93964ed098f367850267b092100bde.1591363657.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <453421a0-2b10-a9df-c224-ec43a5a001ff@redhat.com> Date: Thu, 11 Jun 2020 10:30:27 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2eeee73d6f93964ed098f367850267b092100bde.1591363657.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > 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 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; > } >