From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0054.outbound.protection.outlook.com [104.47.34.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id F152B21CF25C1 for ; Thu, 3 Aug 2017 15:05:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector1-amd-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version; bh=1GlNFik3QKriQmWklB1eTG6PZAQxcr7waQis+0F6m7E=; b=kQ8h/11CYAF18Q7FLyCX98VAfsq2DCtqi1RVZQKaGaSkwoZ7PyKyWevhanSTZyR8hRu/prHWXvPekX72EDl+vFtohFxxtN+I8qmbtJ+NZTRdKUyi4IvWdP3M3U6LMyDdzoSSSyPN/JHhx5FT5+bl9s8mSxyidb4rNewZUdyEL64= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Received: from [10.236.136.62] (165.204.77.1) by SN1PR12MB0158.namprd12.prod.outlook.com (10.162.3.145) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1304.22; Thu, 3 Aug 2017 22:07:30 +0000 Cc: brijesh.singh@amd.com, Tom Lendacky , Jordan Justen , Ard Biesheuvel To: Laszlo Ersek , edk2-devel@lists.01.org References: <1501776600-16369-1-git-send-email-brijesh.singh@amd.com> <1501776600-16369-2-git-send-email-brijesh.singh@amd.com> <2517ecce-c92c-b85c-39a7-f454048e6b3d@redhat.com> From: Brijesh Singh Message-ID: Date: Thu, 3 Aug 2017 17:07:26 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <2517ecce-c92c-b85c-39a7-f454048e6b3d@redhat.com> X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: CY4PR03CA0106.namprd03.prod.outlook.com (10.171.242.175) To SN1PR12MB0158.namprd12.prod.outlook.com (10.162.3.145) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 8b748d31-29a1-4262-aa55-08d4dabc097c X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(48565401081)(300000503095)(300135400095)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:SN1PR12MB0158; X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 3:9JuT2X/zYZrdQ+gFUnh5YAR/0tpetWaqi3+NNWpre4m7TDE8JRSBG5yMh+UWE0juJAhty+oX4radyh3tqCp4M0OnByYZmUQVKoOxcynpjHXKMu/2/2fI8DsAW1OWE9oHZKJBTzBnUcaXjWeSuYAGkCeyMywUFfUSET1v+9xc7CPjg9syIwstte5GY2LAb9Ym/FH4vd6fPF63Wp9ZMwov2gZMCiHj7ekgdk0+9od67+TWBtdJFVxB0bnQSOdEb5tz; 25:X5lANJZLEhqR3m7wyQhexns9OuTxzU/vMDBQwLFCmhda84otPsNH9haWO1ge6K53Of/Avcuxo8W/NULX4/YH2GmRSJhsG/9JkqQAPu/95PAtDa3S6c3un4QcSpgD5da/YZPrmNiBEg41prkH7xIv63OhVPb1k/9RAT+AQ6qhCC3kPF9GOy9bkxzI7Xr7xoaLOn298DKuIhhGBp5crSZcyVCoGo9TRzTSlTiREEgzDpDsZnWYOtFilxoZKkNRM8eMFTSZXnL7v9jiR2SjkPH2gLCX2QMDXj+zcYjI9ZQMRQI/cXgljt8D1kjUDOM0WqtcEVpvK24lL8y2/ossp0rEfg==; 31:zoTnBDImnJmqPtEOtMQBaXo2I42y6SyK8guX6LbmDu6vHO/sFxMnXSLV8SWhfmHwqLlQ/WpfmRhyjPo25SH2aGQDh2jvUYjOAB+T7mHsxOTjJmPwQ9FAKPk0GpIKtaVp4O6/kX1P7Vbbzo5V3j/wSpmTD+LoCq/RUpWGBMNGlfNY7LkUpEZfAFAwhQ4Nd9xTWF+4mHMJhK+Jso0/tGlt4OMjoy0eq/MCBzRZDcwBphw= X-MS-TrafficTypeDiagnostic: SN1PR12MB0158: X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 20:icqLrX6HvqTx4AbQymznMK20DKRRsE5BDerWhfFLYg26AH++Fuy2+K35jnnN7+lZLQ65HChVPjpUAd5Mv0sGJoNVZNVZMd1SWVOmvt8shYAIxsTuqhv2k1Evu+ddcCMWPldaBWwfNUY2YoLBmq2mv9YyfSapwBn0Qtr94KMF82ZkhHG4u1GFm+3PwuYbiIQpNNPLg34Y9t/S4Xfw5ElqVNkbU2X4wObPh56Z1+lrAH62KYeBgiaWoLzAGv2YFSPyaL606lW9yJvZcJAlcVdvlXqbsgN8DjOY+TlZb1UqaqF6tRpfGCYFQGlp+/JOdmf1pstvGPffhVvZZX9mAqWpAW7fB8HTFxMiIU8TdytTRsUkWwZ6dMQd6nizfZIhq+TCBvF3fSNZyBoOelOwBTkeoataGi6yiVgHOXbZKvEW7WnqRHEYDEbxoXBdymW4MoGD4GfIwY/MDys1tLxs0/3+Ov1TPmijO12BIf0bVT5fVu+uj1aiek+O00GH3hzpGyHv; 4:e3uw9Qo087/VIC0QpBES+aKs4O8eCROy+cB0E5VxS/s6VHE3Wy3TiCZmvmyZol3STJ/aWV91jbsSBiiryDIo7t/Lb55tGpyEUvFUnz3fyvXBhybkhNUgOIFzPjDgPFEy8/K+cDVqhla1kuv7RG5XMWkodoaUIIaHcaCIUMRcNhVS6aA+djv5vU7PGz+noBkdJXkoTKH6FGWuPOZ8v7YYhyUpiyP7yE7E2nnkeg1RCLBEaSHWwfK/0ZhgOI/dBYqauh0pjtfQ/mhlMljvHNU3CoSr0Wsne7UlPIdVoupaTEkq0OtCauPdUzBCXnT7jMftQJFEhn+PZPKTCo42BOlw5Edg8OzqOJq3Th1ZByP/J6M= X-Exchange-Antispam-Report-Test: UriScan:(767451399110)(162533806227266)(228905959029699); X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-CFA-Test: BCL:0; PCL:0; RULEID:(100000700101)(100105000095)(100000701101)(100105300095)(100000702101)(100105100095)(6040450)(601004)(2401047)(5005006)(8121501046)(100000703101)(100105400095)(3002001)(10201501046)(93006095)(93001095)(6055026)(6041248)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123558100)(20161123555025)(20161123564025)(20161123560025)(6072148)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:SN1PR12MB0158; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:SN1PR12MB0158; X-Forefront-PRVS: 03883BD916 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(7370300001)(4630300001)(6029001)(6049001)(6009001)(39860400002)(39850400002)(39450400003)(39410400002)(39400400002)(39840400002)(199003)(24454002)(377454003)(52314003)(189002)(51914003)(47776003)(65806001)(33646002)(65956001)(66066001)(305945005)(2906002)(81166006)(54356999)(81156014)(50986999)(76176999)(53946003)(966005)(53936002)(4326008)(6306002)(25786009)(31696002)(54906002)(23676002)(101416001)(36756003)(64126003)(478600001)(68736007)(83506001)(31686004)(8676002)(4001350100001)(7736002)(86362001)(42186005)(189998001)(105586002)(229853002)(6486002)(53546010)(97736004)(77096006)(90366009)(2950100002)(3846002)(6116002)(38730400002)(6666003)(50466002)(110136004)(6246003)(7350300001)(65826007)(230700001)(106356001)(5660300001)(19627235001); DIR:OUT; SFP:1101; SCL:1; SRVR:SN1PR12MB0158; H:[10.236.136.62]; FPR:; SPF:None; PTR:InfoNoRecords; A:1; MX:1; LANG:en; Received-SPF: None (protection.outlook.com: amd.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtTTjFQUjEyTUIwMTU4OzIzOnpFclZjL0VMbzYrbFdPK2FQeG5VQkJ6NFg5?= =?utf-8?B?Q2dhU1BCRGxseTNHYzVnVXF5SG1EcGZiV2cvZXhYTnpzN3pndzNEd3dKWVN2?= =?utf-8?B?SHozUDVWZGxkb0U3bTRteVE5b0JzTm5memxNTVlPOTE3WWJpOUc3ZVk1QVlu?= =?utf-8?B?OThrTUtZZHNXWndHSXlTa2JTSUk5WnEyL0htQXV6Nmc4WGZBL2RHYlkrK2Fp?= =?utf-8?B?SVpDZ3hxMUNISVRyYkcwSHlnbDlVTkRvYXJNMFZQZ2p1VGV2dklFNGdGdmxp?= =?utf-8?B?Zld3WXBLZGpncmE3dzlad242RC9paUR4b29hZmhqYkFnb0hITXRqQ1ZWUjh0?= =?utf-8?B?aEE5eEV1ZmxiMmJ0bnBZNEhmc2doR3JMZEJNUnZpTWw1VEhQbFFrdnlscnBU?= =?utf-8?B?NXZ6NlhHNm5JSUh4eXZlSlVMbytQS2trTWdSNUE4eEFidUEzaWlSQmo3TXdp?= =?utf-8?B?ZTFxZ0grUHlzSUN4OGpFUGV3SVhUdk03TWFNTzZEc1Jpa1J1NU9qSTc2VHZz?= =?utf-8?B?YVVHaVlRUFFyNXNVbVdYVmhIM21qQWpvejBMRmczdjF1ZUxuRUI5enMxQUtQ?= =?utf-8?B?MUlUNUorTmhVekNpbXpkMzRhZVJEekxFc0l0MnRzNUdhbW5Hd1FpN0gwNXY3?= =?utf-8?B?OVNFTGlxT3RISGdqaDhHeUxPZnliV1FrV1BhWld1d2tlYmdkOERCSHI5bEdC?= =?utf-8?B?NUsySmczZlJZTDdZV3BOQ3FNNHpndmJHVnh1djB0Z3laVnYwaUxDSUZCbTFQ?= =?utf-8?B?M0NEaGpSUFlscW12R3dlRjI2NFJNdVFET1RXaUZrUnUxOVdKQTJBZHp1eVU3?= =?utf-8?B?OFRkRHRtck1WZzBFYWMvc3FodUV6c2xVaWxydzRodThCU0VqdXB2WGU4WVp2?= =?utf-8?B?YmNVYjF5Q0NZb3JvRTMxalc1OW92ckx0MzcxNFA2Y3RwbzdKY0tiLzNCVmZE?= =?utf-8?B?R3E5SEg2azg1ZndjbE5JdXpIN2lVVkFvdjlOVzJwNVdubjRBbkNMbVBVOFRi?= =?utf-8?B?NVdoRGZMV1gwbnd3NnpwM1d4YnErN0VIY0tzZTRzTDRVekx6aXB5UGFKeGN2?= =?utf-8?B?WGVvdjQxNXNTT1lCR0pQMmJUQ1VsbHZ2U3p6STkzTW0zaXdFQkxwdVNqZEVa?= =?utf-8?B?dG5vdGtuK0NkWXhpSWt2Y016Z0VsckFKODlxc0NFY3FFZzZEZ2M1c29qZ08v?= =?utf-8?B?Tmx1b1RmeW9nQ1R2djM4ZTZPZmdFV2c5MmJ2UUhsTHQydjN2VzVMSlF5cXIr?= =?utf-8?B?cmhsNmhTQzNHdnBRUzhDRi9JR2dRemh0OGgyY3FhUlhhS0FCMksrckRLUWpG?= =?utf-8?B?clhwMEw4b09aOUk5Sm1kMGppa243cHRQK2NEOE8xdjFjSHVJSmp2aDAwa1dr?= =?utf-8?B?dVJRcm9BOU5kcHJGQk5qcEdpY1ArRlV1Z3pXR0xrNEhvWkhvdnAvV3ZyTHA5?= =?utf-8?B?WmhUVTVKOWFGYnZkbXBkTkdKNWlkcmtwRnVpMFExWENzeTdoWnVLcndGNGty?= =?utf-8?B?SlR4MDFONUJSMnUvZEI3TGhMUGdHQXZiUzJsZ0ttdElOSGJwcWZZbi9SaVU0?= =?utf-8?B?cTAyRzhMc0Q0dFY1UzZxL3lOMmovRDhtS2pORElpakJUK2owN1FWbjRVKzcw?= =?utf-8?B?TTMvU0h3UWtKLzMwMG4zVUVxa09NQ3lYM1p1WlE3RUE2a01vY2VZZ2M1NkdH?= =?utf-8?B?TW5tcm5aMVVIZ3oyQ0ZaTmNiWUJjdmFsVzF6cnhOTXU5N0o5TC95aGZhSFV2?= =?utf-8?B?MWE5a3dJTnVJYjJKVjVUYjBvaG9yM0ZrL1lYZ0dzTDZ0eGFna2E4MUM4NGl5?= =?utf-8?B?NmNPQlJaWFJwNXZqb3ZmZ0h1cHZTSUxwc0JNMnJpZnRqT1ZkejhpQlJVRVpz?= =?utf-8?B?L25Vc21zT3h1elZYQ1V5L3RmMFcrU2VPbXhWRmY3a3ZjNEFnN3BLZExRY2tB?= =?utf-8?B?MVBjMWNmWlNZTzJCaHZVcVdndzkzbTZNWE5DZWcwLzZ5ZmxWc1oyelIwdFJr?= =?utf-8?B?MFVENnREL2JJODNOczF3YXJGSVJlYi9kWVBabXNUdnQ0WTdxMFRIZ0hBTXdM?= =?utf-8?B?eUJNRTg1LzZUY0FwM1NtZi9xaUtmVVNYMU1TN0xZQ290YXowUnV1V05RZVdw?= =?utf-8?B?WFdVaFYvV090dFplZS92RlNxNFVDTkdGZFpUN3FpM3NnZGNMTWUrZWg3N3Nl?= =?utf-8?B?UWxtMWlDYVRraWJQSkNrVTdNNVR1MmlQYW5FMVNmZitQT05jOGExcExLKy83?= =?utf-8?B?WUFpR0hsY1V6VlIzM1NOSy9iZ3c3M1U4MTJKdy9NdjdQcmppVXhGY2FCUzhj?= =?utf-8?Q?uKcTZpmzT7ZOOp1Dz3O4wVDPn34rppNGEKDaw?= X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 6:0b+m2ALXkVzSxWvcnbUgecmT8MjjnQ3mjW8rzoHrwwkQYGh7cQTa2wk06MKw50rs9esF95bpxeLJIXBs19BVhuGN2HJOdnUyYgqFp3MIcvM6uLilHX/0uycXV1+gAPNXPqehUtCwZyKfzT8eOX1ucZqYLCRwuAsl6HJPZYPrmF98iH/j/U51KKFRjBmowreJWlNRnaRGc+IKbrtuw+A3bMY20EGYC+ggYFpOFH47S0BsLVf+jp8MX7RwVYqkCrfoYTZUhZlFWAJezynjcPHd8jGyJLDcpyV4gMtwxEiWW3e/hQlZFeYPCxpNvDE9+qTsFsaNLLzleoYMesF/fHrnmA==; 5:rCMElOuKdv8LJyDDUvcnq2e/3f4UXrLIZqY6RDwU5TDpibbSpWwbFXpFSsD8gtWIYrhy4qPdg4sXxdIWcMrrahGUhzY7YtGTsJqR9r9ea3Dagi2/kDARjCWv9pF4QqVgItWrwWuo2ZTQCYEMgQuezg==; 24:gSbIq3J0vVW20zBMQ23N2BwK5Km1cmioVg04QHXyj5UCmcjz1wMna2hCzQNaLG0wJgqU/a4wMqtwfCSJl8Ohqex+4RnhpXgs1KdGZaMDSuY=; 7:4ccvWpRtozIO/jA6VXlPjCUkab0CXwb7F/dznOQKmkmoUkIDM6Lp0DHp4W3MoDmkFOmAUg2nLVKjw9Q9ZAWNi4NBuZF/9s/TwOp7yBjw57kKJpa96J1VxIyl8yVhLDUmGKbEgYs8BZgsYDWARM8RiVZeJkDtPuk0fXLqn2vS0kKj2svEMIUdHhsWMaV6eblKOLvtBVue1NS+PK9zt3Bh25VQiiy764A6Hu8DS6teCIc= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 20:uMLqdzsvEzrG3FZYOtIyOcCwyBDNOL6uG50RubNiq2aL+3ODOHhUD1v9rJ0k6ssHmx2o73u8M8gEpKOHBnRMpROrj857UN1pT+YJpb58kD1Arf2KjJf0TXDajjfqn4ykm/1ksGppiuR90xcD75Y8baEzVxcWymv1AyMdz8rw0CY0DDzOeyGQ/hNGELEmm2Tfvkk7PmqATZ0Yp831wikn96zvibfQHsyIf1FItzo4ck2mV0VHzuApsogb/K2/UP4c X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Aug 2017 22:07:30.9579 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0158 Subject: Re: [PATCH v2 1/1] OvmfPkg : QemuFwCfgLib: Use BusMasterCommandBuffer to map FW_CFG_DMA_ACCESS X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 03 Aug 2017 22:05:23 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, Thanks for the detail review, I will soon send v3 with all your feedback addressed. I must admit that I have constant struggle with formating issues in EDKII contributions. While browsing the code, several packages have code and comment exceeding 79 char. But looking at your previous feedbacks somehow I was under assumption that comments should be <= 79 chars but code can exceed 79 char limit. I think my understanding was wrong. I will update my vi setting to warn me when I exceed 79 char limit. Sorry about all those formating errors, and thanks for being so patience. -Brijesh On 08/03/2017 03:51 PM, Laszlo Ersek wrote: > On 08/03/17 18:10, Brijesh Singh wrote: >> Commit 09719a01b11b (OvmfPkg/QemuFwCfgLib: Implement SEV internal function for Dxe phase) >> uses IOMMU protocol to allocate and free FW_CFG_DMA_ACCESS buffer >> when SEV is active. During initial commits we made assumption that >> IOMMU.AllocateBuffer() will provide PlainTextAddress (i.e C-bit >> cleared). This assumption was wrong, the AllocateBuffer() protocol >> member is not expected to produce a buffer that is immediatly usable, >> and client is required to call Map() uncondtionally with BusMasterCommonBuffer[64] >> to get a mapping which is accessable by both host and device. >> >> The patch refactors code a bit and add the support to Map() >> FW_CFG_DMA_ACCESS buffer using BusMasterCommonBuffer operation >> after allocation and Unamp() before free. >> >> The complete discussion about this and recommendation from Laszlo >> can be found here [1] >> >> [1] https://lists.01.org/pipermail/edk2-devel/2017-July/012652.html >> >> Suggested-by: Laszlo Ersek >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Brijesh Singh >> --- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h | 42 ++-- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c | 247 ++++++++++++++++---- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c | 130 ----------- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c | 101 +++++--- >> OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c | 56 ++--- >> 5 files changed, 292 insertions(+), 284 deletions(-) > > (1) please remove the space characters in front of the colons (":") in > the subject. > > (2) Still for the subject, it is "CommonBuffer", not "CommandBuffer". > > So the subject should be > > OvmfPkg: QemuFwCfgLib: Use BusMasterCommonBuffer to map FW_CFG_DMA_ACCESS > > (3) Please rewrap the commit message to 74 characters. > >> >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >> index 8cfa7913ffae..327f1d37e17d 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLibInternal.h >> @@ -45,39 +45,25 @@ InternalQemuFwCfgDmaIsAvailable ( >> ); >> >> /** >> - Returns a boolean indicating whether SEV support is enabled >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - @retval TRUE SEV is enabled >> - @retval FALSE SEV is disabled >> -**/ >> -BOOLEAN >> -InternalQemuFwCfgSevIsEnabled ( >> - VOID >> - ); >> + @param[in] Size Size in bytes to transfer or skip. >> >> -/** >> - Allocate a bounce buffer for SEV DMA. >> - >> - @param[out] Buffer Allocated DMA Buffer pointer >> - @param[in] NumPage Number of pages. >> + @param[in,out] Buffer Buffer to read data into or write data from. Ignored, >> + and may be NULL, if Size is zero, or Control is >> + FW_CFG_DMA_CTL_SKIP. >> >> + @param[in] Control One of the following: >> + FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer. >> + FW_CFG_DMA_CTL_READ - read from fw_cfg into Buffer. >> + FW_CFG_DMA_CTL_SKIP - skip bytes in fw_cfg. >> **/ >> VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ); >> >> -/** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> - >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> - >> -**/ >> -VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> - ); >> #endif >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c > > (4) You forgot to remove the InternalQemuFwCfgSevIsEnabled() function > from this file. > >> index f8eb03bc3a9a..e03647bae3bd 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c >> @@ -20,6 +20,7 @@ >> #include >> >> #include >> +#include >> #include >> #include >> #include >> @@ -157,74 +158,228 @@ InternalQemuFwCfgDmaIsAvailable ( >> } >> >> /** >> - Allocate a bounce buffer for SEV DMA. >> - >> - @param[in] NumPage Number of pages. >> - @param[out] Buffer Allocated DMA Buffer pointer >> + Function is used for allocating a bi-directional FW_CFG_DMA_ACCESS used >> + between Host and device to exchange the information. The buffer must be free'd >> + using FreeFwCfgDmaAccessBuffer (). >> >> **/ >> -VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> +STATIC >> +EFI_STATUS >> +AllocFwCfgDmaAccessBuffer ( >> + OUT VOID **Access, >> + OUT VOID **Mapping >> ) >> { >> - EFI_STATUS Status; >> + UINTN Size; >> + UINTN NumPages; >> + EFI_STATUS Status; >> + VOID *HostAddress; >> + EFI_PHYSICAL_ADDRESS DmaAddress; >> >> - ASSERT (mIoMmuProtocol != NULL); >> + Size = sizeof (FW_CFG_DMA_ACCESS); >> + NumPages = EFI_SIZE_TO_PAGES (Size); >> >> + // >> + // As per UEFI spec, in order to map a host address with BusMasterCommomBuffer64, > > (5) This line is too long. Also, you have one too many space chars in > front of the comma. > >> + // the buffer must be allocated using the IOMMU AllocateBuffer() >> + // >> Status = mIoMmuProtocol->AllocateBuffer ( >> - mIoMmuProtocol, >> - 0, >> - EfiBootServicesData, >> - NumPages, >> - Buffer, >> - EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED >> + mIoMmuProtocol, >> + AllocateAnyPages, >> + EfiBootServicesData, >> + NumPages, >> + &HostAddress, >> + EDKII_IOMMU_ATTRIBUTE_MEMORY_CACHED > > (6) Please OR this attribute with > EDKII_IOMMU_ATTRIBUTE_DUAL_ADDRESS_CYCLE, so that the buffer can be > allocated from 64-bit memory. > >> ); > > (7) The closing paren should be aligned with the arguments. > > (8) Please handle any errors returned by AllocateBuffer(), by > propagating the error. > >> + >> + // >> + // Map the host buffer with BusMasterCommonBuffer64 >> + // >> + Status = mIoMmuProtocol->Map ( >> + mIoMmuProtocol, >> + EdkiiIoMmuOperationBusMasterCommonBuffer64, >> + HostAddress, >> + &Size, >> + &DmaAddress, >> + Mapping >> + ); > > (9) Please add a check here that, on success, Size is still equal to > sizeof(FW_CFG_DMA_ACCESS). (Theoretically it could be smaller, even on > success.) > > If it is smaller, then both the map and allocate operations should be > rolled back, and we should return EFI_OUT_OF_RESOURCES. For this, error > handling labels at the end of the function are likely the best. > > (10) We should only set the output parameters Mapping and Access only if > the entire function succeeds. > >> if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, >> - "%a:%a failed to allocate %u pages\n", gEfiCallerBaseName, __FUNCTION__, >> - NumPages)); >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, HostAddress); >> } >> >> - DEBUG ((DEBUG_VERBOSE, >> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName, __FUNCTION__, >> - (UINT64)(UINTN)Buffer, NumPages)); >> + *Access = HostAddress; >> + return Status; >> +} >> + >> +/** >> + Function is to used for freeing the Access buffer allocated using >> + AllocFwCfgDmaAccessBuffer() >> + >> +**/ >> +STATIC >> +VOID >> +FreeFwCfgDmaAccessBuffer ( >> + IN VOID *Access, >> + IN VOID *Mapping >> + ) >> +{ >> + UINTN NumPages; >> + >> + NumPages = EFI_SIZE_TO_PAGES (sizeof (FW_CFG_DMA_ACCESS)); >> + >> + mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); >> + >> + mIoMmuProtocol->FreeBuffer (mIoMmuProtocol, NumPages, Access); >> +} >> + >> +/** >> + Function is used for mapping host address to device address. The buffer must >> + be unmapped with UnmapDmaDataBuffer (). >> + >> +**/ >> +STATIC >> +EFI_STATUS >> +MapFwCfgDmaDataBuffer ( >> + IN BOOLEAN IsWrite, >> + IN VOID *HostAddress, >> + IN UINT32 Size, >> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, >> + OUT VOID **Mapping >> + ) >> +{ >> + EFI_STATUS Status; >> + UINTN NumberOfBytes; >> + >> + NumberOfBytes = Size; >> + Status = mIoMmuProtocol->Map ( >> + mIoMmuProtocol, >> + IsWrite ? EdkiiIoMmuOperationBusMasterRead64 : EdkiiIoMmuOperationBusMasterWrite64, > > (11) Please make sure that the file is not wider than 80 characters -- > the above can be broken up like > > (IsWrite ? > EdkiiIoMmuOperationBusMasterRead64 : > EdkiiIoMmuOperationBusMasterWrite64) > > And if that's still too wide, then a separate variable should be used. > > >> + HostAddress, >> + &NumberOfBytes, >> + DeviceAddress, >> + Mapping >> + ); > > (12) Same comment as (9): on success, we should check that NumberOfBytes > has the original value (i.e., Size). Otherwise, roll back the Map(), and > return EFI_OUT_OF_RESOURCES. > > (13) Same comment as (10): we should only set the output parameters > (DeviceAddress and Mapping) if the entire function succeeds. > > (14) Here's an alternative to the explicit / cascading error handling > recommended above: given that all of these functions will be called from > InternalQemuFwCfgSevDmaFreeBuffer(), which cannot propagate errors > anyway, you might as well hang immediately upon error. For this, please > preserve the following triplet, used earlier in > InternalQemuFwCfgSevDmaAllocateBuffer(): > - DEBUG_ERROR message with gEfiCallerBaseName, > - ASSERT (FALSE), > - CpuDeadLoop(). > > Correspondingly, none of the helper functions would have to return > EFI_STATUS. > >> + return Status; >> +} >> + >> +EFI_STATUS >> +UnmapFwCfgDmaDataBuffer ( >> + IN VOID *Mapping >> + ) >> +{ >> + return mIoMmuProtocol->Unmap (mIoMmuProtocol, Mapping); >> } > > (15) I think it's fine if we ignore (don't propagate) the error here -- > please change the return type to VOID (similarly to > FreeFwCfgDmaAccessBuffer()). > > (16) Please make this function STATIC. > >> >> /** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> + @param[in] Size Size in bytes to transfer or skip. >> >> + @param[in,out] Buffer Buffer to read data into or write data from. Ignored, >> + and may be NULL, if Size is zero, or Control is >> + FW_CFG_DMA_CTL_SKIP. >> + >> + @param[in] Control One of the following: >> + FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer. >> + FW_CFG_DMA_CTL_READ - read from fw_cfg into Buffer. >> + FW_CFG_DMA_CTL_SKIP - skip bytes in fw_cfg. >> **/ >> VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ) >> { >> - EFI_STATUS Status; >> + volatile FW_CFG_DMA_ACCESS LocalAccess; >> + volatile FW_CFG_DMA_ACCESS *Access; >> + UINT32 AccessHigh, AccessLow; >> + UINT32 Status; >> + VOID *AccessMapping, *DataMapping; >> >> - ASSERT (mIoMmuProtocol != NULL); >> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || >> + Control == FW_CFG_DMA_CTL_SKIP); >> >> - Status = mIoMmuProtocol->FreeBuffer ( >> - mIoMmuProtocol, >> - NumPages, >> - Buffer >> - ); >> - if (EFI_ERROR (Status)) { >> - DEBUG ((DEBUG_ERROR, >> - "%a:%a failed to free buffer 0x%Lx pages %u\n", gEfiCallerBaseName, >> - __FUNCTION__, (UINT64)(UINTN)Buffer, NumPages)); >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> + if (Size == 0) { >> + return; >> } >> >> - DEBUG ((DEBUG_VERBOSE, >> - "%a:%a buffer 0x%Lx Pages %u\n", gEfiCallerBaseName,__FUNCTION__, >> - (UINT64)(UINTN)Buffer, NumPages)); >> + // >> + // When SEV is enabled, map Buffer to DMA address before issuing the DMA request > > (17) This line is too long. > >> + // >> + if (MemEncryptSevIsEnabled ()) { >> + VOID *AccessBuffer; >> + EFI_PHYSICAL_ADDRESS DataBuffer; >> + >> + // >> + // Allocate DMA Access buffer >> + // >> + Status = AllocFwCfgDmaAccessBuffer (&AccessBuffer, &AccessMapping); >> + ASSERT_EFI_ERROR (Status); > > (18) You can drop the assert according to cmment (14). > >> + >> + Access = AccessBuffer; >> + >> + // >> + // Map actual data buffer >> + // >> + if (Buffer) { > > (19) Please don't rely on Buffer being NULL for SKIP operations; that > may or may not be true. Instead, please check for > > (Control != FW_CFG_DMA_CTL_SKIP) > >> + Status = MapFwCfgDmaDataBuffer (Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0, >> + Buffer, Size, &DataBuffer, &DataMapping); > > (20) Please break every argument to a separate line. > > (21) The following expression: > > Control == FW_CFG_DMA_CTL_WRITE ? 1 : 0 > > should be simplified as: > > Control == FW_CFG_DMA_CTL_WRITE > >> + ASSERT_EFI_ERROR (Status); > > (22) You can drop this, according to (14). > >> + >> + Buffer = (VOID *) (UINTN) DataBuffer; > > (23) Ugh, please don't overwrite a function parameter. Instead, please > introduce an additional variable. > > For example, you could rename "DataBuffer" to "DataBufferAddress", and > introduce "DataBuffer" as a (VOID*). In the non-SEV case, you could set > DataBuffer to Buffer, and in the SEV case, you could set DataBuffer to > (VOID*)(UINTN)DataBufferAddress. > > Then use DataBuffer for setting Access->Address. > >> + } >> + } else { >> + Access = &LocalAccess; >> + AccessMapping = NULL; >> + DataMapping = NULL; >> + } >> + >> + Access->Control = SwapBytes32 (Control); >> + Access->Length = SwapBytes32 (Size); >> + Access->Address = SwapBytes64 ((UINTN)Buffer); >> + >> + // >> + // Delimit the transfer from (a) modifications to Access, (b) in case of a >> + // write, from writes to Buffer by the caller. >> + // >> + MemoryFence (); >> + >> + // >> + // Start the transfer. >> + // >> + AccessHigh = (UINT32)RShiftU64 ((UINTN)Access, 32); >> + AccessLow = (UINT32)(UINTN)Access; >> + IoWrite32 (FW_CFG_IO_DMA_ADDRESS, SwapBytes32 (AccessHigh)); >> + IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow)); >> + >> + // >> + // Don't look at Access.Control before starting the transfer. >> + // >> + MemoryFence (); >> + >> + // >> + // Wait for the transfer to complete. >> + // >> + do { >> + Status = SwapBytes32 (Access->Control); >> + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); >> + } while (Status != 0); >> + >> + // >> + // After a read, the caller will want to use Buffer. >> + // >> + MemoryFence (); >> + >> + // >> + // If Access buffer was dynamically allocated then free it. >> + // >> + if (AccessMapping) { >> + FreeFwCfgDmaAccessBuffer ((VOID *)Access, AccessMapping); >> + } >> + >> + if (DataMapping) { >> + UnmapFwCfgDmaDataBuffer (DataMapping); >> + } > > (24) The DataMapping check is not valid. You have a code path where > DataMapping is not set: Buffer==NULL (i.e., SKIP operation) with SEV > enabled. I guess the simplest way to fix this is to eliminate the > non-SEV branch near the top, and make all those assignments unconditional: > > Access = &LocalAccess; > AccessMapping = NULL; > DataMapping = NULL; > DataBuffer = Buffer; > // > // When SEV is enabled... > // > if (MemEncryptSevIsEnabled ()) { > ... > >> } >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >> index d3bf75498d60..7f42f38d1c05 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c >> @@ -45,136 +45,6 @@ QemuFwCfgSelectItem ( >> IoWrite16 (FW_CFG_IO_SELECTOR, (UINT16)(UINTN) QemuFwCfgItem); >> } >> >> - >> -/** >> - Transfer an array of bytes, or skip a number of bytes, using the DMA >> - interface. >> - >> - @param[in] Size Size in bytes to transfer or skip. >> - >> - @param[in,out] Buffer Buffer to read data into or write data from. Ignored, >> - and may be NULL, if Size is zero, or Control is >> - FW_CFG_DMA_CTL_SKIP. >> - >> - @param[in] Control One of the following: >> - FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer. >> - FW_CFG_DMA_CTL_READ - read from fw_cfg into Buffer. >> - FW_CFG_DMA_CTL_SKIP - skip bytes in fw_cfg. >> -**/ >> -VOID >> -InternalQemuFwCfgDmaBytes ( >> - IN UINT32 Size, >> - IN OUT VOID *Buffer OPTIONAL, >> - IN UINT32 Control >> - ) >> -{ >> - volatile FW_CFG_DMA_ACCESS LocalAccess; >> - volatile FW_CFG_DMA_ACCESS *Access; >> - UINT32 AccessHigh, AccessLow; >> - UINT32 Status; >> - UINT32 NumPages; >> - VOID *DmaBuffer, *BounceBuffer; >> - >> - ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || >> - Control == FW_CFG_DMA_CTL_SKIP); >> - >> - if (Size == 0) { >> - return; >> - } >> - >> - // >> - // set NumPages to suppress incorrect compiler/analyzer warnings >> - // >> - NumPages = 0; >> - >> - // >> - // When SEV is enabled then allocate DMA bounce buffer >> - // >> - if (InternalQemuFwCfgSevIsEnabled ()) { >> - UINTN TotalSize; >> - >> - TotalSize = sizeof (*Access); >> - // >> - // Skip operation does not need buffer >> - // >> - if (Control != FW_CFG_DMA_CTL_SKIP) { >> - TotalSize += Size; >> - } >> - >> - // >> - // Allocate SEV DMA buffer >> - // >> - NumPages = (UINT32)EFI_SIZE_TO_PAGES (TotalSize); >> - InternalQemuFwCfgSevDmaAllocateBuffer (&BounceBuffer, NumPages); >> - >> - Access = BounceBuffer; >> - DmaBuffer = (UINT8*)BounceBuffer + sizeof (*Access); >> - >> - // >> - // Decrypt data from encrypted guest buffer into DMA buffer >> - // >> - if (Control == FW_CFG_DMA_CTL_WRITE) { >> - CopyMem (DmaBuffer, Buffer, Size); >> - } >> - } else { >> - Access = &LocalAccess; >> - DmaBuffer = Buffer; >> - BounceBuffer = NULL; >> - } >> - >> - Access->Control = SwapBytes32 (Control); >> - Access->Length = SwapBytes32 (Size); >> - Access->Address = SwapBytes64 ((UINTN)DmaBuffer); >> - >> - // >> - // Delimit the transfer from (a) modifications to Access, (b) in case of a >> - // write, from writes to Buffer by the caller. >> - // >> - MemoryFence (); >> - >> - // >> - // Start the transfer. >> - // >> - AccessHigh = (UINT32)RShiftU64 ((UINTN)Access, 32); >> - AccessLow = (UINT32)(UINTN)Access; >> - IoWrite32 (FW_CFG_IO_DMA_ADDRESS, SwapBytes32 (AccessHigh)); >> - IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow)); >> - >> - // >> - // Don't look at Access.Control before starting the transfer. >> - // >> - MemoryFence (); >> - >> - // >> - // Wait for the transfer to complete. >> - // >> - do { >> - Status = SwapBytes32 (Access->Control); >> - ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); >> - } while (Status != 0); >> - >> - // >> - // After a read, the caller will want to use Buffer. >> - // >> - MemoryFence (); >> - >> - // >> - // If Bounce buffer was allocated then copy the data into guest buffer and >> - // free the bounce buffer >> - // >> - if (BounceBuffer != NULL) { >> - // >> - // Encrypt the data from DMA buffer into guest buffer >> - // >> - if (Control == FW_CFG_DMA_CTL_READ) { >> - CopyMem (Buffer, DmaBuffer, Size); >> - } >> - >> - InternalQemuFwCfgSevDmaFreeBuffer (BounceBuffer, NumPages); >> - } >> -} >> - >> - >> /** >> Reads firmware configuration bytes into a buffer >> >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >> index 40f89c3b53e2..bc649b40aec3 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgPei.c >> @@ -16,6 +16,7 @@ >> **/ >> >> #include >> +#include >> #include >> #include >> #include >> @@ -85,7 +86,7 @@ QemuFwCfgInitialize ( >> // (which need to allocate dynamic memory and allocating a PAGE size'd >> // buffer can be challenge in PEI phase) >> // >> - if (InternalQemuFwCfgSevIsEnabled ()) { >> + if (MemEncryptSevIsEnabled ()) { >> DEBUG ((DEBUG_INFO, "SEV: QemuFwCfg fallback to IO Port interface.\n")); >> } else { >> mQemuFwCfgDmaSupported = TRUE; >> @@ -129,56 +130,80 @@ InternalQemuFwCfgDmaIsAvailable ( >> } >> >> /** >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - Returns a boolean indicating whether SEV is enabled >> + @param[in] Size Size in bytes to transfer or skip. >> >> - @retval TRUE SEV is enabled >> - @retval FALSE SEV is disabled >> + @param[in,out] Buffer Buffer to read data into or write data from. Ignored, >> + and may be NULL, if Size is zero, or Control is >> + FW_CFG_DMA_CTL_SKIP. >> + >> + @param[in] Control One of the following: >> + FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer. >> + FW_CFG_DMA_CTL_READ - read from fw_cfg into Buffer. >> + FW_CFG_DMA_CTL_SKIP - skip bytes in fw_cfg. >> **/ >> -BOOLEAN >> -InternalQemuFwCfgSevIsEnabled ( >> - VOID >> +VOID >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ) >> { >> - return MemEncryptSevIsEnabled (); >> -} >> + volatile FW_CFG_DMA_ACCESS Access; >> + UINT32 AccessHigh, AccessLow; >> + UINT32 Status; >> >> -/** >> - Allocate a bounce buffer for SEV DMA. >> + ASSERT (Control == FW_CFG_DMA_CTL_WRITE || Control == FW_CFG_DMA_CTL_READ || >> + Control == FW_CFG_DMA_CTL_SKIP); >> >> - @param[in] NumPage Number of pages. >> - @param[out] Buffer Allocated DMA Buffer pointer >> + if (Size == 0) { >> + return; >> + } >> + >> + if (MemEncryptSevIsEnabled ()) { >> + // >> + // SEV does not support DMA operations in PEI stage, we should >> + // not have reached here. >> + // >> + ASSERT (FALSE); >> + } > > (25) This is good; you are basically restoring / copying the > pre-7cfe445d7f4b state of the InternalQemuFwCfgDmaBytes() function, plus > adding this assert. But, it can be written better like this: > > // > // ... keep the same comment ... > // > ASSERT (!MemEncryptSevIsEnabled ()); > >> + >> + Access.Control = SwapBytes32 (Control); >> + Access.Length = SwapBytes32 (Size); >> + Access.Address = SwapBytes64 ((UINTN)Buffer); >> >> -**/ >> -VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> - ) >> -{ >> // >> - // We should never reach here >> + // Delimit the transfer from (a) modifications to Access, (b) in case of a >> + // write, from writes to Buffer by the caller. >> // >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> -} >> + MemoryFence (); >> + >> + // >> + // Start the transfer. >> + // >> + AccessHigh = (UINT32)RShiftU64 ((UINTN)&Access, 32); >> + AccessLow = (UINT32)(UINTN)&Access; >> + IoWrite32 (FW_CFG_IO_DMA_ADDRESS, SwapBytes32 (AccessHigh)); >> + IoWrite32 (FW_CFG_IO_DMA_ADDRESS + 4, SwapBytes32 (AccessLow)); >> >> -/** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> + // >> + // Don't look at Access.Control before starting the transfer. >> + // >> + MemoryFence (); >> >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> + // >> + // Wait for the transfer to complete. >> + // >> + do { >> + Status = SwapBytes32 (Access.Control); >> + ASSERT ((Status & FW_CFG_DMA_CTL_ERROR) == 0); >> + } while (Status != 0); >> >> -**/ >> -VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> - ) >> -{ >> // >> - // We should never reach here >> + // After a read, the caller will want to use Buffer. >> // >> - ASSERT (FALSE); >> - CpuDeadLoop (); >> + MemoryFence (); >> } >> + >> diff --git a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >> index 071b8d9b91d4..62ddb69d3b4d 100644 >> --- a/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >> +++ b/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgSec.c >> @@ -97,53 +97,25 @@ InternalQemuFwCfgDmaIsAvailable ( >> } >> >> /** >> + Transfer an array of bytes, or skip a number of bytes, using the DMA >> + interface. >> >> - Returns a boolean indicating whether SEV is enabled >> + @param[in] Size Size in bytes to transfer or skip. >> >> - @retval TRUE SEV is enabled >> - @retval FALSE SEV is disabled >> -**/ >> -BOOLEAN >> -InternalQemuFwCfgSevIsEnabled ( >> - VOID >> - ) >> -{ >> - // >> - // DMA is not supported in SEC phase hence SEV support is irrelevant >> - // >> - return FALSE; >> -} >> - >> -/** >> - Allocate a bounce buffer for SEV DMA. >> - >> - @param[in] NumPage Number of pages. >> - @param[out] Buffer Allocated DMA Buffer pointer >> - >> -**/ >> -VOID >> -InternalQemuFwCfgSevDmaAllocateBuffer ( >> - OUT VOID **Buffer, >> - IN UINT32 NumPages >> - ) >> -{ >> - // >> - // We should never reach here >> - // >> - ASSERT (FALSE); >> -} >> - >> -/** >> - Free the DMA buffer allocated using InternalQemuFwCfgSevDmaAllocateBuffer >> - >> - @param[in] NumPage Number of pages. >> - @param[in] Buffer DMA Buffer pointer >> + @param[in,out] Buffer Buffer to read data into or write data from. Ignored, >> + and may be NULL, if Size is zero, or Control is >> + FW_CFG_DMA_CTL_SKIP. >> >> + @param[in] Control One of the following: >> + FW_CFG_DMA_CTL_WRITE - write to fw_cfg from Buffer. >> + FW_CFG_DMA_CTL_READ - read from fw_cfg into Buffer. >> + FW_CFG_DMA_CTL_SKIP - skip bytes in fw_cfg. >> **/ >> VOID >> -InternalQemuFwCfgSevDmaFreeBuffer ( >> - IN VOID *Buffer, >> - IN UINT32 NumPages >> +InternalQemuFwCfgDmaBytes ( >> + IN UINT32 Size, >> + IN OUT VOID *Buffer OPTIONAL, >> + IN UINT32 Control >> ) >> { >> // >> > > (26) Can you add a CpuDeadLoop() here as well (i.e., in the SEC > instance), after the ASSERT()? I know the previous version of the SEC > instance doesn't use CpuDeadLoop(), but I mentioned it here: > > http://mid.mail-archive.com/2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com > > Thanks! > Laszlo >