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.50]) by mx.groups.io with SMTP id smtpd.web10.13045.1590154831322211701 for ; Fri, 22 May 2020 06:40:31 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=OMxDs0Jf; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.70.50, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=mUqtTEhOEKyzNhf0QddsANuli2Mcuge2IN8QnR1EDQXC0Yzn5R/8m4IJcYbnhn0Ojt0atO+7OEBfevd3/iWssett7r6xZyb/bN8BcRPQYmI65ItscJQM/hpjEQWc9HNza7VLL3/7BMxsWxE2pQM7DO09Zj0OJYV30TCgshVEanTFP+zBkECL1pIREpD8u8dYHmylSWgI7pRxXhsyIikgDFYK1+9n2UcmjrCs67KLs47qcR7Z1A/JZq5GPAC/d2tcZbzPEIpW/6tsmipzjKpc8UbwM6hrNzCEsbDqT67fgPJ3vbZdGsCLuKIkWQZpox8InVFUom1QPXy55BAONnX6Fw== 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=9YpStzjM1AB0ibutS5Ak+v7GB1aL8YEXFo9Pc7x79vQ=; b=XIOoFiiiXUKLEaPja0dv6Uo2LbuLDqu3H0kqA9L+Yvv5Kl4g19ceUM2ZCZo59fsgJYC8srK49SdEPIm3I26xcXRLv2gDYjwSZMY5UGjRAr7G3LCwpZTPJ96Rz+QBPRs9OWn9Oixqe4VjhOc+P6Lsx0VZ0BEWQvIBV9TuhCfXwCsdK+AXyBPOoaPzCTlguKeTBMVUQBzy1nK5SU45NXN4z5tfazffrFBNGWuAZy91wT8onrCLWM8ijZvUDt4OITpYw8zvV2Eq15zQcNBHfgHgqhYA7vk6qxNhyX8V2cshF+YSNdSXunXrSXPt22sdBlUF6kyiZxtQWpPAlx5m6KXnkA== 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=9YpStzjM1AB0ibutS5Ak+v7GB1aL8YEXFo9Pc7x79vQ=; b=OMxDs0JfjSj6U9GBla9jyD3JkuXTVt7iLTNOKQSNhuPk+l/Wj+okmD4KINi5X8Kk+sJ1Xwpb7T9en0EdNz7lkDL4d973dyS8olWPgXQzFiKClGeJSnclm8PYSi2Mo+iE/SfaLbhSqrQE6pggnZyrrY62G2OZUCpEuC8olcwEd4I= 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:40:29 +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:40:29 +0000 Subject: Re: [edk2-devel] [PATCH v8 13/46] OvmfPkg/VmgExitLib: Add support for IOIO_PROT NAE events To: Laszlo Ersek , devel@edk2.groups.io 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> From: "Lendacky, Thomas" Message-ID: <40aad277-7ea1-f7ba-8091-2f8afa67f5c0@amd.com> Date: Fri, 22 May 2020 08:40:26 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 In-Reply-To: <23c7dfb3-56ac-b080-2736-687cd11d51a0@redhat.com> X-ClientProxiedBy: SN2PR01CA0021.prod.exchangelabs.com (2603:10b6:804:2::31) 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 SN2PR01CA0021.prod.exchangelabs.com (2603:10b6:804:2::31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.23 via Frontend Transport; Fri, 22 May 2020 13:40:27 +0000 X-Originating-IP: [67.79.209.213] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: f694d65e-2c0d-43e7-85cb-08d7fe55b0ab X-MS-TrafficTypeDiagnostic: DM5PR12MB1916: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:10000; X-Forefront-PRVS: 04111BAC64 X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: SPbVwKn2g2mKtqka3SSlMs8HKMj1Ci/w6pBvPF8JSxCYUSQ+/qZ3uVVykmAq7qxCPZ+ie/vq+fat+eqp9mQt48tvAKVWsM8mdeDbgh4Ri//SgMruYZaJPfXpNAgW5kORJf3JIN9c7ue056JSLC2Gyd1fM+lyDD6msVKv7odAgaqYlekZMcs/nm3RNpVlQgl9OE0o6Lhc1EU0xyYDhnpsqRo8oTj8eUUc6PaaOEHzUx/NknHdnUB3F3IjigzOdVyzjsI4Qy9sS3aGXW4NGpUROGBXSTbYuK3v50+6lDKnSjVUPinXQe5Lptc8/CnW1rzD+Xw50V6K4wxa4F0DEaa/NqyqXKMQ3wsA7AS0UPIA2C7C4wlGKKXRNcuYpmcSCwqog7qkWYF86hBJ2SJE/QyNnS869DiGt5u9jMpILysju7EJZXTldmA9QaClb6MCIX8b3OFvyFuaykq2h4vQFV8G3yhJlFBedW+GrPILg96k2U0Qbb6n6+rtSU4HIqzyNsdtBPRBzjLsXudGsvFbCl2a337cYD0FT2oY6GfclE3aDjSz4vnoesUxdKu8vdUIIYygbomZU01xySQwRisBEQo6pg== 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: jIO3LPGmBevk4A5Yx2w7ZyCWTRdF5nNIYxXEioMFFQzZNgmFlvHdF7iqslgb6EJ8SN60AW8KJIUUQbnDUwSOMSSq8VLrw+7k/d+Gxcc2+WJIS5UloCVYQT/vMPPDj0fzJ4UgBUYBOqsL6h/8w2e0vITTgix7VsIJ2PwG73Nl+55LWPZ8JrE4AbxUOACFc313SzQ2MsPW6yABMl4PczdIHwc79DEGgepPWzpoah2sLrgISVFGbn0clFobfzhRmYZrU1QHathR+8Iue4KaTT/6XeGT8nGkuC/eC+q3+v2N0O9lsn3nSEcgDYcCG4193QooYI9BDbmC1iWowhRJKV7ngq2F3PIX181Abxq3041h5wR/dnXVHaf+GtAb9f5iMzlORmj0/zDFwA1aj/XfVyTwiay1VBLg70F24cDcQKUDPZvrMU3L5L0v31SuHYm32hz9rHa7DjsMM3E2r+ivBbK8aIHkDAAIuy2Z5Pr0C7yIYxs= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: f694d65e-2c0d-43e7-85cb-08d7fe55b0ab X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2020 13:40:28.9169 (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: mu+CY+euVf09e9lozCpqmN2ulpHiNVQrrYqvwn5GK4Jtnt5sJcGb7Hi9OFMtSPYxZvcGjTlcsSBBCgTgTghDwA== 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/21/20 12:25 PM, Laszlo Ersek wrote: > On 05/19/20 23:50, Lendacky, Thomas wrote: >> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Ccba3f15d7c694d95e8e908d7fdabffd6%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637256787485321976&sdata=W4gPd4XgieGLxMCe4Ze77i1iCOZWE60UqnZmem8hpXE%3D&reserved=0 >> >> Under SEV-ES, a IOIO_PROT intercept generates a #VC exception. VMGEXIT >> must be used to allow the hypervisor to handle this intercept. >> >> Add support to construct the required GHCB values to support a IOIO_PROT >> NAE event. Parse the instruction that generated the #VC exception, >> setting the required register values in the GHCB and creating the proper >> SW_EXITINFO1 value in the GHCB. >> >> Cc: Jordan Justen >> 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. Yes, I'll move them to a header file. > >> + >> +/** >> + Checks the GHCB to determine if the specified register has been marked valid. >> + >> + The ValidBitmap area represents the areas of the GHCB that have been marked >> + valid. Return an indication of whether the area of the GHCB that holds the >> + specified register has been marked valid. >> + >> + @param[in] Ghcb Pointer to the Guest-Hypervisor Communication Block >> + @param[in] Reg Offset in the GHCB of the register to check >> + >> + @retval TRUE Register has been marked vald in the GHCB >> + @retval FALSE Register has not been marked valid in the GHCB >> + >> +**/ >> +STATIC >> +BOOLEAN >> +GhcbIsRegValid ( >> + IN GHCB *Ghcb, >> + IN GHCB_REGISTER Reg >> + ) >> +{ >> + UINT32 RegIndex; >> + UINT32 RegBit; >> + >> + RegIndex = Reg / 8; >> + RegBit = Reg & 0x07; >> + >> + return (Ghcb->SaveArea.ValidBitmap[RegIndex] & (1 << RegBit)); >> +} >> + >> +/** >> + Marks a register as valid in the GHCB. >> + >> + The ValidBitmap area represents the areas of the GHCB that have been marked >> + valid. Set the area of the GHCB that holds the specified register as valid. >> + >> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication Block >> + @param[in] Reg Offset in the GHCB of the register to mark valid >> + >> +**/ >> +STATIC >> +VOID >> +GhcbSetRegValid ( >> + IN OUT GHCB *Ghcb, >> + IN GHCB_REGISTER Reg >> + ) >> +{ >> + UINT32 RegIndex; >> + UINT32 RegBit; >> + >> + RegIndex = Reg / 8; >> + RegBit = Reg & 0x07; >> + >> + Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit); >> +} >> + >> +/** >> + Decode instruction prefixes. >> + >> + Parse the instruction data to track the instruction prefixes that have >> + been used. >> + >> + @param[in] Regs x64 processor context >> + @param[in, out] InstructionData Instruction parsing context >> + >> +**/ >> +STATIC >> +VOID >> +DecodePrefixes ( >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + SEV_ES_INSTRUCTION_MODE Mode; >> + SEV_ES_INSTRUCTION_SIZE ModeDataSize; >> + SEV_ES_INSTRUCTION_SIZE ModeAddrSize; >> + UINT8 *Byte; >> + >> + /*TODO: Determine current mode - 64-bit for now */ > > (2) I'm OK with a TODO comment, but please use the idiomatic style. Will do. > >> + Mode = LongMode64Bit; >> + ModeDataSize = Size32Bits; >> + ModeAddrSize = Size64Bits; >> + >> + InstructionData->Mode = Mode; >> + InstructionData->DataSize = ModeDataSize; >> + InstructionData->AddrSize = ModeAddrSize; >> + >> + InstructionData->Prefixes = InstructionData->Begin; >> + >> + Byte = InstructionData->Prefixes; >> + for ( ; ; Byte++, InstructionData->PrefixSize++) { >> + // >> + // Check the 0x40 to 0x4F range using an if statement here since some >> + // compilers don't like the "case 0x40 ... 0x4F:" syntax. This avoids >> + // 16 case statements below. >> + // >> + if ((*Byte >= 0x40) && (*Byte <= 0x4F)) { >> + InstructionData->RexPrefix.Uint8 = *Byte; >> + if (*Byte & 0x08) >> + InstructionData->DataSize = Size64Bits; >> + continue; >> + } >> + >> + switch (*Byte) { >> + case 0x26: >> + case 0x2E: >> + case 0x36: >> + case 0x3E: >> + if (Mode != LongMode64Bit) { >> + InstructionData->SegmentSpecified = TRUE; >> + InstructionData->Segment = (*Byte >> 3) & 3; >> + } >> + break; >> + >> + case 0x64: >> + InstructionData->SegmentSpecified = TRUE; >> + InstructionData->Segment = *Byte & 7; >> + break; >> + >> + case 0x66: >> + if (!InstructionData->RexPrefix.Uint8) { >> + InstructionData->DataSize = >> + (Mode == LongMode64Bit) ? Size16Bits : >> + (Mode == LongModeCompat32Bit) ? Size16Bits : >> + (Mode == LongModeCompat16Bit) ? Size32Bits : 0; >> + } >> + break; >> + >> + case 0x67: >> + InstructionData->AddrSize = >> + (Mode == LongMode64Bit) ? Size32Bits : >> + (Mode == LongModeCompat32Bit) ? Size16Bits : >> + (Mode == LongModeCompat16Bit) ? Size32Bits : 0; >> + break; >> + >> + case 0xF0: >> + break; >> + >> + case 0xF2: >> + InstructionData->RepMode = RepZ; >> + break; >> + >> + case 0xF3: >> + InstructionData->RepMode = RepNZ; >> + break; >> + >> + default: >> + InstructionData->OpCodes = Byte; >> + InstructionData->OpCodeSize = (*Byte == 0x0F) ? 2 : 1; >> + >> + InstructionData->End = Byte + InstructionData->OpCodeSize; >> + InstructionData->Displacement = InstructionData->End; >> + InstructionData->Immediate = InstructionData->End; >> + return; >> + } >> + } >> +} > > (3) Can we introduce macros or enum constants for the prefixes? > > Although, I seem to remember that QEMU's TCG and even KVM's instruction > parser uses open-coded magic constants for the prefixes, so the above > code would not be without precedent. > > Can we please add comments (stating the prefix names) near the case > labels at least? Will do. > >> + >> +/** >> + Determine instruction length >> + >> + Return the total length of the parsed instruction. >> + >> + @param[in] InstructionData Instruction parsing context >> + >> + @retval Length of parsed instruction > > (4) @retval is for specific return values (enum constants, macros etc). > For general descriptions, please use @return, not @retval. > > Please review the rest of the patches from this angle. I'll review all patches for @return/@retval inconsistencies. > >> + >> +**/ >> +STATIC >> +UINT64 >> +InstructionLength ( >> + IN SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + return (UINT64) (InstructionData->End - InstructionData->Begin); >> +} >> + >> +/** >> + Initialize the instruction parsing context. >> + >> + Initialize the instruction parsing context, which includes decoding the >> + instruction prefixes. >> + >> + @param[in, out] InstructionData Instruction parsing context >> + @param[in] Ghcb Pointer to the Guest-Hypervisor Communication >> + Block >> + @param[in] Regs x64 processor context >> + >> +**/ >> +STATIC >> +VOID >> +InitInstructionData ( >> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData, >> + IN GHCB *Ghcb, >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs >> + ) >> +{ >> + SetMem (InstructionData, sizeof (*InstructionData), 0); >> + InstructionData->Ghcb = Ghcb; >> + InstructionData->Begin = (UINT8 *) Regs->Rip; >> + InstructionData->End = (UINT8 *) Regs->Rip; >> + >> + DecodePrefixes (Regs, InstructionData); >> +} >> + >> +/** >> + Report an unsupported event to the hypervisor >> + >> + Use the VMGEXIT support to report an unsupported event to the hypervisor. >> + >> + @param[in] Ghcb Pointer to the Guest-Hypervisor Communication >> + Block >> + @param[in] Regs x64 processor context >> + @param[in] InstructionData Instruction parsing context >> + >> + @retval New exception value to propagate >> + >> +**/ >> +STATIC >> +UINT64 >> +UnsupportedExit ( >> + IN GHCB *Ghcb, >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + UINT64 Status; >> + >> + Status = VmgExit (Ghcb, SVM_EXIT_UNSUPPORTED, Regs->ExceptionData, 0); >> + if (Status == 0) { >> + GHCB_EVENT_INJECTION Event; >> + >> + Event.Uint64 = 0; >> + Event.Elements.Vector = GP_EXCEPTION; >> + Event.Elements.Type = GHCB_EVENT_INJECTION_TYPE_EXCEPTION; >> + Event.Elements.Valid = 1; >> + >> + Status = Event.Uint64; >> + } >> + >> + return Status; >> +} >> + >> +#define IOIO_TYPE_STR (1 << 2) >> +#define IOIO_TYPE_IN 1 >> +#define IOIO_TYPE_INS (IOIO_TYPE_IN | IOIO_TYPE_STR) >> +#define IOIO_TYPE_OUT 0 >> +#define IOIO_TYPE_OUTS (IOIO_TYPE_OUT | IOIO_TYPE_STR) >> + >> +#define IOIO_REP (1 << 3) >> + >> +#define IOIO_ADDR_64 (1 << 9) >> +#define IOIO_ADDR_32 (1 << 8) >> +#define IOIO_ADDR_16 (1 << 7) >> + >> +#define IOIO_DATA_32 (1 << 6) >> +#define IOIO_DATA_16 (1 << 5) >> +#define IOIO_DATA_8 (1 << 4) >> +#define IOIO_DATA_BYTES(x) (((x) & 0x70) >> 4) >> + >> +#define IOIO_SEG_ES (0 << 10) >> +#define IOIO_SEG_DS (3 << 10) > > (5) I feel like these macros belong in: > > MdePkg/Include/Register/Amd/Ghcb.h > > Do you agree? Yes, I can put them in Ghcb.h or some other file. > > If that exact header file is not the best, then I'd request a new header > file under OvmfPkg/Include/Register. > > (6) I'd also suggest using BIT2, BIT3, BIT9, rather than the open-coded > shifts. BITx reads more natural in edk2 source. Will do. > > >> + >> +/** >> + Build the IOIO event information. >> + >> + The IOIO event information identifies the type of IO operation to be performed >> + by the hypervisor. Build this information based on the instruction data. >> + >> + @param[in] Regs x64 processor context >> + @param[in, out] InstructionData Instruction parsing context >> + >> + @retval Others IOIO event information value > > (7) Even though "@retval Others" is a common pattern in edk2, I consider > it a mis-use of "@retval". Whenever we're tempted to write "@retval > Others", we should just use "@return". Here, for example: > > @return IOIO event information values. > > If you strongly disagree, I won't insist; I'm not trying to create > busywork for you. Otherwise, please check the rest of the OvmfPkg > patches for "@retval Others". > >> + >> +**/ >> +STATIC >> +UINT64 >> +IoioExitInfo ( >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + UINT64 ExitInfo; >> + >> + ExitInfo = 0; >> + >> + switch (*(InstructionData->OpCodes)) { >> + // IN immediate opcodes > > (8) Right, I love these comments; please do use one of the idiomatic > comment styles though. Either prepend and append empty "//" lines, or > move the single-line "//" to the right of the case labels. Will do. > >> + case 0xE4: >> + case 0xE5: >> + InstructionData->ImmediateSize = 1; >> + InstructionData->End++; >> + ExitInfo |= IOIO_TYPE_IN; >> + ExitInfo |= ((*(InstructionData->OpCodes + 1)) << 16); >> + break; >> + >> + // OUT immediate opcodes >> + case 0xE6: >> + case 0xE7: >> + InstructionData->ImmediateSize = 1; >> + InstructionData->End++; >> + ExitInfo |= IOIO_TYPE_OUT; >> + ExitInfo |= ((*(InstructionData->OpCodes + 1)) << 16) | IOIO_TYPE_OUT; >> + break; >> + >> + // IN register opcodes >> + case 0xEC: >> + case 0xED: >> + ExitInfo |= IOIO_TYPE_IN; >> + ExitInfo |= ((Regs->Rdx & 0xffff) << 16); >> + break; >> + >> + // OUT register opcodes >> + case 0xEE: >> + case 0xEF: >> + ExitInfo |= IOIO_TYPE_OUT; >> + ExitInfo |= ((Regs->Rdx & 0xffff) << 16); >> + break; >> + >> + default: >> + return 0; >> + } >> + >> + switch (*(InstructionData->OpCodes)) { >> + case 0xE4: >> + case 0xE6: >> + case 0xEC: >> + case 0xEE: >> + // Single-byte opcodes > > (9) Please make both the location and the style of this comment > consistent with (8). Yup. > > All my comments are superficial, and that's fine. I totally intend to > let you do what you need to do in this library (I can tell that writing > a new instruction decoder must have been hellish work), so I don't want > to get in your way -- just please make this easier to browse with "edk2 > eyes". > > I'm ready to ACK the patch once all comments have been addressed -- I'm > not giving an A-b at once because some of my requests / proposals need > concrete values filled in (such as header file names). > Thanks! Tom > Thanks! > Laszlo > >> + ExitInfo |= IOIO_DATA_8; >> + break; >> + >> + default: >> + // Length determined by instruction parsing >> + ExitInfo |= (InstructionData->DataSize == Size16Bits) ? IOIO_DATA_16 >> + : IOIO_DATA_32; >> + } >> + >> + switch (InstructionData->AddrSize) { >> + case Size16Bits: >> + ExitInfo |= IOIO_ADDR_16; >> + break; >> + >> + case Size32Bits: >> + ExitInfo |= IOIO_ADDR_32; >> + break; >> + >> + case Size64Bits: >> + ExitInfo |= IOIO_ADDR_64; >> + break; >> + >> + default: >> + break; >> + } >> + >> + if (InstructionData->RepMode) { >> + ExitInfo |= IOIO_REP; >> + } >> + >> + return ExitInfo; >> +} >> + >> +/** >> + Handle an IOIO event. >> + >> + Use the VMGEXIT instruction to handle an IOIO event. >> + >> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication >> + Block >> + @param[in, out] Regs x64 processor context >> + @param[in] InstructionData Instruction parsing context >> + >> + @retval 0 Event handled successfully >> + @retval Others New exception value to propagate >> + >> +**/ >> +STATIC >> +UINT64 >> +IoioExit ( >> + IN OUT GHCB *Ghcb, >> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + UINT64 ExitInfo1, Status; >> + >> + ExitInfo1 = IoioExitInfo (Regs, InstructionData); >> + if (!ExitInfo1) { >> + return UnsupportedExit (Ghcb, Regs, InstructionData); >> + } >> + >> + if (ExitInfo1 & IOIO_TYPE_IN) { >> + Ghcb->SaveArea.Rax = 0; >> + } else { >> + CopyMem (&Ghcb->SaveArea.Rax, &Regs->Rax, IOIO_DATA_BYTES (ExitInfo1)); >> + } >> + GhcbSetRegValid (Ghcb, GhcbRax); >> + >> + Status = VmgExit (Ghcb, SVM_EXIT_IOIO_PROT, ExitInfo1, 0); >> + if (Status) { >> + return Status; >> + } >> + >> + if (ExitInfo1 & IOIO_TYPE_IN) { >> + if (!GhcbIsRegValid (Ghcb, GhcbRax)) { >> + return UnsupportedExit (Ghcb, Regs, InstructionData); >> + } >> + CopyMem (&Regs->Rax, &Ghcb->SaveArea.Rax, IOIO_DATA_BYTES (ExitInfo1)); >> + } >> + >> + return 0; >> +} >> + >> /** >> Handle a #VC exception. >> >> @@ -38,6 +605,8 @@ VmgExitHandleVc ( >> MSR_SEV_ES_GHCB_REGISTER Msr; >> EFI_SYSTEM_CONTEXT_X64 *Regs; >> GHCB *Ghcb; >> + NAE_EXIT NaeExit; >> + SEV_ES_INSTRUCTION_DATA InstructionData; >> UINT64 ExitCode, Status; >> EFI_STATUS VcRet; >> >> @@ -54,24 +623,31 @@ VmgExitHandleVc ( >> >> ExitCode = Regs->ExceptionData; >> switch (ExitCode) { >> + case SVM_EXIT_IOIO_PROT: >> + NaeExit = IoioExit; >> + break; >> + >> default: >> - Status = VmgExit (Ghcb, SVM_EXIT_UNSUPPORTED, ExitCode, 0); >> - if (Status == 0) { >> - Regs->ExceptionData = 0; >> - *ExceptionType = GP_EXCEPTION; >> + NaeExit = UnsupportedExit; >> + } >> + >> + InitInstructionData (&InstructionData, Ghcb, Regs); >> + >> + Status = NaeExit (Ghcb, Regs, &InstructionData); >> + if (Status == 0) { >> + Regs->Rip += InstructionLength (&InstructionData); >> + } else { >> + GHCB_EVENT_INJECTION Event; >> + >> + Event.Uint64 = Status; >> + if (Event.Elements.ErrorCodeValid) { >> + Regs->ExceptionData = Event.Elements.ErrorCode; >> } else { >> - GHCB_EVENT_INJECTION Event; >> - >> - Event.Uint64 = Status; >> - if (Event.Elements.ErrorCodeValid) { >> - Regs->ExceptionData = Event.Elements.ErrorCode; >> - } else { >> - Regs->ExceptionData = 0; >> - } >> - >> - *ExceptionType = Event.Elements.Vector; >> + Regs->ExceptionData = 0; >> } >> >> + *ExceptionType = Event.Elements.Vector; >> + >> VcRet = EFI_PROTOCOL_ERROR; >> } >> >> >