public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Lendacky, Thomas" <thomas.lendacky@amd.com>
To: devel@edk2.groups.io, lersek@redhat.com
Cc: Jordan Justen <jordan.l.justen@intel.com>,
	Ard Biesheuvel <ard.biesheuvel@linaro.org>,
	Michael D Kinney <michael.d.kinney@intel.com>,
	Liming Gao <liming.gao@intel.com>,
	Eric Dong <eric.dong@intel.com>, Ray Ni <ray.ni@intel.com>,
	Brijesh Singh <brijesh.singh@amd.com>,
	Ard Biesheuvel <ard.biesheuvel@arm.com>
Subject: Re: [edk2-devel] [PATCH v8 13/46] OvmfPkg/VmgExitLib: Add support for IOIO_PROT NAE events
Date: Fri, 22 May 2020 08:41:32 -0500	[thread overview]
Message-ID: <59d9628d-401c-8674-c549-1d1f3042abbc@amd.com> (raw)
In-Reply-To: <9a77d31e-cd80-2ea9-470c-30563c2f44c1@redhat.com>

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&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7C6eeb665fe5884b34f2b408d7fe37afcb%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257387447015060&amp;sdata=zteeG%2B8CL03l1w6SrqcadCH7tSTe8F5FmLhBRxLFy4k%3D&amp;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;
>>>     }
>>>   
>>>
>>
>>
>> 
>>
> 

  reply	other threads:[~2020-05-22 13:41 UTC|newest]

Thread overview: 100+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-19 21:50 [PATCH v8 00/46] SEV-ES guest support Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 01/46] MdeModulePkg: Create PCDs to be used in support of SEV-ES Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 02/46] UefiCpuPkg: Create PCD " Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 03/46] MdePkg: Add the MSR definition for the GHCB register Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 04/46] MdePkg: Add a structure definition for the GHCB Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 05/46] MdeModulePkg/DxeIplPeim: Support GHCB pages when creating page tables Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 06/46] MdePkg/BaseLib: Add support for the XGETBV instruction Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 07/46] MdePkg/BaseLib: Add support for the VMGEXIT instruction Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 08/46] UefiCpuPkg: Implement library support for VMGEXIT Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 09/46] OvmfPkg: Prepare OvmfPkg to use the VmgExitLib library Lendacky, Thomas
2020-05-21 16:42   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 10/46] UefiPayloadPkg: Prepare UefiPayloadPkg " Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 11/46] UefiCpuPkg/CpuExceptionHandler: Add base support for the #VC exception Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 12/46] OvmfPkg/VmgExitLib: Implement library support for VmgExitLib in OVMF Lendacky, Thomas
2020-05-21 16:52   ` [edk2-devel] " Laszlo Ersek
2020-05-21 17:08     ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 13/46] OvmfPkg/VmgExitLib: Add support for IOIO_PROT NAE events Lendacky, Thomas
2020-05-21 17:25   ` [edk2-devel] " Laszlo Ersek
2020-05-22 10:05     ` Laszlo Ersek
2020-05-22 13:41       ` Lendacky, Thomas [this message]
2020-05-22 13:40     ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 14/46] OvmfPkg/VmgExitLib: Support string IO " Lendacky, Thomas
2020-05-22 10:14   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 15/46] OvmfPkg/VmgExitLib: Add support for CPUID " Lendacky, Thomas
2020-05-22 10:27   ` [edk2-devel] " Laszlo Ersek
2020-05-22 19:02     ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 16/46] OvmfPkg/VmgExitLib: Add support for MSR_PROT " Lendacky, Thomas
2020-05-22 10:31   ` [edk2-devel] " Laszlo Ersek
2020-05-22 19:06     ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO) Lendacky, Thomas
2020-05-22 14:14   ` [edk2-devel] " Laszlo Ersek
2020-05-22 14:31     ` Laszlo Ersek
2020-05-22 20:41     ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 18/46] OvmfPkg/VmgExitLib: Add support for WBINVD NAE events Lendacky, Thomas
2020-05-22 14:19   ` [edk2-devel] " Laszlo Ersek
2020-05-22 20:51     ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 19/46] OvmfPkg/VmgExitLib: Add support for RDTSC " Lendacky, Thomas
2020-05-22 14:42   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 20/46] OvmfPkg/VmgExitLib: Add support for RDPMC " Lendacky, Thomas
2020-05-22 14:43   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 21/46] OvmfPkg/VmgExitLib: Add support for INVD " Lendacky, Thomas
2020-05-22 14:46   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 22/46] OvmfPkg/VmgExitLib: Add support for VMMCALL " Lendacky, Thomas
2020-05-22 14:48   ` [edk2-devel] " Laszlo Ersek
2020-05-22 14:50     ` Laszlo Ersek
2020-05-22 21:18       ` Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 23/46] OvmfPkg/VmgExitLib: Add support for RDTSCP " Lendacky, Thomas
2020-05-22 14:52   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 24/46] OvmfPkg/VmgExitLib: Add support for MONITOR/MONITORX " Lendacky, Thomas
2020-05-22 14:55   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 25/46] OvmfPkg/VmgExitLib: Add support for MWAIT/MWAITX " Lendacky, Thomas
2020-05-22 14:56   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 26/46] OvmfPkg/VmgExitLib: Add support for DR7 Read/Write " Lendacky, Thomas
2020-05-22 14:59   ` [edk2-devel] " Laszlo Ersek
2020-05-25 14:47   ` Laszlo Ersek
2020-05-26 15:06     ` Lendacky, Thomas
2020-05-27 11:54       ` Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 27/46] OvmfPkg/MemEncryptSevLib: Add an SEV-ES guest indicator function Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 28/46] OvmfPkg: Add support to perform SEV-ES initialization Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 29/46] OvmfPkg: Create a GHCB page for use during Sec phase Lendacky, Thomas
2020-05-25 15:07   ` [edk2-devel] " Laszlo Ersek
2020-05-26 15:41     ` Lendacky, Thomas
2020-05-26 15:45       ` Lendacky, Thomas
2020-05-27 11:45       ` Laszlo Ersek
2020-05-19 21:50 ` [PATCH v8 30/46] OvmfPkg/PlatformPei: Reserve GHCB-related areas if S3 is supported Lendacky, Thomas
2020-05-19 21:50 ` [PATCH v8 31/46] OvmfPkg: Create GHCB pages for use during Pei and Dxe phase Lendacky, Thomas
2020-05-25 15:21   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:51 ` [PATCH v8 32/46] OvmfPkg/PlatformPei: Move early GDT into ram when SEV-ES is enabled Lendacky, Thomas
2020-05-19 21:51 ` [PATCH v8 33/46] UefiCpuPkg: Create an SEV-ES workarea PCD Lendacky, Thomas
2020-05-19 21:51 ` [PATCH v8 34/46] OvmfPkg: Reserve a page in memory for the SEV-ES usage Lendacky, Thomas
2020-05-25 16:00   ` [edk2-devel] " Laszlo Ersek
2020-05-26 14:28     ` Lendacky, Thomas
2020-05-26 21:47       ` Lendacky, Thomas
2020-05-27 11:50         ` Laszlo Ersek
2020-05-19 21:51 ` [PATCH v8 35/46] OvmfPkg/PlatformPei: Reserve SEV-ES work area if S3 is supported Lendacky, Thomas
2020-05-26  7:53   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:51 ` [PATCH v8 36/46] OvmfPkg/ResetVector: Add support for a 32-bit SEV check Lendacky, Thomas
2020-05-25 16:50   ` [edk2-devel] " Laszlo Ersek
2020-05-26 16:31     ` Lendacky, Thomas
2020-05-27 11:59       ` Laszlo Ersek
2020-05-19 21:51 ` [PATCH v8 37/46] OvmfPkg/Sec: Add #VC exception handling for Sec phase Lendacky, Thomas
2020-05-26 13:58   ` [edk2-devel] " Laszlo Ersek
2020-05-19 21:51 ` [PATCH v8 38/46] OvmfPkg/Sec: Enable cache early to speed up booting Lendacky, Thomas
2020-05-19 21:51 ` [PATCH v8 39/46] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: Bypass flash detection with SEV-ES Lendacky, Thomas
2020-05-26 14:07   ` [edk2-devel] " Laszlo Ersek
2020-05-20  4:46 ` [PATCH v8 00/46] SEV-ES guest support Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 40/46] UefiCpuPkg: Add a 16-bit protected mode code segment descriptor Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 41/46] UefiCpuPkg/MpInitLib: Add CPU MP data flag to indicate if SEV-ES is enabled Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 42/46] UefiCpuPkg: Allow AP booting under SEV-ES Lendacky, Thomas
2020-06-01  6:17   ` Dong, Eric
2020-06-01 16:10     ` Lendacky, Thomas
2020-06-05  6:13       ` Dong, Eric
2020-06-01  7:28   ` Dong, Eric
2020-06-01 16:58     ` Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 43/46] OvmfPkg: Use the SEV-ES work area for the SEV-ES AP reset vector Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 44/46] OvmfPkg: Move the GHCB allocations into reserved memory Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 45/46] UefiCpuPkg/MpInitLib: Prepare SEV-ES guest APs for OS use Lendacky, Thomas
2020-05-20 16:56 ` [PATCH v8 46/46] Maintainers.txt: Add reviewers for the OvmfPkg SEV-related files Lendacky, Thomas
2020-05-19 21:54   ` Brijesh Singh
2020-05-26 14:12   ` [edk2-devel] " Laszlo Ersek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=59d9628d-401c-8674-c549-1d1f3042abbc@amd.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox