BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
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.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 | 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
+ );
+
+
+/**
+ 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 */
+ 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;
+ }
+ }
+}
+
+/**
+ Determine instruction length
+
+ Return the total length of the parsed instruction.
+
+ @param[in] InstructionData Instruction parsing context
+
+ @retval Length of parsed instruction
+
+**/
+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)
+
+/**
+ 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
+
+**/
+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
+ 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
+ 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;
}
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#59869): https://edk2.groups.io/g/devel/message/59869
Mute This Topic: https://groups.io/mt/74336567/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> 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.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 | 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.
> +
> +/**
> + 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.
> + 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?
> +
> +/**
> + 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.
> +
> +**/
> +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?
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.
> +
> +/**
> + 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.
> + 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).
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!
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 (#60047): https://edk2.groups.io/g/devel/message/60047
Mute This Topic: https://groups.io/mt/74336567/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 05/21/20 19:25, Laszlo Ersek wrote:
> On 05/19/20 23:50, Lendacky, Thomas wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>>
>> 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.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 | 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.
>
>> +
>> +/**
>> + 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));
(10) Please check the patches for uses of the bitwise AND operator,
where the result is considered as a logical value.
In those contexts, the edk2 coding style needs an explicit comparison
against 0. For example, the above would be:
return (Ghcb->SaveArea.ValidBitmap[RegIndex] & (1 << RegBit)) != 0;
This is actually more than just a style question, generally speaking.
BOOLEAN in edk2 is a typedef to UINT8 (well, more precisely, to
"unsigned char"). It is not like the _Bool type in ISO C99.
ISO C99 says about _Bool, in section "6.3.1.2 Boolean type": "When any
scalar value is converted to _Bool, the result is 0 if the value
compares equal to 0; otherwise, the result is 1." (Footnote: "NaNs do
not compare equal to 0 and thus convert to 1.")
However, BOOLEAN is "unsigned char" and not _Bool; therefore, if you do
for example:
BOOLEAN
IsBit8Set (
IN UINT16 Value
)
{
return Value & BIT8;
}
then IsBit8Set (0x100) will return FALSE -- converting 0x100 to
"unsigned char" yields 0.
In edk2, in every context (not just in return statements) where we have
(Expr1 & Expr2) consumed as a logical result, we're supposed to write
((Expr1 & Expr2) != 0). (The inner parens are important as bit-AND has
weaker binding than "==" and "!=".)
Please check the rest of the patches for this.
Thanks!
Laszlo
>> +}
>> +
>> +/**
>> + 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.
>
>> + 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?
>
>> +
>> +/**
>> + 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.
>
>> +
>> +**/
>> +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?
>
> 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.
>
>
>> +
>> +/**
>> + 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.
>
>> + 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).
>
> 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!
> 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 (#60107): https://edk2.groups.io/g/devel/message/60107
Mute This Topic: https://groups.io/mt/74336567/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 5/22/20 5:05 AM, Laszlo Ersek wrote:
> On 05/21/20 19:25, 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%7C6eeb665fe5884b34f2b408d7fe37afcb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257387447015060&sdata=zteeG%2B8CL03l1w6SrqcadCH7tSTe8F5FmLhBRxLFy4k%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.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 | 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.
>>
>>> +
>>> +/**
>>> + 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));
>
> (10) Please check the patches for uses of the bitwise AND operator,
> where the result is considered as a logical value.
>
> In those contexts, the edk2 coding style needs an explicit comparison
> against 0. For example, the above would be:
>
> return (Ghcb->SaveArea.ValidBitmap[RegIndex] & (1 << RegBit)) != 0;
>
>
> This is actually more than just a style question, generally speaking.
> BOOLEAN in edk2 is a typedef to UINT8 (well, more precisely, to
> "unsigned char"). It is not like the _Bool type in ISO C99.
>
> ISO C99 says about _Bool, in section "6.3.1.2 Boolean type": "When any
> scalar value is converted to _Bool, the result is 0 if the value
> compares equal to 0; otherwise, the result is 1." (Footnote: "NaNs do
> not compare equal to 0 and thus convert to 1.")
>
> However, BOOLEAN is "unsigned char" and not _Bool; therefore, if you do
> for example:
>
> BOOLEAN
> IsBit8Set (
> IN UINT16 Value
> )
> {
> return Value & BIT8;
> }
>
> then IsBit8Set (0x100) will return FALSE -- converting 0x100 to
> "unsigned char" yields 0.
>
> In edk2, in every context (not just in return statements) where we have
> (Expr1 & Expr2) consumed as a logical result, we're supposed to write
> ((Expr1 & Expr2) != 0). (The inner parens are important as bit-AND has
> weaker binding than "==" and "!=".)
>
> Please check the rest of the patches for this.
Yes, I'll audit all the patches for this.
Thanks!
Tom
>
> Thanks!
> Laszlo
>
>>> +}
>>> +
>>> +/**
>>> + 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.
>>
>>> + 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?
>>
>>> +
>>> +/**
>>> + 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.
>>
>>> +
>>> +**/
>>> +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?
>>
>> 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.
>>
>>
>>> +
>>> +/**
>>> + 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.
>>
>>> + 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).
>>
>> 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!
>> 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 (#60143): https://edk2.groups.io/g/devel/message/60143
Mute This Topic: https://groups.io/mt/74336567/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.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 | 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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.