Re: [edk2-devel] [PATCH v8 13/46] OvmfPkg/VmgExitLib: Add support for IOIO_PROT NAE events

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

On 05/19/20 23:50, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> Under SEV-ES, a 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;
>    }
>  
>