From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-DM6-obe.outbound.protection.outlook.com (NAM11-DM6-obe.outbound.protection.outlook.com [40.107.223.40]) by mx.groups.io with SMTP id smtpd.web10.21493.1590180122478107681 for ; Fri, 22 May 2020 13:42:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=Gkx9qnnI; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.223.40, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ljWgBSEt7qWuAqqbJwB2ZCxCP6QXqRwqn+gIrvxQLju9lpd2pJS0wbd/TNv/ERbAeGvIT/RUt1JpYJwfXxLZU9/UAbBMZYlp/QVObogG8jDVsNsQKicPlB5VLRqh5bDze9IsIVSNoC9VZvg0J2abtrCAWAwUlhEujPs4wa7sGm0qKARenfQiS57pbHgGKHGp02AwmejAZjvKYcQpwQ/3Aj3MOeifCVmlZS96b8uklC+S/1rR+swJoDemVtVBc7uiuaQa5ZlU2JtelvO8iXnyvZSr423PmBDO7DtW8tdSY/hQ7vJTxDLHqcZRThbZ4bUk9i0y0KrC2sb9EAvlVhRXHQ== 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=IJQnoKQDrcJlOKD1N4r9r2jTHKrJyZVrAZ61f17p0og=; b=hgwfctogvGuI3gHx4+TYJxOiefOEtbvgsLl8VfQKVoxvRKm7Ucgz+iD5/KoF01mTdIwFzymQ9gdkcJvzh38TZ4CpODwL8kSVqBfYZLeCMcG17tP6uoKskj1/zVFNE/bUkRSaELBAeJdyS6iJFvDWgLREZDBpN+to9ZSt0rZvEYN+CPqlg2hegwn8k3IP/t2Di48hu7Qokm+rypKejuEalc6mlpe6g9k4o8S7XM3pgauH3Gop1VehjNN44K748FurYCTLYfdud0VsyXpWMMS4lXayJM2XLRCVgTjIDGBErgbhmYQB5M9yY2dRDdd/iLwURXXXrsdZ/rETP4/K55BL+g== 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=IJQnoKQDrcJlOKD1N4r9r2jTHKrJyZVrAZ61f17p0og=; b=Gkx9qnnIi+bkSJKb00w29+WocMyuRFRfPHW2W2qMc6fdznW36DSFbj7FLk6qi23CATzKfZMut5VXIe1iWjtM2dPj1zpPn1wLINVHIVE1Cy56OQln9wet6f3OsMn7uiPgMXQGoRZSukTRdhfr5JNH6O2UXi3cBbBaFS5tI3Tm/Ds= 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 DM5PR12MB1337.namprd12.prod.outlook.com (2603:10b6:3:6e::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.26; Fri, 22 May 2020 20:42:00 +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 20:42:00 +0000 Subject: Re: [edk2-devel] [PATCH v8 17/46] OvmfPkg/VmgExitLib: Add support for NPF NAE events (MMIO) 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: <1a5f159a487e2f20b5ad7bff83659fb1442ecbcd.1589925074.git.thomas.lendacky@amd.com> <169e44cb-2c1c-6d9a-342a-2a1f618e3753@redhat.com> From: "Lendacky, Thomas" Message-ID: <8540f6e8-89da-dd1c-53fa-8d74f1f5c044@amd.com> Date: Fri, 22 May 2020 15:41:57 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 In-Reply-To: <169e44cb-2c1c-6d9a-342a-2a1f618e3753@redhat.com> X-ClientProxiedBy: SN4PR0201CA0065.namprd02.prod.outlook.com (2603:10b6:803:20::27) 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 SN4PR0201CA0065.namprd02.prod.outlook.com (2603:10b6:803:20::27) 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 20:41:59 +0000 X-Originating-IP: [67.79.209.213] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 04c43c43-bcfe-42d1-782c-08d7fe90936b X-MS-TrafficTypeDiagnostic: DM5PR12MB1337: 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: VnjEPVe8mTXOWN4zqpvHyyRNMRh+xqU1YDVkTa+tlWB7uho8vhfSPT9HnDJhE69z2Bc0+EU9BJ5ZmYZPWmvbHu68HN1nSvWcmXWltfJT2d6KsCtCV17so2VwlH8QhSeN6FXzAOccFg16TW8WsB/fRumDR3Wx7JRIdKzax7TVdKHMca5rsL9k91rcW21vJa/twlSR1QQlcTHlEJmButvUeVN/WM/lczdZwLI55FqBBDbXsqj6XvIF1uEQShBJxD02uf/SXlvpNP8oU+O3po5twSB7KvK3M91Pe8TNOH0E+Si6pLC2ksNd1uf+EFywJY/sbJcGMN9fwbi/h8sdzZ+W4ZXeFagTrWBihAJ83fr9fcreFYbcV9duHUBZCP8TBdGKVt8/HRKq8cLHEqHcvEZ2b8/f3IYdL1FD16w0DZhxAhWSD5R22Lecgq0iI1mOU4xCdibVszcS+dzJTm/r15FPUQ== 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)(376002)(346002)(136003)(396003)(39860400002)(366004)(2616005)(19627235002)(5660300002)(54906003)(6512007)(316002)(31686004)(36756003)(45080400002)(966005)(86362001)(4326008)(31696002)(478600001)(956004)(8936002)(8676002)(26005)(52116002)(16526019)(6506007)(30864003)(53546011)(66946007)(6486002)(186003)(2906002)(66556008)(66476007)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: vAgPQVmVrcAx1QRGcU1/5s/V4+zPgcBUoxrcGnLlTqxuc8v91BTAHrWf4cSX5QIi7GKsdSF+jUm6Z+vPGQwU1lz1coaZocnDACsvRq2QajS7rHTqkpgtskCifomTq8StNY9+pMIAnFQ2g8m4+D7rv74Gf03LxhLNbke+sVIJKl/Q8u1Asm20CHXffPo1EiAJrl8yPTgbpxwXLSylV3unyoHThBbR8Cqd7CxmUZ8MUDGkKemwYRhXZg/AQgVSYrFq4TdFM7mG07tGP81nWqe5995oEUmpm8uDvsEP6HwXpBITRRm1rEtOeqD6p6Xb0zeePrlJ/7ppa7KnzdRsWzxmic+rjvi2OCGI+T5h5GI408SBHUoDmD+XF/A9Ti9HJIkGEFMTHCHBA+7+CfZdCz7boP0Fb2V9lBM9OArTCKihVMziusbr+4UumIzYSybfB2rDAZR446LYGnU7PV4xQvarGBIoP2SUGItGW/ij7oU405w= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 04c43c43-bcfe-42d1-782c-08d7fe90936b X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 May 2020 20:42:00.4371 (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: kDsXuEezLguEOzqT8KRj5gXhlsLu0oraSeuVQ3AkPEJ3Fpk8W9PFltnEOlq69IAHqQUe8ifKpukiymLHf2dyQw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB1337 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 5/22/20 9:14 AM, 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%7Cfdd2325c2e5341d8ae5408d7fe5a75f5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637257536808864483&sdata=RICgoxIHQzIwTS0UWB0gFK39ENqFaoSH%2FeX6DU0h0VI%3D&reserved=0 >> >> Under SEV-ES, a NPF intercept for an NPT entry with a reserved bit set >> generates a #VC exception. This condition is assumed to be an MMIO access. >> VMGEXIT must be used to allow the hypervisor to handle this intercept. >> >> Add support to construct the required GHCB values to support a NPF NAE >> event for MMIO. Parse the instruction that generated the #VC exception, >> setting the required register values in the GHCB and creating the proper >> SW_EXIT_INFO1, SW_EXITINFO2 and SW_SCRATCH values in the GHCB. >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Signed-off-by: Tom Lendacky >> --- >> .../Library/VmgExitLib/X64/VmgExitVcHandler.c | 436 ++++++++++++++++++ >> 1 file changed, 436 insertions(+) >> >> diff --git a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c >> index 1c6b472a47c4..50199845ceef 100644 >> --- a/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c >> +++ b/OvmfPkg/Library/VmgExitLib/X64/VmgExitVcHandler.c >> @@ -224,6 +224,263 @@ GhcbSetRegValid ( >> Ghcb->SaveArea.ValidBitmap[RegIndex] |= (1 << RegBit); >> } >> >> +/** >> + Return a pointer to the contents of the specified register. >> + >> + Based upon the input register, return a pointer to the registers contents >> + in the x86 processor context. >> + >> + @param[in] Regs x64 processor context >> + @param[in] Register Register to obtain pointer for >> + >> + @retval Pointer to the contents of the requested register >> + >> +**/ >> +STATIC >> +INT64 * > > (1) Please change the return type from (INT64*) to (UINT64*). > > My request will look more justified once I get to the rest of my points > below. > >> +GetRegisterPointer ( >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN UINT8 Register >> + ) >> +{ >> + UINT64 *Reg; >> + >> + switch (Register) { >> + case 0: >> + Reg = &Regs->Rax; >> + break; >> + case 1: >> + Reg = &Regs->Rcx; >> + break; >> + case 2: >> + Reg = &Regs->Rdx; >> + break; >> + case 3: >> + Reg = &Regs->Rbx; >> + break; >> + case 4: >> + Reg = &Regs->Rsp; >> + break; >> + case 5: >> + Reg = &Regs->Rbp; >> + break; >> + case 6: >> + Reg = &Regs->Rsi; >> + break; >> + case 7: >> + Reg = &Regs->Rdi; >> + break; >> + case 8: >> + Reg = &Regs->R8; >> + break; >> + case 9: >> + Reg = &Regs->R9; >> + break; >> + case 10: >> + Reg = &Regs->R10; >> + break; >> + case 11: >> + Reg = &Regs->R11; >> + break; >> + case 12: >> + Reg = &Regs->R12; >> + break; >> + case 13: >> + Reg = &Regs->R13; >> + break; >> + case 14: >> + Reg = &Regs->R14; >> + break; >> + case 15: >> + Reg = &Regs->R15; >> + break; >> + default: >> + Reg = NULL; >> + } >> + ASSERT (Reg != NULL); >> + >> + return (INT64 *) Reg; >> +} > > (2) Please remove the cast in the "return" statement. > >> + >> +/** >> + Update the instruction parsing context for displacement bytes. >> + >> + @param[in, out] InstructionData Instruction parsing context >> + @param[in] Size The instruction displacement size >> + >> +**/ >> +STATIC >> +VOID >> +UpdateForDisplacement ( >> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData, >> + IN UINTN Size >> + ) >> +{ >> + InstructionData->DisplacementSize = Size; >> + InstructionData->Immediate += Size; >> + InstructionData->End += Size; >> +} >> + >> +/** >> + Determine if an instruction address if RIP relative. >> + >> + Examine the instruction parsing context to determine if the address offset >> + is relative to the instruction pointer. >> + >> + @param[in] InstructionData Instruction parsing context >> + >> + @retval TRUE Instruction addressing is RIP relative >> + @retval FALSE Instruction addressing is not RIP relative >> + >> +**/ >> +STATIC >> +BOOLEAN >> +IsRipRelative ( >> + IN SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext; >> + >> + Ext = &InstructionData->Ext; >> + >> + return ((InstructionData->Mode == LongMode64Bit) && >> + (Ext->ModRm.Mod == 0) && >> + (Ext->ModRm.Rm == 5) && >> + (InstructionData->SibPresent == FALSE)); >> +} >> + >> +/** >> + Return the effective address of a memory operand. >> + >> + Examine the instruction parsing context to obtain the effective memory >> + address of a memory operand. >> + >> + @param[in] Regs x64 processor context >> + @param[in] InstructionData Instruction parsing context >> + >> + @retval The memory operand effective address >> + >> +**/ >> +STATIC >> +UINTN > > (3) Please make the return type UINT64. > > It doesn't change behavior at all, as this is X64-only code, but it will > make our reasoning easier. > > (The return value of GetEffectiveMemoryAddress() is assigned to > Ext->RmData (SEV_ES_INSTRUCTION_OPCODE_EXT.RmData) later, which has type > UINTN. But this is X64-only code, so that assignment is fine.) > >> +GetEffectiveMemoryAddress ( >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext; >> + INTN EffectiveAddress; > > (4) Please make this a UINT64 too. > >> + >> + Ext = &InstructionData->Ext; >> + EffectiveAddress = 0; >> + >> + if (IsRipRelative (InstructionData)) { >> + /* RIP-relative displacement is a 32-bit signed value */ > > (5) Please update the comment style. > >> + INT32 RipRelative; >> + >> + RipRelative = *(INT32 *) InstructionData->Displacement; > > OK. > >> + >> + UpdateForDisplacement (InstructionData, 4); >> + return (UINTN) ((INTN) Regs->Rip + RipRelative); > > So, casting "Regs->Rip" (of type UINT64) to INTN is where I start > fidgeting :) The C standard says in "6.3.1.3 Signed and unsigned > integers", paragraph 3: > > Otherwise, the new type is signed and the value cannot be represented > in it; either the result is implementation-defined or an > implementation-defined signal is raised. > > Now I *do* realize that our particular C language implementation(s) *do* > define the behavior here. If Rip is in the upper half of the address > space, we flip to negative (in two's complement representation), perform > the signed addition, then flip back to positive (which is *not* > implementation defined but standard-defined, but will do the right thing > here). > > But that's way too hard to follow if you actually want to pay attention > to the signed/unsigned conversions. We can do this without relying on > the implementation-dependent two's complement representation. Here's > what I suggest: > > RipRelative is an INT32, and may be negative. Consider the cast > > (UINT64)RipRelative > > If RipRelative is non-negative, then the value doesn't change (we'll > perform a plain increment). > > If RipRelative is negative, we'll get the following value from the cast, > mathematically speaking: > > (MAX_UINT64 + 1) - (-RipRelative) [*] > > which is just a different way of writing > > (MAX_UINT64 + 1) + RipRelative > > And the latter comes straight from the C standard, 6.3.1.3p2: > > Otherwise, if the new type is unsigned, the value is converted by > repeatedly adding or subtracting one more than the maximum value that > can be represented in the new type until the value is in the range of > the new type. > > Now consider what happens when we add [*] to Regs->Rip (which is itself > a UINT64): > > Regs->Rip + ((MAX_UINT64 + 1) - (-RipRelative)) > > Unpack the outer parens: > > Regs->Rip + (MAX_UINT64 + 1) - (-RipRelative) > > making for > > (Regs->Rip + (MAX_UINT64 + 1)) - (-RipRelative) > > The middle term falls away, per "6.2.5 Types", paragraph 9: > > [...] A computation involving unsigned operands can never overflow, > because a result that cannot be represented by the resulting unsigned > integer type is reduced modulo the number that is one greater than the > largest value that can be represented by the resulting type. > > Therefore we get: > > Regs->Rip - (-RipRelative) > > which is exactly what we want, for a negative RipRelative. > > (6) Thus, the return statement should be: > > // > // Negative displacement is handled by standard UINT64 wrap-around. > // > return Regs->Rip + (UINT64)RipRelative; > > (Technically, we could even drop the explicit (UINT64) cast -- > RipRelative would be converted automatically to UINT64 --, but we should > keep the (UINT64) cast for documentation purposes.) Impressive! I'll make all those changes. > >> + } >> + >> + switch (Ext->ModRm.Mod) { >> + case 1: >> + UpdateForDisplacement (InstructionData, 1); >> + EffectiveAddress += (INT8) (*(INT8 *) (InstructionData->Displacement)); > > Considering the patch as-is, the outer (INT8) cast is redundant. But, > that's not really my point. My point is how we should update this, after > changing the type of EffectiveAddress to UINT64: > > (7) Replace the outer (INT8) cast with (UINT64). > > EffectiveAddress += (UINT64) (*(INT8 *) (InstructionData->Displacement)); > > The reasoning is the same as for (6). If the displacement is negative, > the value we get on the right hand side is > > (MAX_UINT64 + 1) - (-Displacement) > > And when we add that to EffectiveAddress (also of type UINT64), the > (MAX_UINT64 + 1) term falls away, and we get > > EffectiveAddress - (-Displacement) > > (The UINT64 conversion would happen anyway, per the "usual arithmetic > conversions", given the new UINT64 type of EffectiveAddress; so the cast > is mainly for documentation, again.) > >> + break; >> + case 2: >> + switch (InstructionData->AddrSize) { >> + case Size16Bits: >> + UpdateForDisplacement (InstructionData, 2); >> + EffectiveAddress += (INT16) (*(INT16 *) (InstructionData->Displacement)); > > (8) Same as (7); please change the outer cast to (UINT64). > >> + break; >> + default: >> + UpdateForDisplacement (InstructionData, 4); >> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement)); > > (9) Same as (7); please change the outer cast to (UINT64). > >> + break; >> + } >> + break; >> + } >> + >> + if (InstructionData->SibPresent) { >> + if (Ext->Sib.Index != 4) { >> + EffectiveAddress += (*GetRegisterPointer (Regs, Ext->Sib.Index) << Ext->Sib.Scale); > > In the patch, as-is, we're left-shifting an INT64 that may be negative. > That's not defined by the standard; see "6.5.7 Bitwise shift operators", > p4: > > [...] If E1 has a signed type and nonnegative value, and E1 * 2^E2 is > representable in the result type, then that is the resulting value; > otherwise, the behavior is undefined. > > (10) Therefore we should do: > > INT64 Displacement; > > CopyMem (&Displacement, GetRegisterPointer (Regs, Ext->Sib.Index), > sizeof Displacement); > Displacement *= (1 << Ext->Sib.Scale); > // > // Negative displacement is handled by standard UINT64 wrap-around. > // > EffectiveAddress += (UINT64)Displacement; > > Assuming that the instruction we're decoding isn't malformed in the > first place, this is safe. > > (10a) The CopyMem could be replaced with > > Displacement = *(INT64 *)GetRegisterPointer (Regs, Ext->Sib.Index); > > but the CopyMem() is cleaner. (It is where we *explicitly* rely on two's > complement representation.) > > (10b) "Ext->Sib.Scale" is at most 3 (from DecodeModRm() below -- it > comes from a 2-bits wide bitfield), so left-shifting value 1 (of type > INT32) is OK. > > (10c) Multiplying a negative INT64 by 1, 2, 4, or 8 is well-defined > (assuming again that the initial Displacement value is small enough, > which depends on the original instruction). > > If we wanted to be super-safe, we could replace this open-coded > INT64 multiplication with a call to SafeInt64Mult(), from > , and hang here, if the call fails. > > Up to you. > > (10d) The final addition follows the same argument as above. We could > again drop the UINT64 cast (the INT64 operand would be converted to > UINT64 via the "usual arithmetic conversions"), but we should keep it > for documentation purposes. > >> + } >> + >> + if ((Ext->Sib.Base != 5) || Ext->ModRm.Mod) { >> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->Sib.Base); > > This will just continue working, with EffectiveAddress and > (*GetRegisterPointer()) being both UINT64's. A negative displacement > will be encoded within the register that (*GetRegisterPointer()) reads > out. > >> + } else { >> + UpdateForDisplacement (InstructionData, 4); >> + EffectiveAddress += (INT32) (*(INT32 *) (InstructionData->Displacement)); > > (11) Same as (9) -- please change the outer (INT32) cast to (UINT64), > for documentation. > >> + } >> + } else { >> + EffectiveAddress += *GetRegisterPointer (Regs, Ext->ModRm.Rm); > > Continues working fine. > >> + } >> + >> + return (UINTN) EffectiveAddress; > > (12) Please drop the cast. Ditto here. > >> +} >> + >> +/** >> + Decode a ModRM byte. >> + >> + Examine the instruction parsing context to decode a ModRM byte and the SIB >> + byte, if present. >> + >> + @param[in] Regs x64 processor context >> + @param[in, out] InstructionData Instruction parsing context >> + >> +**/ >> +STATIC >> +VOID >> +DecodeModRm ( >> + IN EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + SEV_ES_INSTRUCTION_REX_PREFIX *RexPrefix; >> + SEV_ES_INSTRUCTION_OPCODE_EXT *Ext; >> + SEV_ES_INSTRUCTION_MODRM *ModRm; >> + SEV_ES_INSTRUCTION_SIB *Sib; >> + >> + RexPrefix = &InstructionData->RexPrefix; >> + Ext = &InstructionData->Ext; >> + ModRm = &InstructionData->ModRm; >> + Sib = &InstructionData->Sib; >> + >> + InstructionData->ModRmPresent = TRUE; >> + ModRm->Uint8 = *(InstructionData->End); >> + >> + InstructionData->Displacement++; >> + InstructionData->Immediate++; >> + InstructionData->End++; >> + >> + Ext->ModRm.Mod = ModRm->Bits.Mod; >> + Ext->ModRm.Reg = (RexPrefix->Bits.BitR << 3) | ModRm->Bits.Reg; >> + Ext->ModRm.Rm = (RexPrefix->Bits.BitB << 3) | ModRm->Bits.Rm; >> + >> + Ext->RegData = *GetRegisterPointer (Regs, Ext->ModRm.Reg); >> + >> + if (Ext->ModRm.Mod == 3) { >> + Ext->RmData = *GetRegisterPointer (Regs, Ext->ModRm.Rm); > > Both of these UINTN field assignments will continue working, with > GetRegisterPointer() returning (UINT64*). > >> + } else { >> + if (ModRm->Bits.Rm == 4) { >> + InstructionData->SibPresent = TRUE; >> + Sib->Uint8 = *(InstructionData->End); >> + >> + InstructionData->Displacement++; >> + InstructionData->Immediate++; >> + InstructionData->End++; >> + >> + Ext->Sib.Scale = Sib->Bits.Scale; >> + Ext->Sib.Index = (RexPrefix->Bits.BitX << 3) | Sib->Bits.Index; >> + Ext->Sib.Base = (RexPrefix->Bits.BitB << 3) | Sib->Bits.Base; >> + } >> + >> + Ext->RmData = GetEffectiveMemoryAddress (Regs, InstructionData); >> + } >> +} >> + >> /** >> Decode instruction prefixes. >> >> @@ -411,6 +668,181 @@ UnsupportedExit ( >> return Status; >> } >> >> +/** >> + Handle an MMIO event. >> + >> + Use the VMGEXIT instruction to handle either an MMIO read or an MMIO write. >> + >> + @param[in, out] Ghcb Pointer to the Guest-Hypervisor Communication >> + Block >> + @param[in, out] Regs x64 processor context >> + @param[in, out] InstructionData Instruction parsing context >> + >> + @retval 0 Event handled successfully >> + @retval Others New exception value to propagate >> + >> +**/ >> +STATIC >> +UINT64 >> +MmioExit ( >> + IN OUT GHCB *Ghcb, >> + IN OUT EFI_SYSTEM_CONTEXT_X64 *Regs, >> + IN OUT SEV_ES_INSTRUCTION_DATA *InstructionData >> + ) >> +{ >> + UINT64 ExitInfo1, ExitInfo2, Status; >> + UINTN Bytes; >> + INTN *Register; > > (13) Please change this to (UINT64 *). > >> + UINT8 OpCode, SignByte; >> + >> + Bytes = 0; >> + >> + OpCode = *(InstructionData->OpCodes); >> + if (OpCode == 0x0F) { >> + OpCode = *(InstructionData->OpCodes + 1); >> + } > > (14) Can you add a comment regarding the 0x0F constant? I'll create a #define (TWO_BYTE_OPCODE_ESCAPE) that should (hopefully) be self commenting. > >> + >> + switch (OpCode) { >> + /* MMIO write */ > > (15) Please update the comment style. > > Also, can we be more explicit about the opcodes, with comments? Can do. > >> + case 0x88: >> + Bytes = 1; > > (16) Please add a "fall through" comment. For this and remaining comments: Will do. Thanks! Tom > >> + case 0x89: >> + DecodeModRm (Regs, InstructionData); >> + Bytes = (Bytes) ? Bytes > > (17) Please use an explicit (Bytes > 0) comparison. > >> + : (InstructionData->DataSize == Size16Bits) ? 2 >> + : (InstructionData->DataSize == Size32Bits) ? 4 >> + : (InstructionData->DataSize == Size64Bits) ? 8 >> + : 0; > > I struggled for a while to figure out what bothered me about this syntax > :) > > (18) The colons ":" should be at the ends of the lines. > > Bytes = ((Bytes > 0) ? Bytes : > (InstructionData->DataSize == Size16Bits) ? 2 : > (InstructionData->DataSize == Size32Bits) ? 4 : > (InstructionData->DataSize == Size64Bits) ? 8 : > 0); > > (I recommend the outermost parens only for supporting the indentation.) > >> + >> + if (InstructionData->Ext.ModRm.Mod == 3) { >> + /* NPF on two register operands??? */ > > (19) Please update the comment style. > >> + return UnsupportedExit (Ghcb, Regs, InstructionData); >> + } >> + >> + ExitInfo1 = InstructionData->Ext.RmData; >> + ExitInfo2 = Bytes; >> + CopyMem (Ghcb->SharedBuffer, &InstructionData->Ext.RegData, Bytes); >> + >> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; >> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2); >> + if (Status) { > > (20) please write (Status > 0) or (Status != 0) > >> + return Status; >> + } >> + break; >> + >> + case 0xC6: >> + Bytes = 1; > > (21) Please add a "fall through" comment. > >> + case 0xC7: >> + DecodeModRm (Regs, InstructionData); >> + Bytes = (Bytes) ? Bytes >> + : (InstructionData->DataSize == Size16Bits) ? 2 >> + : (InstructionData->DataSize == Size32Bits) ? 4 >> + : 0; > > (22) please see (17) and (18) > >> + >> + InstructionData->ImmediateSize = Bytes; >> + InstructionData->End += Bytes; >> + >> + ExitInfo1 = InstructionData->Ext.RmData; >> + ExitInfo2 = Bytes; >> + CopyMem (Ghcb->SharedBuffer, InstructionData->Immediate, Bytes); >> + >> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; >> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_WRITE, ExitInfo1, ExitInfo2); >> + if (Status) { > > (23) please write (Status > 0) or (Status != 0) > >> + return Status; >> + } >> + break; >> + >> + /* MMIO read */ > > (24) pls see (15) > >> + case 0x8A: >> + Bytes = 1; > > (25) Please add a "fall through" comment. > >> + case 0x8B: >> + DecodeModRm (Regs, InstructionData); >> + Bytes = (Bytes) ? Bytes >> + : (InstructionData->DataSize == Size16Bits) ? 2 >> + : (InstructionData->DataSize == Size32Bits) ? 4 >> + : (InstructionData->DataSize == Size64Bits) ? 8 >> + : 0; > > (26) please see (17) and (18) > >> + if (InstructionData->Ext.ModRm.Mod == 3) { >> + /* NPF on two register operands??? */ > > (27) Please update the comment style. > >> + return UnsupportedExit (Ghcb, Regs, InstructionData); >> + } >> + >> + ExitInfo1 = InstructionData->Ext.RmData; >> + ExitInfo2 = Bytes; >> + >> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; >> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2); >> + if (Status) { > > (28) please write (Status > 0) or (Status != 0) > >> + return Status; >> + } >> + >> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); >> + if (Bytes == 4) { >> + /* Zero-extend for 32-bit operation */ > > (29) Please update the comment style. > >> + *Register = 0; > > Continues working with Register having type (UINT64 *). > >> + } >> + CopyMem (Register, Ghcb->SharedBuffer, Bytes); >> + break; >> + >> + /* MMIO Read w/ zero-extension */ > > (30) Please see (15) > >> + case 0xB6: >> + Bytes = 1; > > (31) Please add a "fall through" comment. > >> + case 0xB7: >> + Bytes = (Bytes) ? Bytes : 2; > > (32) Please use an explicit (Bytes > 0) comparison. > >> + >> + ExitInfo1 = InstructionData->Ext.RmData; >> + ExitInfo2 = Bytes; >> + >> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; >> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2); >> + if (Status) { > > (33) please write (Status > 0) or (Status != 0) > >> + return Status; >> + } >> + >> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); >> + SetMem (Register, InstructionData->DataSize, 0); >> + CopyMem (Register, Ghcb->SharedBuffer, Bytes); >> + break; >> + >> + /* MMIO Read w/ sign-extension */ > > (34) Please see (15) > >> + case 0xBE: >> + Bytes = 1; > > (35) Please add a "fall through" comment. > >> + case 0xBF: >> + Bytes = (Bytes) ? Bytes : 2; > > (36) Please see (17) > >> + >> + ExitInfo1 = InstructionData->Ext.RmData; >> + ExitInfo2 = Bytes; >> + >> + Ghcb->SaveArea.SwScratch = (UINT64) Ghcb->SharedBuffer; >> + Status = VmgExit (Ghcb, SVM_EXIT_MMIO_READ, ExitInfo1, ExitInfo2); >> + if (Status) { > > (37) please write (Status > 0) or (Status != 0) > >> + return Status; >> + } >> + >> + if (Bytes == 1) { >> + UINT8 *Data = (UINT8 *) Ghcb->SharedBuffer; >> + >> + SignByte = (*Data & 0x80) ? 0xFF : 0x00; > > (38) Please use BIT7 (or if there's an even better dedicated macro, then > that), rather than 0x80. > > (39) Usual comment about bitmask used in logical context. > >> + } else { >> + UINT16 *Data = (UINT16 *) Ghcb->SharedBuffer; >> + >> + SignByte = (*Data & 0x8000) ? 0xFF : 0x00; >> + } > > (40) Same two comments as (38) and (39). > >> + >> + Register = GetRegisterPointer (Regs, InstructionData->Ext.ModRm.Reg); >> + SetMem (Register, InstructionData->DataSize, SignByte); >> + CopyMem (Register, Ghcb->SharedBuffer, Bytes); >> + break; >> + >> + default: >> + Status = GP_EXCEPTION; >> + ASSERT (FALSE); >> + } >> + >> + return Status; >> +} >> + >> /** >> Handle an MSR event. >> >> @@ -806,6 +1238,10 @@ VmgExitHandleVc ( >> NaeExit = MsrExit; >> break; >> >> + case SVM_EXIT_NPF: >> + NaeExit = MmioExit; >> + break; >> + >> default: >> NaeExit = UnsupportedExit; >> } >> > > Thanks! > Laszlo >