From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.13642.1590156877932492241 for ; Fri, 22 May 2020 07:14:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=D7NdcLPx; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1590156877; 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=tw42YwHQ8f1uZ00+LdTbndTfewV5ge/8GkLJb44L/j8=; b=D7NdcLPxiSE29cos4fMpo5eQ7zRSOdJ4ky6lfdr6VutlpZNG6OS3UZUuztDq1c+Yx2lccv eJrhq+vzT6LdimvyylHKEVLWtQrNLg+2kixz3ipYC5NxVFR/vGixYQBZrzaPzQ0g8uc2ZZ UFc8QPxp/1rpmqy8S+x/XN40w97IACI= 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-399-l-GpyNuPPAmHdY3UTXBhzw-1; Fri, 22 May 2020 10:14:28 -0400 X-MC-Unique: l-GpyNuPPAmHdY3UTXBhzw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 5F459100CCC0; Fri, 22 May 2020 14:14:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-40.ams2.redhat.com [10.36.112.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id AAAE560CD3; Fri, 22 May 2020 14:14:23 +0000 (UTC) From: "Laszlo Ersek" Subject: Re: [edk2-devel] [PATCH v8 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO) To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh , Ard Biesheuvel References: <1a5f159a487e2f20b5ad7bff83659fb1442ecbcd.1589925074.git.thomas.lendacky@amd.com> Message-ID: <169e44cb-2c1c-6d9a-342a-2a1f618e3753@redhat.com> Date: Fri, 22 May 2020 16:14:22 +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: <1a5f159a487e2f20b5ad7bff83659fb1442ecbcd.1589925074.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 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 05/19/20 23:50, Lendacky, Thomas 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 > --- > .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 436 ++++++++++++++++++ > 1 file changed, 436 insertions(+) > > diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c > index 1c6b472a47c4..50199845ceef 100644 > --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c > +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c > @@ -224,6 +224,263 @@ 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 > + > + @retval Pointer to the contents of the requested register > + > +**/ > +STATIC > +INT64 * (1) Please change the return type from (INT64*) to (UINT64*). My request will look more justified once I get to the rest of my points below. > +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 (INT64 *) Reg; > +} (2) Please remove the cast in the "return" statement. > + > +/** > + 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 > + > + @retval The memory operand effective address > + > +**/ > +STATIC > +UINTN (3) Please make the return type UINT64. It doesn't change behavior at all, as this is X64-only code, but it will make our reasoning easier. (The return value of GetEffectiveMemoryAddress() is assigned to Ext->RmData (SEV_ES_INSTRUCTION_OPCODE_EXT.RmData) later, which has type UINTN. But this is X64-only code, so that assignment is fine.) > +GetEffectiveMemoryAddress ( > + IN EFI_SYSTEM_CONTEXT_X64 *Regs, > + IN SEV_ES_INSTRUCTION_DATA *InstructionData > + ) > +{ > + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext; > + INTN EffectiveAddress; (4) Please make this a UINT64 too. > + > + Ext = &InstructionData->Ext; > + EffectiveAddress = 0; > + > + if (IsRipRelative (InstructionData)) { > + /* RIP-relative displacement is a 32-bit signed value */ (5) Please update the comment style. > + INT32 RipRelative; > + > + RipRelative = *(INT32 *) InstructionData->Displacement; OK. > + > + UpdateForDisplacement (InstructionData, 4); > + return (UINTN) ((INTN) Regs->Rip + RipRelative); So, casting "Regs->Rip" (of type UINT64) to INTN is where I start fidgeting :) The C standard says in "6.3.1.3 Signed and unsigned integers", paragraph 3: Otherwise, the new type is signed and the value cannot be represented in it; either the result is implementation-defined or an implementation-defined signal is raised. Now I *do* realize that our particular C language implementation(s) *do* define the behavior here. If Rip is in the upper half of the address space, we flip to negative (in two's complement representation), perform the signed addition, then flip back to positive (which is *not* implementation defined but standard-defined, but will do the right thing here). But that's way too hard to follow if you actually want to pay attention to the signed/unsigned conversions. We can do this without relying on the implementation-dependent two's complement representation. Here's what I suggest: RipRelative is an INT32, and may be negative. Consider the cast (UINT64)RipRelative If RipRelative is non-negative, then the value doesn't change (we'll perform a plain increment). If RipRelative is negative, we'll get the following value from the cast, mathematically speaking: (MAX_UINT64 + 1) - (-RipRelative) [*] which is just a different way of writing (MAX_UINT64 + 1) + RipRelative And the latter comes straight from the C standard, 6.3.1.3p2: Otherwise, if the new type is unsigned, the value is converted by repeatedly adding or subtracting one more than the maximum value that can be represented in the new type until the value is in the range of the new type. Now consider what happens when we add [*] to Regs->Rip (which is itself a UINT64): Regs->Rip + ((MAX_UINT64 + 1) - (-RipRelative)) Unpack the outer parens: Regs->Rip + (MAX_UINT64 + 1) - (-RipRelative) making for (Regs->Rip + (MAX_UINT64 + 1)) - (-RipRelative) The middle term falls away, per "6.2.5 Types", paragraph 9: [...] A computation involving unsigned operands can never overflow, because a result that cannot be represented by the resulting unsigned integer type is reduced modulo the number that is one greater than the largest value that can be represented by the resulting type. Therefore we get: Regs->Rip - (-RipRelative) which is exactly what we want, for a negative RipRelative. (6) Thus, the return statement should be: // // Negative displacement is handled by standard UINT64 wrap-around. // return Regs->Rip + (UINT64)RipRelative; (Technically, we could even drop the explicit (UINT64) cast -- RipRelative would be converted automatically to UINT64 --, but we should keep the (UINT64) cast for documentation purposes.) > + } > + > + switch (Ext->ModRm.Mod) { > + case 1: > + UpdateForDisplacement (InstructionData, 1); > + EffectiveAddress += (INT8) (*(INT8 *) (InstructionData->Displacement)); Considering the patch as-is, the outer (INT8) cast is redundant. But, that's not really my point. My point is how we should update this, after changing the type of EffectiveAddress to UINT64: (7) Replace the outer (INT8) cast with (UINT64). EffectiveAddress += (UINT64) (*(INT8 *) (InstructionData->Displacement)); The reasoning is the same as for (6). If the displacement is negative, the value we get on the right hand side is (MAX_UINT64 + 1) - (-Displacement) And when we add that to EffectiveAddress (also of type UINT64), the (MAX_UINT64 + 1) term falls away, and we get EffectiveAddress - (-Displacement) (The UINT64 conversion would happen anyway, per the "usual arithmetic conversions", given the new UINT64 type of EffectiveAddress; so the cast is mainly for documentation, again.) > + break; > + case 2: > + switch (InstructionData->AddrSize) { > + case Size16Bits: > + UpdateForDisplacement (InstructionData, 2); > + EffectiveAddress += (INT16) (*(INT16 *) (InstructionData->Displacement)); (8) Same as (7); please change the outer cast to (UINT64). > + break; > + default: > + UpdateForDisplacement (InstructionData, 4); > + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement)); (9) Same as (7); please change the outer cast to (UINT64). > + break; > + } > + break; > + } > + > + if (InstructionData->SibPresent) { > + if (Ext->Sib.Index != 4) { > + EffectiveAddress += (*GetRegisterPointer (Regs, Ext->Sib.Index) << Ext->Sib.Scale); In the patch, as-is, we're left-shifting an INT64 that may be negative. That's not defined by the standard; see "6.5.7 Bitwise shift operators", p4: [...] If E1 has a signed type and nonnegative value, and E1 * 2^E2 is representable in the result type, then that is the resulting value; otherwise, the behavior is undefined. (10) Therefore we should do: INT64 Displacement; CopyMem (&Displacement, GetRegisterPointer (Regs, Ext->Sib.Index), sizeof Displacement); Displacement *= (1 << Ext->Sib.Scale); // // Negative displacement is handled by standard UINT64 wrap-around. // EffectiveAddress += (UINT64)Displacement; Assuming that the instruction we're decoding isn't malformed in the first place, this is safe. (10a) The CopyMem could be replaced with Displacement = *(INT64 *)GetRegisterPointer (Regs, Ext->Sib.Index); but the CopyMem() is cleaner. (It is where we *explicitly* rely on two's complement representation.) (10b) "Ext->Sib.Scale" is at most 3 (from DecodeModRm() below -- it comes from a 2-bits wide bitfield), so left-shifting value 1 (of type INT32) is OK. (10c) Multiplying a negative INT64 by 1, 2, 4, or 8 is well-defined (assuming again that the initial Displacement value is small enough, which depends on the original instruction). If we wanted to be super-safe, we could replace this open-coded INT64 multiplication with a call to SafeInt64Mult(), from , and hang here, if the call fails. Up to you. (10d) The final addition follows the same argument as above. We could again drop the UINT64 cast (the INT64 operand would be converted to UINT64 via the "usual arithmetic conversions"), but we should keep it for documentation purposes. > + } > + > + if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) { > + EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base); This will just continue working, with EffectiveAddress and (*GetRegisterPointer()) being both UINT64's. A negative displacement will be encoded within the register that (*GetRegisterPointer()) reads out. > + } else { > + UpdateForDisplacement (InstructionData, 4); > + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement)); (11) Same as (9) -- please change the outer (INT32) cast to (UINT64), for documentation. > + } > + } else { > + EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm); Continues working fine. > + } > + > + return (UINTN) EffectiveAddress; (12) Please drop the cast. > +} > + > +/** > + 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_REX_PREFIX *RexPrefix; > + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext; > + SEV_ES_INSTRUCTION_MODRM *ModRm; > + SEV_ES_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); Both of these UINTN field assignments will continue working, with GetRegisterPointer() returning (UINT64*). > + } 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. > > @@ -411,6 +668,181 @@ 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 > + > + @retval 0 Event handled successfully > + @retval 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; > + INTN *Register; (13) Please change this to (UINT64 *). > + UINT8 OpCode, SignByte; > + > + Bytes = 0; > + > + OpCode = *(InstructionData->OpCodes); > + if (OpCode == 0x0F) { > + OpCode = *(InstructionData->OpCodes + 1); > + } (14) Can you add a comment regarding the 0x0F constant? > + > + switch (OpCode) { > + /* MMIO write */ (15) Please update the comment style. Also, can we be more explicit about the opcodes, with comments? > + case 0x88: > + Bytes = 1; (16) Please add a "fall through" comment. > + case 0x89: > + DecodeModRm (Regs, InstructionData); > + Bytes = (Bytes) ? Bytes (17) Please use an explicit (Bytes > 0) comparison. > + : (InstructionData->DataSize == Size16Bits) ? 2 > + : (InstructionData->DataSize == Size32Bits) ? 4 > + : (InstructionData->DataSize == Size64Bits) ? 8 > + : 0; I struggled for a while to figure out what bothered me about this syntax :) (18) The colons ":" should be at the ends of the lines. Bytes = ((Bytes > 0) ? Bytes : (InstructionData->DataSize == Size16Bits) ? 2 : (InstructionData->DataSize == Size32Bits) ? 4 : (InstructionData->DataSize == Size64Bits) ? 8 : 0); (I recommend the outermost parens only for supporting the indentation.) > + > + if (InstructionData->Ext.ModRm.Mod == 3) { > + /* NPF on two register operands??? */ (19) Please update the comment style. > + 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) { (20) please write (Status > 0) or (Status != 0) > + return Status; > + } > + break; > + > + case 0xC6: > + Bytes = 1; (21) Please add a "fall through" comment. > + case 0xC7: > + DecodeModRm (Regs, InstructionData); > + Bytes = (Bytes) ? Bytes > + : (InstructionData->DataSize == Size16Bits) ? 2 > + : (InstructionData->DataSize == Size32Bits) ? 4 > + : 0; (22) please see (17) and (18) > + > + 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) { (23) please write (Status > 0) or (Status != 0) > + return Status; > + } > + break; > + > + /* MMIO read */ (24) pls see (15) > + case 0x8A: > + Bytes = 1; (25) Please add a "fall through" comment. > + case 0x8B: > + DecodeModRm (Regs, InstructionData); > + Bytes = (Bytes) ? Bytes > + : (InstructionData->DataSize == Size16Bits) ? 2 > + : (InstructionData->DataSize == Size32Bits) ? 4 > + : (InstructionData->DataSize == Size64Bits) ? 8 > + : 0; (26) please see (17) and (18) > + if (InstructionData->Ext.ModRm.Mod == 3) { > + /* NPF on two register operands??? */ (27) Please update the comment style. > + 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) { (28) please write (Status > 0) or (Status != 0) > + return Status; > + } > + > + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); > + if (Bytes == 4) { > + /* Zero-extend for 32-bit operation */ (29) Please update the comment style. > + *Register = 0; Continues working with Register having type (UINT64 *). > + } > + CopyMem (Register, Ghcb->SharedBuffer, Bytes); > + break; > + > + /* MMIO Read w/ zero-extension */ (30) Please see (15) > + case 0xB6: > + Bytes = 1; (31) Please add a "fall through" comment. > + case 0xB7: > + Bytes = (Bytes) ? Bytes : 2; (32) Please use an explicit (Bytes > 0) comparison. > + > + ExitInfo1 = InstructionData->Ext.RmData; > + ExitInfo2 = Bytes; > + > + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; > + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2); > + if (Status) { (33) please write (Status > 0) or (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 */ (34) Please see (15) > + case 0xBE: > + Bytes = 1; (35) Please add a "fall through" comment. > + case 0xBF: > + Bytes = (Bytes) ? Bytes : 2; (36) Please see (17) > + > + ExitInfo1 = InstructionData->Ext.RmData; > + ExitInfo2 = Bytes; > + > + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; > + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2); > + if (Status) { (37) please write (Status > 0) or (Status != 0) > + return Status; > + } > + > + if (Bytes == 1) { > + UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer; > + > + SignByte = (*Data & 0x80) ? 0xFF : 0x00; (38) Please use BIT7 (or if there's an even better dedicated macro, then that), rather than 0x80. (39) Usual comment about bitmask used in logical context. > + } else { > + UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer; > + > + SignByte = (*Data & 0x8000) ? 0xFF : 0x00; > + } (40) Same two comments as (38) and (39). > + > + 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. > > @@ -806,6 +1238,10 @@ VmgExitHandleVc ( > NaeExit = MsrExit; > break; > > + case SVM_EXIT_NPF: > + NaeExit = MmioExit; > + break; > + > default: > NaeExit = UnsupportedExit; > } > Thanks! Laszlo