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.69]) by mx.groups.io with SMTP id smtpd.web12.12296.1602769083991605751 for ; Thu, 15 Oct 2020 06:38:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amdcloud.onmicrosoft.com header.s=selector2-amdcloud-onmicrosoft-com header.b=kXM673q6; spf=none, err=SPF record not found (domain: amd.com, ip: 40.107.223.69, mailfrom: thomas.lendacky@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=OZPgWsxKnOmHdwEhcZNuIkLKXNe0scnOD3Fwck+GWcgQFawjjB4WZN5jEPH1MaJ/gCCgkSTMZ/xPaVsACGD9eD3/uBfWyUfxMmv3SfjvZrsPmH7vLFg2burohYEZcCXWaPOy896MPEXAm1pPryuD/5rAWyu/BYksBaJXYTH6iPyPToRMheWXi+IbYeIUzkkwfpmnh2JOR24YIbb+ZXmJuU5MAI7/i6BPQsx2Tx0KVGgtiJvDR/NEqMHNfGWMAmWNZATiRjOFjgw3ks4Wwykrxzv+sJFKCK+z9V//BZkgcO8KzXW1tgc20mUYbGMRESYI3MkzBpMzQHaC+6MZMSVISA== 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=yh57OHMj64fxG4g4BN6Icn8qurhtYMMjWRNZmlbv7Bg=; b=ZQ02Kf2XSZGGsub3YRexXE2V2HnDCUlEkeOfRiV/AL6yI1PP4pBT7DRuDHi3xtkPQMvXdDnBtBjZa1ribQbPuoocvkkhS2TXRTvgU1YU0QaCb/7G6aFYV1JZZkP3rba47gVzjP1qQDLfQhSthzoDoUAzoE34ij3bHOS6mpSl/NnkhdrbOwwiJtTHPORpVlgwhugztnrm4lrRQ6NLe8iY7qQr0VEdyH9UgeNNX5JA8IeGuOHG5FWvI6BIYga7183H5BJs6t6hQBhGQ0v4tyWeJluQQ5Rw57HuyYMzTSILE8/frmcfoID3ARZVvLT31/kAB/5oXID3ZaO47cpqgjJpiQ== 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=yh57OHMj64fxG4g4BN6Icn8qurhtYMMjWRNZmlbv7Bg=; b=kXM673q6opODk+fleJNh6OSt84xB1BwqjV3aXfm8SF0zZlNdnZZOTtp6OhEsWf6W5FyuL9mzXW1DJqhop369c8cmknEpW5DG4qaQ+QeZ5h8gwgX6CvXyKK8P3RQ9woIyIpijrYbqsM6nQGu1Fd8OdSbNlCnoeN5o4x8TFt+a2QQ= 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 DM6PR12MB3179.namprd12.prod.outlook.com (2603:10b6:5:183::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3455.26; Thu, 15 Oct 2020 13:38:01 +0000 Received: from DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4d88:9239:2419:7348]) by DM5PR12MB1355.namprd12.prod.outlook.com ([fe80::4d88:9239:2419:7348%2]) with mapi id 15.20.3455.030; Thu, 15 Oct 2020 13:38:01 +0000 Subject: Re: [PATCH 1/9] OvmfPkg/VmgExitLib: Update ValidBitmap settings To: Laszlo Ersek , devel@edk2.groups.io Cc: Brijesh Singh , Michael D Kinney , Liming Gao , Zhiguang Liu , Jordan Justen , Ard Biesheuvel References: <2f5fbbbe7183109a3ec28f17f0810032476ddbc3.1602346027.git.thomas.lendacky@amd.com> From: "Lendacky, Thomas" Message-ID: <457a27c4-2319-c7a5-ee14-0ecf61bd1a6f@amd.com> Date: Thu, 15 Oct 2020 08:37:59 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 In-Reply-To: X-Originating-IP: [67.79.209.213] X-ClientProxiedBy: SN6PR01CA0002.prod.exchangelabs.com (2603:10b6:805:b6::15) 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 SN6PR01CA0002.prod.exchangelabs.com (2603:10b6:805:b6::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3477.20 via Frontend Transport; Thu, 15 Oct 2020 13:38:00 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: 2386f7e3-8da9-48fc-207b-08d8710f8935 X-MS-TrafficTypeDiagnostic: DM6PR12MB3179: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:8882; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: z4xH3+QJ6FsdjPISRzNA5seWvILjGMWP2vI/TFEy4vihsM1ClRlhGX1nyp+EEjjzi8KbKHXN6ONE+IEi3271vCXDQQ3Vk60qqsVinh/wr7atP2f+3Yb+n1puEMK0qc33emnEzouR2O75Lau218Zi8rUHmcCc0/Ozbe87TuYToAs2UxKEyYenoeXaEpJk1xBQKObbcTEw1XwEZg1FsFftV3eJlXL4aCIFaaDmz+MsFdmINymxYZGYYzHw+41IJhayTIXnO5vtoRQfHwTKLWvaSyS8xTYEMQNFXZNjtwcB5tWOF9jRAhw6pO8NH0jxVZDGjKO/wdnWmVvA9Z3o+bNYHsNqTZj/M7rZVKQSL67qCW9Cz+5X6NG4VsyA+W0MA7QCcNM/qCoy/9u2M7CIf9McIldhCClVmWhteW2mMYN1RN4= 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;SFS:(4636009)(136003)(396003)(346002)(376002)(39860400002)(366004)(478600001)(83380400001)(8676002)(5660300002)(31686004)(66556008)(8936002)(6486002)(66946007)(66476007)(36756003)(34490700002)(19627235002)(52116002)(6506007)(53546011)(54906003)(316002)(31696002)(86362001)(4326008)(2616005)(956004)(6512007)(186003)(16526019)(2906002)(26005)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: e2pU5jXA9WwzPQy3NXWftmQMhDJ8msxhHPJnigpDpgN9oES2F6VXdpV3CdKuJs5Fm//3Z7g2wGphru9P7/Lgu0aJ7TvRh6ByHpz6oGfsd/hdX/n5mR5G28B6XkrdC0bP+wqx1W0iySaetkvsFkX6TCQpm2LUny+K1R5i9DoNa9bbPhoD8ZdzozbBgClIy+8iB6lnwOt5BY6aq09SKLFVQl2URvr7XXKo6wKYjqoJ1jHCrECdgxint52ePF//Rv2ggPS8H3na6yOV/bO2qzyW4803yuiZaWPB6D6UrV60OgkiKeNNdrJeTqXwmm12iFAOBUJeivt6sMob1CA2V9+9TH7ZaGI/JNBGy66h4F0WGWLJCpVcZzPXVFIbf63Dl2hAk2CjfY5m2kle2+aJf4QLKW/JnMARA1RCzPVhqRj0AcSBAV8seDCxutH+wp2GuoFXu1amENSQzKnFJn8nFayNW5QMBZ83Zdtrw1m1cQbQ1OLmuXpon+r830Ud3YhH2oFeGQgRyv9bW5nqw2/CQ0cPNiF63u+upLe5lffd0Z6DzDQCxgrwEGxvgZ5TVyoZzBEW3PBBap5M3uJGdq2HOZIr4v5YP1zUdU7pcyF6/DwsTCSa597pryQE1H64L5caSwuiqcbfkGGARqSwHfg6f5esHQ== X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 2386f7e3-8da9-48fc-207b-08d8710f8935 X-MS-Exchange-CrossTenant-AuthSource: DM5PR12MB1355.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Oct 2020 13:38:01.6502 (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: WWwaGB1RjACrbpSecR5SP+EfxO46DHIp1M2CNq5dxUjYbtuDorW+cXiv4eq3onQr/VLslJ2hSUdMen4KVcpy3g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB3179 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/15/20 3:40 AM, Laszlo Ersek wrote: > Hi Tom, Hi Laszlo, > > On 10/10/20 18:06, Tom Lendacky wrote: >> From: Tom Lendacky >> >> Use OFFSET_OF () and sizeof () to calculate the GHCB register field >> offsets instead of hardcoding the values in the GHCB_REGISTER enum. >> Rename GHCB_REGISTER to GHCB_QWORD_OFFSET to more appropriately describe >> the enum. While redefing the values, only include (and add) fields that >> are used per the GHCB specification. >> >> Also, remove the DR7 field from the GHCB_SAVE_AREA structure since it is >> not used/defined in the GHCB specification and then rename the reserved >> fields as appropriate. >> >> Cc: Michael D Kinney >> Cc: Liming Gao >> Cc: Zhiguang Liu >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Tom Lendacky >> Cc: Brijesh Singh >> Signed-off-by: Tom Lendacky >> --- >> MdePkg/Include/Register/Amd/Ghcb.h | 48 ++++++++------------ >> OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c | 4 +- >> 2 files changed, 20 insertions(+), 32 deletions(-) > > This patch modifies both MdePkg and OvmfPkg. I agree that, as an > exception, this is the right thing to do. > > That's because we are not changing the identifiers of the enumeration > constants (such as GhcbCpl). Because those identifiers are declared at > file scope, having both GHCB_REGISTER and GHCB_QWORD_OFFSET declare > (e.g.) GhcbCpl would cause a compilation failure. Therefore we can't > implement this in multiple stages (first introduce GHCB_QWORD_OFFSET, > then remove GHCB_REGISTER separately). > > (1) However, the subject line doesn't look correct. It should mention > both MdePkg and OvmfPkg. Also, we're not updating ValidBitmap settings > just yet. > > I suggest: > > MdePkg, OvmfPkg: clean up GHCB field offsets and save area Yup, I'll fix this up. > >> >> diff --git a/MdePkg/Include/Register/Amd/Ghcb.h b/MdePkg/Include/Register/Amd/Ghcb.h >> index 54a80da0f6d7..33c7e8939a28 100644 >> --- a/MdePkg/Include/Register/Amd/Ghcb.h >> +++ b/MdePkg/Include/Register/Amd/Ghcb.h >> @@ -82,50 +82,25 @@ >> #define IOIO_SEG_DS (BIT11 | BIT10) >> >> >> -typedef enum { >> - GhcbCpl = 25, >> - GhcbRflags = 46, >> - GhcbRip, >> - GhcbRsp = 59, >> - GhcbRax = 63, >> - GhcbRcx = 97, >> - GhcbRdx, >> - GhcbRbx, >> - GhcbRbp = 101, >> - GhcbRsi, >> - GhcbRdi, >> - GhcbR8, >> - GhcbR9, >> - GhcbR10, >> - GhcbR11, >> - GhcbR12, >> - GhcbR13, >> - GhcbR14, >> - GhcbR15, >> - GhcbXCr0 = 125, >> -} GHCB_REGISTER; >> - >> typedef PACKED struct { >> UINT8 Reserved1[203]; >> UINT8 Cpl; >> - UINT8 Reserved2[148]; >> - UINT64 Dr7; >> - UINT8 Reserved3[144]; >> + UINT8 Reserved2[300]; >> UINT64 Rax; >> - UINT8 Reserved4[264]; >> + UINT8 Reserved3[264]; >> UINT64 Rcx; >> UINT64 Rdx; >> UINT64 Rbx; >> - UINT8 Reserved5[112]; >> + UINT8 Reserved4[112]; >> UINT64 SwExitCode; >> UINT64 SwExitInfo1; >> UINT64 SwExitInfo2; >> UINT64 SwScratch; >> - UINT8 Reserved6[56]; >> + UINT8 Reserved5[56]; >> UINT64 XCr0; >> UINT8 ValidBitmap[16]; >> UINT64 X87StateGpa; >> - UINT8 Reserved7[1016]; >> + UINT8 Reserved6[1016]; >> } GHCB_SAVE_AREA; > > (2) The meaning of a field called "ReservedN" must never change (it must > describe the same offset range in the structure, after introduction). If > we need to change the structure incompatibly, then please remove the old > ReservedN field name altogether. If a new reserved field has to be > introduced, in addition, then please find the largest N used with > Reserved fields in the structure currently, and use (N+1) in the name of > the new field. > > This practice makes sure that any (out of tree) code that refers to a > ReservedN field in MdePkg will cleanly break during compilation after > this change. Changing the meaning of a ReservedN field could otherwise > cause misbehavior that could be harder to track down. > > I realize this is somewhat unusual -- after all, if someone deliberately > accessed a field called "Reserved", any subsequent breakage is *their* > fault. However, the reason for this practice is that sometimes MdePkg > headers lag behind industry specs (meaning, non-UEFI specs), but drivers > or libs (outside of MdePkg or even edk2) already need to refer to > newly-specified fields. Even edk2 used to have code similar to the example > > Reserved3[12] = 1; > > Normally I don't advocate accommodating out-of-tree code, but following > this ReservedN scheme isn't hard. Ok, will do. I'll change the old Reserved2/Dr7/Reserved3 fields to a new combined Reserved8 field. Thanks, Tom > > >> >> typedef PACKED struct { >> @@ -136,6 +111,19 @@ typedef PACKED struct { >> UINT32 GhcbUsage; >> } GHCB; >> >> +typedef enum { >> + GhcbCpl = OFFSET_OF (GHCB, SaveArea.Cpl) / sizeof (UINT64), >> + GhcbRax = OFFSET_OF (GHCB, SaveArea.Rax) / sizeof (UINT64), >> + GhcbRbx = OFFSET_OF (GHCB, SaveArea.Rbx) / sizeof (UINT64), >> + GhcbRcx = OFFSET_OF (GHCB, SaveArea.Rcx) / sizeof (UINT64), >> + GhcbRdx = OFFSET_OF (GHCB, SaveArea.Rdx) / sizeof (UINT64), >> + GhcbXCr0 = OFFSET_OF (GHCB, SaveArea.XCr0) / sizeof (UINT64), >> + GhcbSwExitCode = OFFSET_OF (GHCB, SaveArea.SwExitCode) / sizeof (UINT64), >> + GhcbSwExitInfo1 = OFFSET_OF (GHCB, SaveArea.SwExitInfo1) / sizeof (UINT64), >> + GhcbSwExitInfo2 = OFFSET_OF (GHCB, SaveArea.SwExitInfo2) / sizeof (UINT64), >> + GhcbSwScratch = OFFSET_OF (GHCB, SaveArea.SwScratch) / sizeof (UINT64), >> +} GHCB_QWORD_OFFSET; >> + >> typedef union { >> struct { >> UINT32 Lower32Bits; >> diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c >> index 8e42b305e83c..c5484a3f478c 100644 >> --- a/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c >> +++ b/OvmfPkg/Library/VmgExitLib/VmgExitVcHandler.c >> @@ -153,7 +153,7 @@ STATIC >> BOOLEAN >> GhcbIsRegValid ( >> IN GHCB *Ghcb, >> - IN GHCB_REGISTER Reg >> + IN GHCB_QWORD_OFFSET Reg >> ) >> { >> UINT32 RegIndex; >> @@ -179,7 +179,7 @@ STATIC >> VOID >> GhcbSetRegValid ( >> IN OUT GHCB *Ghcb, >> - IN GHCB_REGISTER Reg >> + IN GHCB_QWORD_OFFSET Reg >> ) >> { >> UINT32 RegIndex; >> > > Thanks, > Laszlo >