From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM04-SN1-obe.outbound.protection.outlook.com (NAM04-SN1-obe.outbound.protection.outlook.com [40.107.70.52]) by mx.groups.io with SMTP id smtpd.web12.13238.1590154897157064116 for ; Fri, 22 May 2020 06:41:37 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=2wz5uQav; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.70.52, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KznySUyU28yUBqjqasNYgZ2N6kv6ChEiYR7vlE5bM20tIuhKyMnxY3KzdXZVnCTp317c70bQ1ob6K/5UE+acrZh4a95ZtJ8sJxQ17OmLlb6rZ7OiahH6q8mozNPg5MbtkJOzQHlGV8grArnxyP2/+0rgp0wLEwt5/b4CN7Ms3bjLhJRi4t/jzJ5TN9yrusSMYA7gznUYp6tdR7C61VGIojn6F+msuaATetiOuCAPJQK92V4/tsKQCCTSWB9OEgR1ZZoTJG0fUWlNaZfvGCxNhstRFZCr/4YMf07S6a/0Yfpyvlg2ihSHefnwXIPzTDyBIS2aGQ74ObIB4NXPwehrcQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=b6Qlmb8OoD/G4u801fPvjZ+Nu9pRYkuWL03jHySU3Yo=; b=eJ2EeKgpea77fsOFQZbwua2qUolLlEm4ulsGypyg4afwumLMKe81YXt8641/TLjcIyUO6jenjRpbvuORr6pIgWV4zkOaZ+N+6n0YeAgqNNNANU4+2pzzIhGeHAcTVXAgpfc+xW/QxkNevWW06+QKYKmgGS3i+KCBIl1jgzQ966t8GwnjwGC9EFn0+Nl7qzI/UEtBY/xTTtDFiFGgav0B22vlAzc/eMxlnhrZ+G3rH1FjqHHRqf8TzzjJCLhDBGiq85cp42qZ5RlKRiq25cgT5TPSVXZTfYdLMtBCie0zJNLck9tPFzxWRxTRfXTid/90IsJR+x+6W27L16+ds19SVQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector2-amdcloud-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=b6Qlmb8OoD/G4u801fPvjZ+Nu9pRYkuWL03jHySU3Yo=; b=2wz5uQavKsCdbDHQtyS6VJfbPmAmN6sd6YW/dVaWTufjkKp1/OL5gF/RcdL8Lq1gv+Pp7iWKFMziY8FhGRunuE1et+uoodcT/nekmXingP3+eeT9F7BArQSj0Vu+1nTDioHxTC6ABeD4VBzZELMFwQXZVSC7xa2qAnkG7XNdclo= Authentication-Results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=amd.com; Received: from DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) by DM5PR12MB1916.namprd12.prod.outlook.com (2603:10b6:3:112::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3000.20; Fri, 22 May 2020 13:41:35 +0000 Received: from DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4ce1:9947:9681:c8b1]) by DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4ce1:9947:9681:c8b1%10]) with mapi id 15.20.3000.034; Fri, 22 May 2020 13:41:35 +0000 Subject: Re: [edk2-devel] [PATCH v8 13/46] OvmfPkg/VmgExitLib: Add support for IOIO_PROT NAE events To: devel@edk2.groups.io, lersek@redhat.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh , Ard Biesheuvel References: <735eff31b5d88a9b453ca8b4f4a672eeaed3fc50.1589925074.git.thomas.lendacky@amd.com> <23c7dfb3-56ac-b080-2736-687cd11d51a0@redhat.com> <9a77d31e-cd80-2ea9-470c-30563c2f44c1@redhat.com> From: "Lendacky, Thomas" Message-ID: <59d9628d-401c-8674-c549-1d1f3042abbc@amd.com> Date: Fri, 22 May 2020 08:41:32 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 In-Reply-To: <9a77d31e-cd80-2ea9-470c-30563c2f44c1@redhat.com> X-ClientProxiedBy: SN2PR01CA0038.prod.exchangelabs.com (2603:10b6:804:2::48) To DM5PR12MB1355.namprd12.prod.outlook.com (2603:10b6:3:6e::7) Return-Path: thomas.lendacky@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from office-linux.texastahm.com (67.79.209.213) by SN2PR01CA0038.prod.exchangelabs.com (2603:10b6:804:2::48) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.24 via Frontend Transport; Fri, 22 May 2020 13:41:34 +0000 X-Originating-IP: [67.79.209.213] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 55b8f229-d8c6-42df-a9f7-08d7fe55d830 X-MS-TrafficTypeDiagnostic: DM5PR12MB1916: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:9508; X-Forefront-PRVS: 04111BAC64 X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: wBkyuiZjGNtBTz6qAx/a1Nax8XMlSLloSx0skNdrDgq3vXkzKzhxa5ETJ9LpFvSZJY0XpE0lRJht705rkp6ImL3cUTEDMTMw5qetW2IHRQMOh9Qya6uajwHunOBks09hE5RnN7t8qSREJURNAJr6FezuzFWX+q7b/3wtD4iizVDhEMnHW8WaVQVcoQVgQrgCVEAnwU1H5+wLZrz5uQcLOFnwrUqxIV80VAChQ3NOyX6udLQP4REQ6RXT68wGbX6W3QuAhRawKYDKvD+/jYzJvNrDnhY6KYbtuANWTJQ8kJW5HGTqBpDZl0R3AnXFz06rSEi5MMYVqRt13yR+yofWnFJ7/TPitHvZrsrA2gI26ueg0pUmoWyUYZQof9mFLnJHW3+Co4Sm/93eT6DbQJBbJvRbu/ZveRB0cptYkMiWUjhlY98uZOHZ8iMg5cJsU7sDT9P8vZdaisd3ix1Lur3PeMjvX4ez0fb2Gr2Or+PmORZpPW6C/9Sf1v572Oz8vVlMP+567lm0hllbePE6qXbL2CnIGpEsgQLkGVjsU67zoefM/2F2q2PiiZCh8fEyNqAw+leC/Dhpvj+PXUVJMssDRw== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:DM5PR12MB1355.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(396003)(376002)(366004)(39860400002)(346002)(136003)(31696002)(966005)(26005)(4326008)(6512007)(53546011)(6506007)(316002)(66556008)(186003)(66476007)(6486002)(66946007)(16526019)(5660300002)(36756003)(54906003)(86362001)(45080400002)(2906002)(478600001)(30864003)(2616005)(956004)(8676002)(8936002)(31686004)(19627235002)(52116002)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: /yCYb03ncLn100tCyGbUoWyCST/owDqMotD9lS093LcgZZIoXoTh3LM/qkyWcOLpK60PKrxZhor1/UHp8GSSPF5XMyU8tQ0+TeVZHyvocfWtWm8G2VJVZWUThW7+N5+Zf/eP9uSDii08IlUg75GytDkHyUzFjpyTkK6zklXoiowbJl2aJyv66fPpMHnMeX2+9rPqtYUfFOwHHq3zRilZEVQ4QoMPeeboDXxBWQWNzKT3c4VbZnecGPralF/yKcmZ6PyDr42KyzgKEB3PkpZMDSdVnlAFUj+5m+LPIIJVnE3qvCt2LklK7fj/E2ipRKCcNMl5341ylt3CQB3EQGMXIXqASGB8tDZ8s22jfSbWcpZmjb1mFsfcccBoWY3T6yB4ULDD+vdKVxERmPPKewchHHGSvaRIGwkx39LPL6ngEuT70Hjjg6ejFXQEdyd+Bj12yr/OvfAno6A3fcVbOkgjD4JgEL9yYKRbU8BljXP2A1k= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 55b8f229-d8c6-42df-a9f7-08d7fe55d830 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2020 13:41:35.2466 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: a/rZXYRT8eNni0dptDKtqOritlb7X5Ip6BHDHGY9M0IJkkF8gGhGDOKU4RVGszjEz6bCa16hCGATLm5Eq/gJeQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1916 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Signed-off-by: Tom Lendacky >>> --- >>> .../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 >>> #include >>> >>> +// >>> +// 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; >>> } >>> >>> >> >> >> >> >