On 5/21/20 12:25 PM, 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%7Ccba3f15d7c694d95e8e908d7fdabffd6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637256787485321976&sdata=W4gPd4XgieGLxMCe4Ze77i1iCOZWE60UqnZmem8hpXE%3D&reserved=0

Under SEV-ES, a IOIO_PROT intercept generates a #VC exception. VMGEXIT
must be used to allow the hypervisor to handle this intercept.

Add support to construct the required GHCB values to support a IOIO_PROT
NAE event.  Parse the instruction that generated the #VC exception,
setting the required register values in the GHCB and creating the proper
SW_EXITINFO1 value in the GHCB.

Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Laszlo Ersek <ler...@redhat.com>
Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
---
  .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 604 +++++++++++++++++-
  1 file changed, 590 insertions(+), 14 deletions(-)

diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c 
b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
index 036f030d6b34..b4578ae922c1 100644
--- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
+++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
@@ -12,6 +12,573 @@
  #include <Library/VmgExitLib.h>
  #include <Register/Amd/Msr.h>
+//
+// Instruction execution mode definition
+//
+typedef enum {
+  LongMode64Bit        = 0,
+  LongModeCompat32Bit,
+  LongModeCompat16Bit,
+} SEV_ES_INSTRUCTION_MODE;
+
+//
+// Instruction size definition (for operand and address)
+//
+typedef enum {
+  Size8Bits            = 0,
+  Size16Bits,
+  Size32Bits,
+  Size64Bits,
+} SEV_ES_INSTRUCTION_SIZE;
+
+//
+// Intruction segment definition
+//
+typedef enum {
+  SegmentEs            = 0,
+  SegmentCs,
+  SegmentSs,
+  SegmentDs,
+  SegmentFs,
+  SegmentGs,
+} SEV_ES_INSTRUCTION_SEGMENT;
+
+//
+// Instruction rep function definition
+//
+typedef enum {
+  RepNone              = 0,
+  RepZ,
+  RepNZ,
+} SEV_ES_INSTRUCTION_REP;
+
+//
+// Instruction REX prefix definition
+//
+typedef union {
+  struct {
+    UINT8  BitB:1;
+    UINT8  BitX:1;
+    UINT8  BitR:1;
+    UINT8  BitW:1;
+    UINT8  Rex:4;
+  } Bits;
+
+  UINT8  Uint8;
+} SEV_ES_INSTRUCTION_REX_PREFIX;
+
+//
+// Instruction ModRM definition
+//
+typedef union {
+  struct {
+    UINT8  Rm:3;
+    UINT8  Reg:3;
+    UINT8  Mod:2;
+  } Bits;
+
+  UINT8  Uint8;
+} SEV_ES_INSTRUCTION_MODRM;
+
+typedef struct {
+  UINT8  Rm;
+  UINT8  Reg;
+  UINT8  Mod;
+} SEV_ES_INSTRUCTION_MODRM_EXT;
+
+//
+// Instruction SIB definition
+//
+typedef union {
+  struct {
+    UINT8  Base:3;
+    UINT8  Index:3;
+    UINT8  Scale:2;
+  } Bits;
+
+  UINT8  Uint8;
+} SEV_ES_INSTRUCTION_SIB;
+
+typedef struct {
+  UINT8  Base;
+  UINT8  Index;
+  UINT8  Scale;
+} SEV_ES_INSTRUCTION_SIB_EXT;
+
+//
+// Instruction opcode definition
+//
+typedef struct {
+  SEV_ES_INSTRUCTION_MODRM_EXT  ModRm;
+
+  SEV_ES_INSTRUCTION_SIB_EXT    Sib;
+
+  UINTN                         RegData;
+  UINTN                         RmData;
+} SEV_ES_INSTRUCTION_OPCODE_EXT;
+
+//
+// Instruction parsing context definition
+//
+typedef struct {
+  GHCB                           *Ghcb;
+
+  SEV_ES_INSTRUCTION_MODE        Mode;
+  SEV_ES_INSTRUCTION_SIZE        DataSize;
+  SEV_ES_INSTRUCTION_SIZE        AddrSize;
+  BOOLEAN                        SegmentSpecified;
+  SEV_ES_INSTRUCTION_SEGMENT     Segment;
+  SEV_ES_INSTRUCTION_REP         RepMode;
+
+  UINT8                          *Begin;
+  UINT8                          *End;
+
+  UINT8                          *Prefixes;
+  UINT8                          *OpCodes;
+  UINT8                          *Displacement;
+  UINT8                          *Immediate;
+
+  SEV_ES_INSTRUCTION_REX_PREFIX  RexPrefix;
+
+  BOOLEAN                        ModRmPresent;
+  SEV_ES_INSTRUCTION_MODRM       ModRm;
+
+  BOOLEAN                        SibPresent;
+  SEV_ES_INSTRUCTION_SIB         Sib;
+
+  UINTN                          PrefixSize;
+  UINTN                          OpCodeSize;
+  UINTN                          DisplacementSize;
+  UINTN                          ImmediateSize;
+
+  SEV_ES_INSTRUCTION_OPCODE_EXT  Ext;
+} SEV_ES_INSTRUCTION_DATA;
+
+//
+// Non-automatic Exit function prototype
+//
+typedef
+UINT64
+(*NAE_EXIT) (
+  GHCB                     *Ghcb,
+  EFI_SYSTEM_CONTEXT_X64   *Regs,
+  SEV_ES_INSTRUCTION_DATA  *InstructionData
+  );
+

(1) From the typedefs above, can we move those that are defined in
industry specs (such as AMD SEV specs) to header file(s)? For example,
under OvmfPkg/Include/Register or OvmfPkg/Include/IndustryStandard.

Yes, I'll move them to a header file.


+
+/**
+  Checks the GHCB to determine if the specified register has been marked valid.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Return an indication of whether the area of the GHCB that holds the
+  specified register has been marked valid.
+
+  @param[in] Ghcb    Pointer to the Guest-Hypervisor Communication Block
+  @param[in] Reg     Offset in the GHCB of the register to check
+
+  @retval TRUE       Register has been marked vald in the GHCB
+  @retval FALSE      Register has not been marked valid in the GHCB
+
+**/
+STATIC
+BOOLEAN
+GhcbIsRegValid (
+  IN GHCB                *Ghcb,
+  IN GHCB_REGISTER       Reg
+  )
+{
+  UINT32  RegIndex;
+  UINT32  RegBit;
+
+  RegIndex = Reg / 8;
+  RegBit   = Reg & 0x07;
+
+  return (Ghcb->SaveArea.ValidBitmap[RegIndex] & (1 << RegBit));
+}
+
+/**
+  Marks a register as valid in the GHCB.
+
+  The ValidBitmap area represents the areas of the GHCB that have been marked
+  valid. Set the area of the GHCB that holds the specified register as valid.
+
+  @param[in, out] Ghcb    Pointer to the Guest-Hypervisor Communication Block
+  @param[in] Reg          Offset in the GHCB of the register to mark valid
+
+**/
+STATIC
+VOID
+GhcbSetRegValid (
+  IN OUT GHCB                *Ghcb,
+  IN     GHCB_REGISTER       Reg
+  )
+{
+  UINT32  RegIndex;
+  UINT32  RegBit;
+
+  RegIndex = Reg / 8;
+  RegBit   = Reg & 0x07;
+
+  Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit);
+}
+
+/**
+  Decode instruction prefixes.
+
+  Parse the instruction data to track the instruction prefixes that have
+  been used.
+
+  @param[in]      Regs             x64 processor context
+  @param[in, out] InstructionData  Instruction parsing context
+
+**/
+STATIC
+VOID
+DecodePrefixes (
+  IN     EFI_SYSTEM_CONTEXT_X64   *Regs,
+  IN OUT SEV_ES_INSTRUCTION_DATA  *InstructionData
+  )
+{
+  SEV_ES_INSTRUCTION_MODE  Mode;
+  SEV_ES_INSTRUCTION_SIZE  ModeDataSize;
+  SEV_ES_INSTRUCTION_SIZE  ModeAddrSize;
+  UINT8                    *Byte;
+
+  /*TODO: Determine current mode - 64-bit for now */

(2) I'm OK with a TODO comment, but please use the idiomatic style.

Will do.


+  Mode = LongMode64Bit;
+  ModeDataSize = Size32Bits;
+  ModeAddrSize = Size64Bits;
+
+  InstructionData->Mode = Mode;
+  InstructionData->DataSize = ModeDataSize;
+  InstructionData->AddrSize = ModeAddrSize;
+
+  InstructionData->Prefixes = InstructionData->Begin;
+
+  Byte = InstructionData->Prefixes;
+  for ( ; ; Byte++, InstructionData->PrefixSize++) {
+    //
+    // Check the 0x40 to 0x4F range using an if statement here since some
+    // compilers don't like the "case 0x40 ... 0x4F:" syntax. This avoids
+    // 16 case statements below.
+    //
+    if ((*Byte >= 0x40) && (*Byte <= 0x4F)) {
+      InstructionData->RexPrefix.Uint8 = *Byte;
+      if (*Byte & 0x08)
+        InstructionData->DataSize = Size64Bits;
+      continue;
+    }
+
+    switch (*Byte) {
+    case 0x26:
+    case 0x2E:
+    case 0x36:
+    case 0x3E:
+      if (Mode != LongMode64Bit) {
+        InstructionData->SegmentSpecified = TRUE;
+        InstructionData->Segment = (*Byte >> 3) & 3;
+      }
+      break;
+
+    case 0x64:
+      InstructionData->SegmentSpecified = TRUE;
+      InstructionData->Segment = *Byte & 7;
+      break;
+
+    case 0x66:
+      if (!InstructionData->RexPrefix.Uint8) {
+        InstructionData->DataSize =
+          (Mode == LongMode64Bit)       ? Size16Bits :
+          (Mode == LongModeCompat32Bit) ? Size16Bits :
+          (Mode == LongModeCompat16Bit) ? Size32Bits : 0;
+      }
+      break;
+
+    case 0x67:
+      InstructionData->AddrSize =
+        (Mode == LongMode64Bit)       ? Size32Bits :
+        (Mode == LongModeCompat32Bit) ? Size16Bits :
+        (Mode == LongModeCompat16Bit) ? Size32Bits : 0;
+      break;
+
+    case 0xF0:
+      break;
+
+    case 0xF2:
+      InstructionData->RepMode = RepZ;
+      break;
+
+    case 0xF3:
+      InstructionData->RepMode = RepNZ;
+      break;
+
+    default:
+      InstructionData->OpCodes = Byte;
+      InstructionData->OpCodeSize = (*Byte == 0x0F) ? 2 : 1;
+
+      InstructionData->End = Byte + InstructionData->OpCodeSize;
+      InstructionData->Displacement = InstructionData->End;
+      InstructionData->Immediate = InstructionData->End;
+      return;
+    }
+  }
+}

(3) Can we introduce macros or enum constants for the prefixes?

Although, I seem to remember that QEMU's TCG and even KVM's instruction
parser uses open-coded magic constants for the prefixes, so the above
code would not be without precedent.

Can we please add comments (stating the prefix names) near the case
labels at least?

Will do.


+
+/**
+  Determine instruction length
+
+  Return the total length of the parsed instruction.
+
+  @param[in] InstructionData  Instruction parsing context
+
+  @retval                     Length of parsed instruction

(4) @retval is for specific return values (enum constants, macros etc).
For general descriptions, please use @return, not @retval.

Please review the rest of the patches from this angle.

I'll review all patches for @return/@retval inconsistencies.


+
+**/
+STATIC
+UINT64
+InstructionLength (
+  IN SEV_ES_INSTRUCTION_DATA  *InstructionData
+  )
+{
+  return (UINT64) (InstructionData->End - InstructionData->Begin);
+}
+
+/**
+  Initialize the instruction parsing context.
+
+  Initialize the instruction parsing context, which includes decoding the
+  instruction prefixes.
+
+  @param[in, out] InstructionData  Instruction parsing context
+  @param[in]      Ghcb             Pointer to the Guest-Hypervisor 
Communication
+                                   Block
+  @param[in]      Regs             x64 processor context
+
+**/
+STATIC
+VOID
+InitInstructionData (
+  IN OUT SEV_ES_INSTRUCTION_DATA  *InstructionData,
+  IN     GHCB                     *Ghcb,
+  IN     EFI_SYSTEM_CONTEXT_X64   *Regs
+  )
+{
+  SetMem (InstructionData, sizeof (*InstructionData), 0);
+  InstructionData->Ghcb = Ghcb;
+  InstructionData->Begin = (UINT8 *) Regs->Rip;
+  InstructionData->End = (UINT8 *) Regs->Rip;
+
+  DecodePrefixes (Regs, InstructionData);
+}
+
+/**
+  Report an unsupported event to the hypervisor
+
+  Use the VMGEXIT support to report an unsupported event to the hypervisor.
+
+  @param[in] Ghcb             Pointer to the Guest-Hypervisor Communication
+                              Block
+  @param[in] Regs             x64 processor context
+  @param[in] InstructionData  Instruction parsing context
+
+  @retval                     New exception value to propagate
+
+**/
+STATIC
+UINT64
+UnsupportedExit (
+  IN GHCB                     *Ghcb,
+  IN EFI_SYSTEM_CONTEXT_X64   *Regs,
+  IN SEV_ES_INSTRUCTION_DATA  *InstructionData
+  )
+{
+  UINT64  Status;
+
+  Status = VmgExit (Ghcb, SVM_EXIT_UNSUPPORTED, Regs->ExceptionData, 0);
+  if (Status == 0) {
+    GHCB_EVENT_INJECTION  Event;
+
+    Event.Uint64 = 0;
+    Event.Elements.Vector = GP_EXCEPTION;
+    Event.Elements.Type   = GHCB_EVENT_INJECTION_TYPE_EXCEPTION;
+    Event.Elements.Valid  = 1;
+
+    Status = Event.Uint64;
+  }
+
+  return Status;
+}
+
+#define IOIO_TYPE_STR       (1 << 2)
+#define IOIO_TYPE_IN        1
+#define IOIO_TYPE_INS       (IOIO_TYPE_IN | IOIO_TYPE_STR)
+#define IOIO_TYPE_OUT       0
+#define IOIO_TYPE_OUTS      (IOIO_TYPE_OUT | IOIO_TYPE_STR)
+
+#define IOIO_REP            (1 << 3)
+
+#define IOIO_ADDR_64        (1 << 9)
+#define IOIO_ADDR_32        (1 << 8)
+#define IOIO_ADDR_16        (1 << 7)
+
+#define IOIO_DATA_32        (1 << 6)
+#define IOIO_DATA_16        (1 << 5)
+#define IOIO_DATA_8         (1 << 4)
+#define IOIO_DATA_BYTES(x)  (((x) & 0x70) >> 4)
+
+#define IOIO_SEG_ES         (0 << 10)
+#define IOIO_SEG_DS         (3 << 10)

(5) I feel like these macros belong in:

   MdePkg/Include/Register/Amd/Ghcb.h

Do you agree?

Yes, I can put them in Ghcb.h or some other file.


If that exact header file is not the best, then I'd request a new header
file under OvmfPkg/Include/Register.

(6) I'd also suggest using BIT2, BIT3, BIT9, rather than the open-coded
shifts. BITx reads more natural in edk2 source.

Will do.



+
+/**
+  Build the IOIO event information.
+
+  The IOIO event information identifies the type of IO operation to be 
performed
+  by the hypervisor. Build this information based on the instruction data.
+
+  @param[in]       Regs             x64 processor context
+  @param[in, out]  InstructionData  Instruction parsing context
+
+  @retval Others                    IOIO event information value

(7) Even though "@retval Others" is a common pattern in edk2, I consider
it a mis-use of "@retval". Whenever we're tempted to write "@retval
Others", we should just use "@return". Here, for example:

   @return  IOIO event information values.

If you strongly disagree, I won't insist; I'm not trying to create
busywork for you. Otherwise, please check the rest of the OvmfPkg
patches for "@retval Others".

+
+**/
+STATIC
+UINT64
+IoioExitInfo (
+  IN     EFI_SYSTEM_CONTEXT_X64   *Regs,
+  IN OUT SEV_ES_INSTRUCTION_DATA  *InstructionData
+  )
+{
+  UINT64  ExitInfo;
+
+  ExitInfo = 0;
+
+  switch (*(InstructionData->OpCodes)) {
+  // IN immediate opcodes

(8) Right, I love these comments; please do use one of the idiomatic
comment styles though. Either prepend and append empty "//" lines, or
move the single-line "//" to the right of the case labels.

Will do.


+  case 0xE4:
+  case 0xE5:
+    InstructionData->ImmediateSize = 1;
+    InstructionData->End++;
+    ExitInfo |= IOIO_TYPE_IN;
+    ExitInfo |= ((*(InstructionData->OpCodes + 1)) << 16);
+    break;
+
+  // OUT immediate opcodes
+  case 0xE6:
+  case 0xE7:
+    InstructionData->ImmediateSize = 1;
+    InstructionData->End++;
+    ExitInfo |= IOIO_TYPE_OUT;
+    ExitInfo |= ((*(InstructionData->OpCodes + 1)) << 16) | IOIO_TYPE_OUT;
+    break;
+
+  // IN register opcodes
+  case 0xEC:
+  case 0xED:
+    ExitInfo |= IOIO_TYPE_IN;
+    ExitInfo |= ((Regs->Rdx & 0xffff) << 16);
+    break;
+
+  // OUT register opcodes
+  case 0xEE:
+  case 0xEF:
+    ExitInfo |= IOIO_TYPE_OUT;
+    ExitInfo |= ((Regs->Rdx & 0xffff) << 16);
+    break;
+
+  default:
+    return 0;
+  }
+
+  switch (*(InstructionData->OpCodes)) {
+  case 0xE4:
+  case 0xE6:
+  case 0xEC:
+  case 0xEE:
+    // Single-byte opcodes

(9) Please make both the location and the style of this comment
consistent with (8).

Yup.


All my comments are superficial, and that's fine. I totally intend to
let you do what you need to do in this library (I can tell that writing
a new instruction decoder must have been hellish work), so I don't want
to get in your way -- just please make this easier to browse with "edk2
eyes".

I'm ready to ACK the patch once all comments have been addressed -- I'm
not giving an A-b at once because some of my requests / proposals need
concrete values filled in (such as header file names).


Thanks!
Tom

Thanks!
Laszlo

+    ExitInfo |= IOIO_DATA_8;
+    break;
+
+  default:
+    // Length determined by instruction parsing
+    ExitInfo |= (InstructionData->DataSize == Size16Bits) ? IOIO_DATA_16
+                                                          : IOIO_DATA_32;
+  }
+
+  switch (InstructionData->AddrSize) {
+  case Size16Bits:
+    ExitInfo |= IOIO_ADDR_16;
+    break;
+
+  case Size32Bits:
+    ExitInfo |= IOIO_ADDR_32;
+    break;
+
+  case Size64Bits:
+    ExitInfo |= IOIO_ADDR_64;
+    break;
+
+  default:
+    break;
+  }
+
+  if (InstructionData->RepMode) {
+    ExitInfo |= IOIO_REP;
+  }
+
+  return ExitInfo;
+}
+
+/**
+  Handle an IOIO event.
+
+  Use the VMGEXIT instruction to handle an IOIO event.
+
+  @param[in, out] Ghcb             Pointer to the Guest-Hypervisor 
Communication
+                                   Block
+  @param[in, out] Regs             x64 processor context
+  @param[in]      InstructionData  Instruction parsing context
+
+  @retval 0                        Event handled successfully
+  @retval Others                   New exception value to propagate
+
+**/
+STATIC
+UINT64
+IoioExit (
+  IN OUT GHCB                     *Ghcb,
+  IN OUT EFI_SYSTEM_CONTEXT_X64   *Regs,
+  IN     SEV_ES_INSTRUCTION_DATA  *InstructionData
+  )
+{
+  UINT64  ExitInfo1, Status;
+
+  ExitInfo1 = IoioExitInfo (Regs, InstructionData);
+  if (!ExitInfo1) {
+    return UnsupportedExit (Ghcb, Regs, InstructionData);
+  }
+
+  if (ExitInfo1 & IOIO_TYPE_IN) {
+    Ghcb->SaveArea.Rax = 0;
+  } else {
+    CopyMem (&Ghcb->SaveArea.Rax, &Regs->Rax, IOIO_DATA_BYTES (ExitInfo1));
+  }
+  GhcbSetRegValid (Ghcb, GhcbRax);
+
+  Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, 0);
+  if (Status) {
+    return Status;
+  }
+
+  if (ExitInfo1 & IOIO_TYPE_IN) {
+    if (!GhcbIsRegValid (Ghcb, GhcbRax)) {
+      return UnsupportedExit (Ghcb, Regs, InstructionData);
+    }
+    CopyMem (&Regs->Rax, &Ghcb->SaveArea.Rax, IOIO_DATA_BYTES (ExitInfo1));
+  }
+
+  return 0;
+}
+
  /**
    Handle a #VC exception.
@@ -38,6 +605,8 @@ VmgExitHandleVc (
    MSR_SEV_ES_GHCB_REGISTER  Msr;
    EFI_SYSTEM_CONTEXT_X64    *Regs;
    GHCB                      *Ghcb;
+  NAE_EXIT                  NaeExit;
+  SEV_ES_INSTRUCTION_DATA   InstructionData;
    UINT64                    ExitCode, Status;
    EFI_STATUS                VcRet;
@@ -54,24 +623,31 @@ VmgExitHandleVc ( ExitCode = Regs->ExceptionData;
    switch (ExitCode) {
+  case SVM_EXIT_IOIO_PROT:
+    NaeExit = IoioExit;
+    break;
+
    default:
-    Status = VmgExit (Ghcb, SVM_EXIT_UNSUPPORTED, ExitCode, 0);
-    if (Status == 0) {
-      Regs->ExceptionData = 0;
-      *ExceptionType = GP_EXCEPTION;
+    NaeExit = UnsupportedExit;
+  }
+
+  InitInstructionData (&InstructionData, Ghcb, Regs);
+
+  Status = NaeExit (Ghcb, Regs, &InstructionData);
+  if (Status == 0) {
+    Regs->Rip += InstructionLength (&InstructionData);
+  } else {
+    GHCB_EVENT_INJECTION  Event;
+
+    Event.Uint64 = Status;
+    if (Event.Elements.ErrorCodeValid) {
+      Regs->ExceptionData = Event.Elements.ErrorCode;
      } else {
-      GHCB_EVENT_INJECTION  Event;
-
-      Event.Uint64 = Status;
-      if (Event.Elements.ErrorCodeValid) {
-        Regs->ExceptionData = Event.Elements.ErrorCode;
-      } else {
-        Regs->ExceptionData = 0;
-      }
-
-      *ExceptionType = Event.Elements.Vector;
+      Regs->ExceptionData = 0;
      }
+ *ExceptionType = Event.Elements.Vector;
+
      VcRet = EFI_PROTOCOL_ERROR;
    }


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#60142): https://edk2.groups.io/g/devel/message/60142
Mute This Topic: https://groups.io/mt/74336567/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Reply via email to