From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (NAM11-BN8-obe.outbound.protection.outlook.com [40.107.236.53]) by mx.groups.io with SMTP id smtpd.web08.10629.1620155259690878633 for ; Tue, 04 May 2021 12:07:40 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@amd.com header.s=selector1 header.b=ZL6OrFBK; spf=permerror, err=parse error for token &{10 18 %{i}._ip.%{h}._ehlo.%{d}._spf.vali.email}: invalid domain name (domain: amd.com, ip: 40.107.236.53, mailfrom: brijesh.singh@amd.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=AV6fplHYIs9MaHqkA4gyvdL5ooyoG+I/n9GOI3pDLtaMVu09fEhAdpMsKBKaicRaPt/98nOy8zTqaR9sYkx7Yl72mcwGkaQY85NyOIhDSHb0W7E08kLtJ9LnUvtJXGBEyqKVjdfzfgeQJ5tOVJ0jKB8Yd3fnph/nfz1PxcNcUT/n40C6UMSgOvz+X6lcLrSXiyu9eO3r8Vw+qWvy0q/hJhbZxdk3r2DWcHGJYGd4aVxAKT57gAccaoI8NNgFa8EbBvU51Qr0iYOnpalHGR65lrrnPii/+masgB6BFiemYDHWd1tp63+tjka7pHQuEoz1TQTX4tDoeI133DB7ZV9c9w== 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=PmT7iYBgz7KTistv6un7VSgiJvmIb5nck2PHcwZ3LVI=; b=C49+FZ8HcnvTs/t2/02NSs8h9p1NO79IkfbEGGNggK1aijnhiT+Qe1k5i9S8eT49vMCYc1r7iN+0Qn/9PsDc29DD82CbBuE2zsCEJcjAZ5cw+A4QZwE3f2OhAEAl76oEFSw7k7l2ldGNaPIfJSZB+bwubEkhu0SLU76Jb9kLyEpOhw1FlvJzUdRvKY05Cz+3Q4FpPxMXaaGxjMrb0ePBY9wqXFvoZJOJ//8vSi070joQjn1PHW9hualPaG+sl7i/prEdaAum933PiSJSCGSqhs4cRZQzwp+uJWmUkYwGPZ/0eC2itgHBtfJui6RzfMPyIhoXUhdU1JqiU4BtT6KidQ== 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=amd.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=PmT7iYBgz7KTistv6un7VSgiJvmIb5nck2PHcwZ3LVI=; b=ZL6OrFBKpJ+zRZELh+WKw8wrfgV0nYg3TVCZ9pIOc+c866wFV3CxTum6lE2J822i9r4csn3nwirq6PfSqUDJzgfCwyrvkopxS7aR9MgTzzjHt5yYzufvWoe528K+SdpMQ60xkEKJepB+2viHRqE11xr74a+YQKpPHtBmpw24bBE= Authentication-Results: google.com; dkim=none (message not signed) header.d=none;google.com; dmarc=none action=none header.from=amd.com; Received: from SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) by SN6PR12MB4671.namprd12.prod.outlook.com (2603:10b6:805:e::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4087.38; Tue, 4 May 2021 19:07:36 +0000 Received: from SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::9898:5b48:a062:db94]) by SN6PR12MB2718.namprd12.prod.outlook.com ([fe80::9898:5b48:a062:db94%6]) with mapi id 15.20.4108.025; Tue, 4 May 2021 19:07:36 +0000 Cc: brijesh.singh@amd.com, James Bottomley , Min Xu , Jiewen Yao , Tom Lendacky , Jordan Justen , Ard Biesheuvel , Erdem Aktas Subject: Re: [edk2-devel] [PATCH RFC v2 05/28] MdePkg: Add AsmPvalidate() support To: Laszlo Ersek , devel@edk2.groups.io References: <20210430115148.22267-1-brijesh.singh@amd.com> <20210430115148.22267-6-brijesh.singh@amd.com> From: "Brijesh Singh" Message-ID: <2d5e8530-7433-f27e-3a89-9a9ce49bcc08@amd.com> Date: Tue, 4 May 2021 14:07:33 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.10.0 In-Reply-To: X-Originating-IP: [70.112.153.56] X-ClientProxiedBy: SN4PR0501CA0096.namprd05.prod.outlook.com (2603:10b6:803:22::34) To SN6PR12MB2718.namprd12.prod.outlook.com (2603:10b6:805:6f::22) Return-Path: brijesh.singh@amd.com MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from Brijeshs-MacBook-Pro.local (70.112.153.56) by SN4PR0501CA0096.namprd05.prod.outlook.com (2603:10b6:803:22::34) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4108.8 via Frontend Transport; Tue, 4 May 2021 19:07:35 +0000 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 30e6a8b5-3dcb-4be6-99a8-08d90f2fe10b X-MS-TrafficTypeDiagnostic: SN6PR12MB4671: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:7691; X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: po/0Kt3klm5KGdzXtmbVBnhTWVq6Eb3yjNukxc43c07J3H58gQTs3B22y4BeFCR75koD9yDgrrAzUVfV6W8iVgOlM6K4JTDQ+zsRzxEA2lnGsPt8H3R89CTQsMuMPa1ZZXLXNOzwrWIcrsK7vmK6+yYckQ+vapRRldhJo2iALcrufqZ6wvWnd4/rIZ25krheSfBuNBhSNJpEFZ5qvT3PR9utcFRNxr8at1CVARArFZXbIpFjUa5mMESaVyBnOOs2uSMWxpQVI3U3VgRAu5jn8if9RHYT6vHp3WfAZml0Z3kfB8RyP6q4ROEAUGkYQDwDqFldKjeDAY9Kc6hi/Y0NJEHSyUXZclyWxTlIJQirl+hIVUb72hXtuksYpfoJsQ5ace/dmCmneMSvlDXXK8IPswSNXCN1VWEXbxpm3kAYpRsFMBTKN+NkwMEfbXIx99qidbM85dVjQdRMGiXmOZyX7kw1rWntCtqT2wqxYbiFXw5HJcu7WUXcHWVHUapyFJuKWkaL2poEnjvQ35Z8UHFqkqRg/grLLlujsMIa3DUoQKuEmAbVjM/S1uNd194zwvTFF84czgbRIxceQFa0sz9fR/LcsOlwMgiTkDNlSckTighHHPuLUR82geBCZgWllMpQw4OegTaBnACLJk/W/VgF3RvA5zAVbt2fTcfsHqVhPJfPvp/izJ3voDoAXh+rNgzYg0sNnO1H0rVyx/a479Gu+r5TLiBGEPqtpjU7PwCmrRhibgbISEDZ5uOyrr409GeZVrrtYEydGBLjXGG97Prsj7vx/jghDSw24zmC/ks2xbyPzCJdcGBXj3uAAOhfLASM X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:SN6PR12MB2718.namprd12.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(346002)(366004)(136003)(376002)(396003)(39860400002)(38350700002)(38100700002)(53546011)(19627235002)(30864003)(86362001)(66556008)(66476007)(6506007)(66946007)(26005)(31686004)(16526019)(83380400001)(54906003)(52116002)(5660300002)(6486002)(36756003)(4326008)(8676002)(316002)(478600001)(44832011)(956004)(966005)(186003)(2906002)(45080400002)(31696002)(6512007)(2616005)(8936002)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-AntiSpam-MessageData: =?utf-8?B?ZDZqa3NEQTdScjJwdlZFM1VrMTh2NEMwSXN0bVdqUzBaV0QwMXFRdXRvZjY5?= =?utf-8?B?ZWpLbEJxd1JlNFE4TWZiZndXM3l0eGRRZFlxcXBDS0FjTk16ZHl1OTlvdHZy?= =?utf-8?B?WW1KWEVRek5CMlhvWFFwWHhocW1zU1E1VEh0Q2RaRGRZaWl3VmpNYkdLdVgr?= =?utf-8?B?S1RlU3hZUGk0OWtYME5FeDNXYnVINEdQWlFxdFpibmFGdkpFcnpJMkFPTTh3?= =?utf-8?B?Rlozd3pyZ2NsTjN1VkdFWmRvTkxaeUtsN1Q4NXJDYWFNRnZIN1NxWllnekpz?= =?utf-8?B?eHh0cytqWFlzREt6OTBkRXhDekJ0cUQ0U1FsMlNuOHM4TDc1OGNTMTMweUtx?= =?utf-8?B?a1doLytVWW1tcGkzL1MxcTNkOW5idVdOSlpyQWZZcndjdXhhVHZxQmFUc1VY?= =?utf-8?B?T29DdFR4WEpTcXp1bldjZE9BREpyMjVCYnU0Q3ArZlMxeE5GNEdrbklOK1Zh?= =?utf-8?B?L3ZiUU5RSVF0bDlibkFrNFVkYlFBbFQrem9nclovNDFjWDBIQ1lBZlZzUTFI?= =?utf-8?B?bWRidkZUbkZNTlFKNnNJWlF6Q1lYNGpqTWFvZDN2SC9SekdrK3llK1VnVy80?= =?utf-8?B?dWtVd0NvK1FoVmcxc2pndEJKcDFwTll4RDVjSDFHWVpGTzQzMndFY0syVVh6?= =?utf-8?B?dml0T0V5eHJBVEpvOFdEVEkwR0NXNHRsemVTcjF2QzdkcW40emF2MjU5RW4x?= =?utf-8?B?bUZlTE40a2ZyWjhTUC9nNDBYNStmVEdJeU9WNDVNYmZUSjl0TzkxSVord3hv?= =?utf-8?B?ellsU1E4MUorajdSNjY4Z3MzSWdaWFV0YlIzcXZ3UWc1ZEs4eXNSUXV4ZnlQ?= =?utf-8?B?cjRQSUp6Y1k3ZDRFNXBsV1drREhrYUF6QTM3NlZkMEVVTnhZUThKQmtLVUY2?= =?utf-8?B?OCtpL2xIOWVXTFc1cWhLSWc3MTNrRTc2MTZ1NEY4dGhNck5aS1ZoREFTbWY0?= =?utf-8?B?ekNkNFkyRmZjK3NzMnlrT1ZzRXVHQW0ybjVDV202K2pUSVNvWVpHalh4cnhH?= =?utf-8?B?NXBoWWJacWFuYjh3Z0wxclNDUnl2ckdvRUl3a1dwSUJlbHljTlUrbE5Ub0J4?= =?utf-8?B?SGdLaDZ2bEVRZnpUWVFuWUQwNzFSWFAyRy95cThMUnpBVFFBNTB5Qko1TjRq?= =?utf-8?B?aTFOUzZta0daOFVtWW1GVlVzbVV0eWJkNEVDVDBEektBcnJTS2hFU1BxOTNO?= =?utf-8?B?ZGFTRTloK0k4dVYwdm44UnJkVzhwdzg5U29scW4wVTR3dVIvdk1PL09FT1BE?= =?utf-8?B?VHdPVUtPRzdRTzJyS0RDdnRhRjZwaEJnSXRLcGJ4dWh2aitUNTd6RFVWRFhq?= =?utf-8?B?U1pSYW9WUDVsMnFMdGhMWGVicDRoa1JqWVc0UXNKYjZpMGlTTFp0cWgyK2NK?= =?utf-8?B?S0xzWTZ4ZFRTV2VMcUFtQW1zR2JMS2VzYXdYc3ZoSUlLQnBYZ2lzNkJTanVJ?= =?utf-8?B?TEQrYXM4VUxTdVJJYkdQaHZ2dHYyM3RwaEdrTWY0ZmJDS21GdGpNUlR0NG1M?= =?utf-8?B?Ri9WSW1NRk5kdFVtaEcxZzNZMUd5MkdOZkl1eTFUUi9iR3laLzAzakxOYWhR?= =?utf-8?B?U1JETkQvYmNqZ3VTcDdVS0Rvb2k3WEhsbXdkU212ZER3S3JmcmlpVTMzOVI0?= =?utf-8?B?WS9PTi9MYnNCdFhBM0pZT3VVaWtkak5tdStNVStOQW9HNlh3OUpIZG9Dd3VT?= =?utf-8?B?bFlDbWpCNDNsd3I0bnBsdnlWVTh3a0JpYnBTUkJtSWR3dDhJNjM2QjlzaGZT?= =?utf-8?Q?TZO0g8AhfYaiiKs4ZT5mTrtuPamC8oFb0NnqKtZ?= X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: 30e6a8b5-3dcb-4be6-99a8-08d90f2fe10b X-MS-Exchange-CrossTenant-AuthSource: SN6PR12MB2718.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 04 May 2021 19:07:36.4548 (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: ommz3QpiNG83CbDCkC3rBPgbMFkhakSyOjR++WvXcL/jFQWQxrHbrRgKaPYobAB0AvcESh2iD45vGJ2T8yYTjw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR12MB4671 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US On 5/4/21 8:58 AM, Laszlo Ersek wrote: > On 04/30/21 13:51, Brijesh Singh wrote: >> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&data=04%7C01%7Cbrijesh.singh%40amd.com%7C6ceeec6c984d468bb87908d90f04b789%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637557335220626621%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=jF44vk%2FBUVukpuNg4vrcRqHnEa7nO%2FGPEe9ti720cnE%3D&reserved=0 >> >> The PVALIDATE instruction validates or rescinds validation of a guest >> page RMP entry. Upon completion, a return code is stored in EAX, rFLAGS >> bits OF, ZF, AF, PF and SF are set based on this return code. If the >> instruction completed succesfully, the rFLAGS bit CF indicates if the >> contents of the RMP entry were changed or not. >> >> For more information about the instruction see AMD APM volume 3. >> >> Cc: James Bottomley >> Cc: Min Xu >> Cc: Jiewen Yao >> Cc: Tom Lendacky >> Cc: Jordan Justen >> Cc: Ard Biesheuvel >> Cc: Laszlo Ersek >> Cc: Erdem Aktas >> Signed-off-by: Brijesh Singh >> --- >> MdePkg/Include/Library/BaseLib.h | 37 +++++++++++++++++ >> MdePkg/Library/BaseLib/BaseLib.inf | 1 + >> MdePkg/Library/BaseLib/X64/Pvalidate.nasm | 43 ++++++++++++++++++++ >> 3 files changed, 81 insertions(+) >> >> diff --git a/MdePkg/Include/Library/BaseLib.h b/MdePkg/Include/Library/BaseLib.h >> index 7253997a6f..92ce695e93 100644 >> --- a/MdePkg/Include/Library/BaseLib.h >> +++ b/MdePkg/Include/Library/BaseLib.h >> @@ -7518,5 +7518,42 @@ PatchInstructionX86 ( >> IN UINTN ValueSize >> ); >> >> +/** >> + Execute a PVALIDATE instruction to validate or rescnids validation of a guest > (1) typo: "rescnids" Noted. > > >> + page's RMP entry. >> + >> + Upon completion, in addition to the return value the instruction also updates >> + the eFlags. A caller must check both the return code as well as eFlags to >> + determine if the RMP entry has been updated. >> + >> + The function is available on x64. > (2) Please write "X64"; that's how the architecture is usually mentioned > in both the UEFI spec and in edk2. Noted. > > >> + >> + @param[in] Address The guest virtual address to validate. >> + @param[in] PageSize The page size to use. >> + @param[i] Validate Validate or rescinds. >> + @param[out] Eflags The value of Eflags after PVALIDATE completion. > (3) Typo: "[i]" should be "[in]". Noted. > > > (4) The order of parameters listed in this comment block differs from > the actual parameter list. > > The ECC plugin of the edk2 CI will catch this issue anyway. So, before > submitting the patch set to the list, please submit a personal PR on > github.com against the main repo, just to run CI on your patches. > Interestingly I did ran the series with edk2 CI and everything passed before I submitted for the review. But, I will go ahead and fix the order. >> + >> + @retval PvalidateRetValue The return value from the PVALIDATE instruction. > More on the return value / type later, below. > >> +**/ >> +typedef enum { >> + PvalidatePageSize4K = 0, >> + PvalidatePageSize2MB, >> +} PVALIDATE_PAGE_SIZE; >> + >> +typedef enum { >> + PvalidateRetSuccess = 0, >> + PvalidateRetFailInput = 1, >> + PvalidateRetFailSizemismatch = 6, >> +} PVALIDATE_RET_VALUE; >> + > (5) These typedefs do not belong between the function leading comment > and the function declaration. Please hoist the typedefs just above the > leading comment, and add a separate comment for the typedefs -- using > the proper comment style for typedefs, too. > > >> +PVALIDATE_RET_VALUE > (6) In my opinion, using an enum for an EFIAPI function's return type is > problematic. According to the UEFI spec (v2.9), "Table 2-3 Common UEFI > Data Types", may correspond to INT32 or UINT32. I > don't like that ambiguity here. The spec also says that such types > should never be used at least as structure fields. > > I'm perfectly fine with functions in standard (ISO) C programs returning > enums, but I think the situation is less clear in UEFI. I don't recall > standard interfaces (spec-level, or even edk2 / MdePkg interfaces) that > return enums. > > I suggest the following instead. Drop the PVALIDATE_RET_VALUE enum > altogether. Specify EFI_STATUS as the return type, in the declaration of > the function. Works with me. > > In the UEFI spec, Appendix D specifies the numeric values of the status > codes. Furthermore, there are examples for NASM sources using EFI_* > status codes *numerically* in edk2; minimally: > > - IntelFsp2Pkg/FspSecCore/Ia32/FspApiEntryT.nasm > - IntelFsp2WrapperPkg/Library/SecFspWrapperPlatformSecLibSample/Ia32/SecEntry.nasm > > Thus, please modify the assembly source code in this patch to return > EFI_SUCCESS (already value 0, conveniently) if the instruction succeeds. Works with me. > Return EFI_INVALID_PARAMETER (0x8000_0002) in case the instruction fails > with error code 1 (FAIL_INPUT). Works with me. > > Return EFI_UNSUPPORTED (0x8000_0003), or even EFI_NO_MAPPING > (0x8000_0017), for value 6 (FAIL_SIZEMISMATCH). I am not sure if we really want to do this. You will see later in the patches that in some cases the PVALIDATE will return a failure and we will need to know the failure code to determine the next steps. Especially this particular error code is used later. This error happens when the page size of the backing pages does not match with the pvalidated size. In those cases we need to retry the PVALIDATE with lower page size so that a validation succeed. One such a example is: - Guest ask hypervisor to add the page as 2M in RMP table. - Hypervisor added the page as 512 4K pages - because it was not able to find a large backing pages. - Guest attempts to pvalidate the page as a 2M. The pvalidate will return a failure saying its a size mismatch between the requested pvalidated and RMP table. The recommendation is that guest should try with a smaller page size. I would prefer to pass the pvalidate error as-is to caller so that it can make the correct decision. > > The leading comment block of the function is supposed to explain these > associations: > > @retval EFI_SUCCESS Successful completion (regardless of > whether the Validated bit changed state). > @retval INVALID_PARAMETER Invalid input parameters (FAIL_INPUT). > @retval EFI_UNSUPPORTED Page size mismatch between guest (2M) and > RMP entry (4K) (FAIL_SIZEMISMATCH). > > (Passing in the PVALIDATE_PAGE_SIZE enum, as a parameter, should be > fine, BTW) > > > (7) According to the AMD APM, "Support for this instruction is indicated > by CPUID Fn8000_001F_EAX[SNP]=1". > > Presumably, if the (physical, or emulated) hardware does not support > PVALIDATE, an #UD is raised. That condition should be explained in the > function's leading comment. (Mention the CPUID and the #UD, I guess.) Noted. > >> +EFIAPI >> +AsmPvalidate ( >> + IN PVALIDATE_PAGE_SIZE PageSize, >> + IN BOOLEAN Validate, >> + IN UINTN Address, > (8) This should be EFI_PHYSICAL_ADDRESS, not UINTN. Noted. > >> + OUT IA32_EFLAGS32 *Eflags >> + ); >> + >> #endif // defined (MDE_CPU_IA32) || defined (MDE_CPU_X64) >> #endif // !defined (__BASE_LIB__) > (9) Unless you foresee particular uses for eflags *other than* CF, I > would suggest replacing the Eflags output parameter with > > OUT BOOLEAN *RmpEntryUpdated > > The function would still only have 4 parameters, which shouldn't be > difficult to handle in the assembly implementation (i.e. write to the > UINT8 (= BOOLEAN) object referenced by "RmpEntryUpdated"). EFIAPI means > that the first four params are passed in RCX, RDX, R8, R9. Works with me, thats what I ended up doing for the guest kernel patches and will do the same for OVMF as well. > > Thus far, I can see only one AsmPvalidate() call: in IssuePvalidate(), > from patch #21 ("OvmfPkg/MemEncryptSevLib: Add support to validate > system RAM"). And there, CF looks sufficient. > > > (10) The instruction is X64 only, but you are providing the declaration > even if MDE_CPU_IA32 is #defined. That seems wrong; even the declaration > should be invisible in that case. Please declare the function for > MDE_CPU_X64 only. > Noted. >> diff --git a/MdePkg/Library/BaseLib/BaseLib.inf b/MdePkg/Library/BaseLib/BaseLib.inf >> index b76f3af380..d33b4a8f7d 100644 >> --- a/MdePkg/Library/BaseLib/BaseLib.inf >> +++ b/MdePkg/Library/BaseLib/BaseLib.inf >> @@ -321,6 +321,7 @@ >> X64/XGetBv.nasm >> X64/XSetBv.nasm >> X64/VmgExit.nasm >> + X64/Pvalidate.nasm >> ChkStkGcc.c | GCC >> >> [Sources.EBC] > (11) This list of source files is already not sorted alphabetically, > unfortunately. But we can still do better than this: I suggest inserting > "X64/Pvalidate.nasm" just before "X64/RdRand.nasm". Noted. > > (12) Your git setup seems less than ideal for formatting edk2 patches. > The @@ hunk header above does not show the INF file section being > modified. It should look something like this: > > @@ -317,6 +317,7 @@ [Sources.X64] > ^^^^^^^^^^^^^ > > Please run the "BaseTools/Scripts/SetupGit.py" script in your working > tree. Will do. > > Alternatively, please see "xfuncname" at > . > > This is of course not a bug in the patch, but fixing your setup will > help with the next round of review. > > >> diff --git a/MdePkg/Library/BaseLib/X64/Pvalidate.nasm b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm >> new file mode 100644 >> index 0000000000..f2aba114ac >> --- /dev/null >> +++ b/MdePkg/Library/BaseLib/X64/Pvalidate.nasm >> @@ -0,0 +1,43 @@ >> +;----------------------------------------------------------------------------- >> +; >> +; Copyright (c) 2020-2021, AMD. All rights reserved.
> (13) I believe we don't introduce new files with copyright notices > referring to the past. IOW, I think you should only say "2021" here. > Noted. >> +; SPDX-License-Identifier: BSD-2-Clause-Patent >> +; >> +; Module Name: >> +; >> +; Pvalidate.Asm >> +; >> +; Abstract: >> +; >> +; AsmPvalidate function >> +; >> +; Notes: >> +; > (14) I defer to the MdePkg maintainers on this, but "Module Name" is > plain wrong, and the Abstract is useless. Either fix those up please > ("Abstract" could be a copy of the corrected leading comment block), or > just drop them both. > > >> +;----------------------------------------------------------------------------- >> + >> + SECTION .text >> + >> +;----------------------------------------------------------------------------- >> +; PvalidateRetValue >> +; EFIAPI >> +; AsmPvalidate ( >> +; IN UINT32 RmpPageSize >> +; IN UINT32 Validate, >> +; IN UINTN Address, >> +; OUT UINTN *Eflags, >> +; ) >> +;----------------------------------------------------------------------------- > (15) Please update this accordingly to the corrected function > specification. Noted. > >> +global ASM_PFX(AsmPvalidate) >> +ASM_PFX(AsmPvalidate): >> + mov rax, r8 >> + >> + ; PVALIDATE instruction opcode >> + DB 0xF2, 0x0F, 0x01, 0xFF > This is bad practice; we make every effort to avoid DB-encoded > instructions. > > We have two PVALIDATE instances in the patch set (... that I can see > immediateyl); the first here, and the other in > "OvmfPkg/ResetVector/Ia32/PageTables64.asm" (from patch #17, > "OvmfPkg/ResetVector: Invalidate the GHCB page"). Therefore, hiding the > encoding of PVALIDATE behind a NASM macro definitely makes sense. > > (16a) Please file a NASM feature request for PVALIDATE at > . Will do. > > (16b) In the present MdePkg patch, please extend the file > > MdePkg/Include/X64/Nasm.inc > > as follows: > >> diff --git a/MdePkg/Include/X64/Nasm.inc b/MdePkg/Include/X64/Nasm.inc >> index 527f71e9eb4d..ff37f1e35707 100644 >> --- a/MdePkg/Include/X64/Nasm.inc >> +++ b/MdePkg/Include/X64/Nasm.inc >> @@ -33,6 +33,15 @@ >> DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8 >> %endmacro >> >> +; >> +; Macro for the PVALIDATE instruction, defined in AMD publication #24594 >> +; revision 3.32. NASM feature request URL: >> +; . >> +; >> +%macro PVALIDATE 0 >> + DB 0xF2, 0x0F, 0x01, 0xFF >> +%endmacro >> + >> ; NASM provides built-in macros STRUC and ENDSTRUC for structure definition. >> ; For example, to define a structure called mytype containing a longword, >> ; a word, a byte and a string of bytes, you might code Sure, I will add the new macro and use it. > (16c) Please replace the FIXME placeholder above with the actual NASM BZ > number (from (16a)). > > (16d) In the "MdePkg/Library/BaseLib/X64/Pvalidate.nasm" source file, > and also (later) in the "OvmfPkg/ResetVector/Ia32/PageTables64.asm" > source file, please use the PVALIDATE macro, in place of the naked DBs. > > > Back to your patch: > > On 04/30/21 13:51, Brijesh Singh wrote: >> + >> + ; Read the Eflags >> + pushfq >> + pop r8 >> + mov [r9], r8 >> + >> + ; The PVALIDATE instruction returns the status in rax register. >> + ret >> > (17) The assembly code should be updated to match the new interface > contract (parameter order, parameter types, return values). Sure, I will update based on your feedback. > > I'll continue reviewing the series later this week (hopefully tomorrow). thanks for all your review feedback.