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.jus...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheu...@arm.com>
> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com>
> ---
> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 604 +++++++++++++++++-
> 1 file changed, 590 insertions(+), 14 deletions(-)
>
> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> index 036f030d6b34..b4578ae922c1 100644
> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c
> @@ -12,6 +12,573 @@
> #include <Library/VmgExitLib.h>
> #include <Register/Amd/Msr.h>
>
> +//
> +// Instruction execution mode definition
> +//
> +typedef enum {
> + LongMode64Bit = 0,
> + LongModeCompat32Bit,
> + LongModeCompat16Bit,
> +} SEV_ES_INSTRUCTION_MODE;
> +
> +//
> +// Instruction size definition (for operand and address)
> +//
> +typedef enum {
> + Size8Bits = 0,
> + Size16Bits,
> + Size32Bits,
> + Size64Bits,
> +} SEV_ES_INSTRUCTION_SIZE;
> +
> +//
> +// Intruction segment definition
> +//
> +typedef enum {
> + SegmentEs = 0,
> + SegmentCs,
> + SegmentSs,
> + SegmentDs,
> + SegmentFs,
> + SegmentGs,
> +} SEV_ES_INSTRUCTION_SEGMENT;
> +
> +//
> +// Instruction rep function definition
> +//
> +typedef enum {
> + RepNone = 0,
> + RepZ,
> + RepNZ,
> +} SEV_ES_INSTRUCTION_REP;
> +
> +//
> +// Instruction REX prefix definition
> +//
> +typedef union {
> + struct {
> + UINT8 BitB:1;
> + UINT8 BitX:1;
> + UINT8 BitR:1;
> + UINT8 BitW:1;
> + UINT8 Rex:4;
> + } Bits;
> +
> + UINT8 Uint8;
> +} SEV_ES_INSTRUCTION_REX_PREFIX;
> +
> +//
> +// Instruction ModRM definition
> +//
> +typedef union {
> + struct {
> + UINT8 Rm:3;
> + UINT8 Reg:3;
> + UINT8 Mod:2;
> + } Bits;
> +
> + UINT8 Uint8;
> +} SEV_ES_INSTRUCTION_MODRM;
> +
> +typedef struct {
> + UINT8 Rm;
> + UINT8 Reg;
> + UINT8 Mod;
> +} SEV_ES_INSTRUCTION_MODRM_EXT;
> +
> +//
> +// Instruction SIB definition
> +//
> +typedef union {
> + struct {
> + UINT8 Base:3;
> + UINT8 Index:3;
> + UINT8 Scale:2;
> + } Bits;
> +
> + UINT8 Uint8;
> +} SEV_ES_INSTRUCTION_SIB;
> +
> +typedef struct {
> + UINT8 Base;
> + UINT8 Index;
> + UINT8 Scale;
> +} SEV_ES_INSTRUCTION_SIB_EXT;
> +
> +//
> +// Instruction opcode definition
> +//
> +typedef struct {
> + SEV_ES_INSTRUCTION_MODRM_EXT ModRm;
> +
> + SEV_ES_INSTRUCTION_SIB_EXT Sib;
> +
> + UINTN RegData;
> + UINTN RmData;
> +} SEV_ES_INSTRUCTION_OPCODE_EXT;
> +
> +//
> +// Instruction parsing context definition
> +//
> +typedef struct {
> + GHCB *Ghcb;
> +
> + SEV_ES_INSTRUCTION_MODE Mode;
> + SEV_ES_INSTRUCTION_SIZE DataSize;
> + SEV_ES_INSTRUCTION_SIZE AddrSize;
> + BOOLEAN SegmentSpecified;
> + SEV_ES_INSTRUCTION_SEGMENT Segment;
> + SEV_ES_INSTRUCTION_REP RepMode;
> +
> + UINT8 *Begin;
> + UINT8 *End;
> +
> + UINT8 *Prefixes;
> + UINT8 *OpCodes;
> + UINT8 *Displacement;
> + UINT8 *Immediate;
> +
> + SEV_ES_INSTRUCTION_REX_PREFIX RexPrefix;
> +
> + BOOLEAN ModRmPresent;
> + SEV_ES_INSTRUCTION_MODRM ModRm;
> +
> + BOOLEAN SibPresent;
> + SEV_ES_INSTRUCTION_SIB Sib;
> +
> + UINTN PrefixSize;
> + UINTN OpCodeSize;
> + UINTN DisplacementSize;
> + UINTN ImmediateSize;
> +
> + SEV_ES_INSTRUCTION_OPCODE_EXT Ext;
> +} SEV_ES_INSTRUCTION_DATA;
> +
> +//
> +// Non-automatic Exit function prototype
> +//
> +typedef
> +UINT64
> +(*NAE_EXIT) (
> + GHCB *Ghcb,
> + EFI_SYSTEM_CONTEXT_X64 *Regs,
> + SEV_ES_INSTRUCTION_DATA *InstructionData
> + );
> +
(1) From the typedefs above, can we move those that are defined in
industry specs (such as AMD SEV specs) to header file(s)? For example,
under OvmfPkg/Include/Register or OvmfPkg/Include/IndustryStandard.
> +
> +/**
> + 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
