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

["Laszlo Ersek" <lersek@redhat.com>]

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 <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.)

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

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