Re: [edk2-devel] [PATCH v8 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO)

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 5/22/20 9:14 AM, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas 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%7Cfdd2325c2e5341d8ae5408d7fe5a75f5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257536808864483&sdata=RICgoxIHQzIwTS0UWB0gFK39ENqFaoSH%2FeX6DU0h0VI%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@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   .../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.)

Impressive!  I'll make all those changes.

> 
>> +  }
>> +
>> +  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
> <Library/SafeIntLib.h>, 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.

Ditto here.

> 
>> +}
>> +
>> +/**
>> +  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?

I'll create a #define (TWO_BYTE_OPCODE_ESCAPE) that should (hopefully) be 
self commenting.

> 
>> +
>> +  switch (OpCode) {
>> +  /* MMIO write */
> 
> (15) Please update the comment style.
> 
> Also, can we be more explicit about the opcodes, with comments?

Can do.

> 
>> +  case 0x88:
>> +    Bytes = 1;
> 
> (16) Please add a "fall through" comment.

For this and remaining comments: Will do.

Thanks!
Tom

> 
>> +  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
>