From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0069.outbound.protection.outlook.com [104.47.34.69]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D4C7E21E11D89 for ; Wed, 9 Aug 2017 11:21:13 -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=LgGRTkKcWUbVzX9/8g0hVVh9uQd2lGm9+R272AveNow=; b=XKrJfKSQR+eN61doY8Tc4gfl56lOFTv0tozRO+Y4/+roQuYkHQRK1xgx4NZVuHDZzQWEIFj/Ezf/w7kzjk+3s0Vk0bewBgE+iv/rpVxaP2lXMKUtSIS6Waw/76vtMS+yv8ycYUDKUtGMMIhWROYpmlnn/zENoAeLoynybhu8R+M= 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.1320.16; Wed, 9 Aug 2017 18:23:28 +0000 Cc: brijesh.singh@amd.com, Jordan Justen , Tom Lendacky , Ard Biesheuvel To: Laszlo Ersek , edk2-devel@lists.01.org References: <1502107139-412-1-git-send-email-brijesh.singh@amd.com> <1502107139-412-2-git-send-email-brijesh.singh@amd.com> From: Brijesh Singh Message-ID: <35b958de-ed97-6275-be96-1cbb93a99d97@amd.com> Date: Wed, 9 Aug 2017 13:23:23 -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: X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: CO1PR15CA0046.namprd15.prod.outlook.com (10.175.176.14) To SN1PR12MB0158.namprd12.prod.outlook.com (10.162.3.145) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 164a7952-f47b-4703-5087-08d4df53bc73 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:AqMr8O3nQneke5gM6tatNrA9LagJIUOGGrV2UEIh9TzvPm+A3MZ3okDkL1ZVNwWHXDuS5Q1fBuWGcHCf7za5MTzqcUln79qD8uHcFvEoOX85TE6A0iw5ta2jexGrXF+CZtN8dCWn41oDFxuSxtJGtePF+CJ+QdGirEB21GYG8+fPmiZC32l87uaTA09jnQT1hm/TqhRn8dPd2aU8eEohH0g+06xLz9sAD5VSIM75xQurwk2NYVIi+wfLmCil3BTw; 25:sOeglSuprHsLwVE8NKjY6BaRcQurLAUxQTNPM7urc7Ian3qsjj3PLQOn9qvxsK6dZA4RNdq7EjjgHSOkwPjGCIQWGYwvBGwNK3TQRoD5a4R0yGYNpuys5XcLnaHalW2KoHeVIGDxutvOGZ82/NzQNH0+i8SlwRBmu5vFDRakbivGmpbBD5FVY7nOOSphxKVaR/ErL9dFXcAv2+ZLOkIn28c7e7WHSstGn7pPj8wBfFGVgJq+m8oMCIvErflSgRfbZo70Cmqz/sqwnDP0ACzsXF88/B0ov5wyytTHuinPTGlawrdazujPHrQaA0xQKs2MxmPm0p8DCIx9vG1XjsYIhg==; 31:znr9Ifx7OfLZNz0kOM3vrptPBYEDdL/5skaVlSpWPfDxHAhnisqTwQasFW6v7pIrSEHRUjrnJrdTD7katW1NvzYp+lMAndSmjObd88URRmWF3kkA+eRaM54VHUdtIT3LjKnjyZllODI64TdhiNg5cSdSjcf8XG0KuNxJkxKo90kDK5GwNN/xiMISGHCSqqjrloB7VeTz40LqbqKWTEGSgYlFjsExIzlF2Dep42m6GJg= X-MS-TrafficTypeDiagnostic: SN1PR12MB0158: X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 20:OpyUzttNqmCSLaLmFYOmcvNI6yol2Fs0p+kDxK9lXEldJHr65LJoVlzcMFt/E70cUqlRjuaIa/iHSncCyaCloAbQ2B+HynuPpKRy7BUcIdlsOsEhWXm36R/v1FK7qnqGnZrgrqUpoDTI1lM6G1kD9N58tHSPPZW6Il/v7N4PvX0TZM25ckB5YJ8Z/GupEB1trGjv15Rl1nBzeekmaynmWJQKNSE3bqAS3615rlwSYkWve31FDQngK7QJTBTrDEflmi/smj8nln+d4hOar7dUN16k+6Q/2MPgGHQtzrlgxfBqjhBfzqf8Fj66Evl8sKDnKkfa+x+3V17EPpXKX1JaFOyYF9FKWXy1CWcgNT3g2Fk0mdOhNlqC04jBwKtJdOWbkjjriA+10mi3avcF/EU5WSP84Cr9KLIy6toQNK27MMqG71boUut5wyiPxYuB7TsgVQTNyASgWbqbEfkLgRI2XtXw3mBRX5iCCo9qh6aFKxlpAtBtjtjZukYbn3iyIxzT; 4:r0Q4oL+2IjAbG57V3+1cEgc7iPpM9f6z5cr6CZzzkxuPWIzuE8fUKmGFKUZ2oq6CZi1Dv5f/I8qNRcpJYvlfzTqaKrXN+I4gBiBiGAcLjWWXmVsxOu324nevl3lNXmoOVT9jE3q8JcWPOukaD7ZhYW2d9k4AhctSDRqYdZf2QRojJda2kCi70fNdXBcjA5s5TplAC3VHNgvRqjAuGqSTIOM7Sy5LRdjQEVwLU2Snv/99qVbKWAmjvSESX728KIFnZq32xvj0wJCTLqeWopsHBpwxyZ6qhkHICYAdXQW0Ukn+04v7kewA6DNd+N5GfBHXsaoAc067e5VGIOrPaZvTUK4J7dvSwNRtV5dAL0yQ+vU= X-Exchange-Antispam-Report-Test: UriScan:(767451399110)(788757137089)(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)(93006095)(93001095)(10201501046)(3002001)(6055026)(6041248)(20161123564025)(20161123560025)(20161123558100)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123562025)(20161123555025)(6072148)(201708071742011)(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: 0394259C80 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(4630300001)(7370300001)(6049001)(6009001)(39410400002)(39400400002)(39860400002)(39450400003)(39850400002)(39840400002)(377454003)(76104003)(189002)(24454002)(199003)(478600001)(110136004)(38730400002)(50986999)(47776003)(76176999)(54356999)(305945005)(31686004)(101416001)(7736002)(8676002)(5660300001)(54906002)(106356001)(66066001)(81166006)(6246003)(4326008)(81156014)(65826007)(230700001)(2906002)(7350300001)(25786009)(53936002)(105586002)(229853002)(68736007)(36756003)(31696002)(6116002)(3846002)(33646002)(42186005)(83506001)(65806001)(77096006)(23676002)(53546010)(64126003)(90366009)(50466002)(4001350100001)(97736004)(6666003)(6486002)(86362001)(2950100002)(65956001)(189998001); DIR:OUT; SFP:1101; SCL:1; SRVR:SN1PR12MB0158; H:[10.236.136.62]; FPR:; SPF:None; PTR:InfoNoRecords; MX:1; A:1; LANG:en; Received-SPF: None (protection.outlook.com: amd.com does not designate permitted sender hosts) X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtTTjFQUjEyTUIwMTU4OzIzOkxLZExIMlNIdHdBVG1rWjkrZG9VbDZSUXVt?= =?utf-8?B?elgzTzYzbUFab3crNTFZRkNoTjFmbTdqZ3NHcG56TTRGMEpQSG9yT1RMMGtq?= =?utf-8?B?c2ZFd0ErQmxwbkJVYVQ2N3Z0cDdYaGhzWWhUblVYOU1oN0lFdVA1ai8wUDZq?= =?utf-8?B?TGwvbG9NMHdiSmc5UFp3cWpvQmw5QmNsaG1yTGNiaG55M0ZUM3ZOMmFKdXRv?= =?utf-8?B?dTRXM0YvOUIyVVhRUlJ4Yyt5TmZCOGdRU0c1QUU5aTdNeUpQdk9Bb09JZjJm?= =?utf-8?B?c2ZPdVdXUS9rT1JjejRYZk5GalN3VkRLbVlSc3lmdmxsSWc2YytFbDFyQkli?= =?utf-8?B?aUlPT0FPZE5EWHArTGoxVjlsT3YwU05YdGp5ajBuZDRhdzZGR0JocVpvQjZE?= =?utf-8?B?aUZHU1RDUUQzWFpkYWFYT1liYWhUU2QxNCtySmZBVzhjTUtLZkIyYmpJQVIy?= =?utf-8?B?Sk9ia05mTUFFOXJlMWR4SE11NXIyWjdxQnVyVkxuYXJ3SEsyNEhaZ2piejha?= =?utf-8?B?UkNNUWtGM25Nb3ppeGtXNzhiTDdsbUxnTDNleklOTW5xTnQyYWhMZnZXOCt5?= =?utf-8?B?M1M0WU5hSExBSjNOcG9pVXNSNUozUTV6Q0RzcFcxUFR0TmF0OVJwVzY2Z3ht?= =?utf-8?B?Q3BKYmV3Y25pR0xMd08xenhFV3FMUEdKL2lZajIzZmlySHlVbXRIaGlaVUk4?= =?utf-8?B?REZ1Vk94M1JiLzBFVUhTeFZrTzFGNkZzbC9HRFVMVGpNZWVFWVczZ1B6d3Iy?= =?utf-8?B?RnA1S0xIUmJZdlByK2JDVzgxR3hFUzFIWXZiNXN6Y1NSUm95MmZkOEZIYjA3?= =?utf-8?B?VVR6RlVkbmJpKzU4RzZKRCtncWtySE1xT0llOGMvRE5tRnJVQmE1Q1NEU1FH?= =?utf-8?B?QUZicUNiM3NhellOaGFDdDMyQlhqSlg5a0REYWhaSTd4SmRRc3dSRVBXbXhR?= =?utf-8?B?dklnNVJiOHhXOWRDTTVQZk42NWZxSU4zMFdTRDVLM1ZlOFV5RU9PL2lDRFhJ?= =?utf-8?B?d1JyOGNqeEZOWkovanFJcWIzaUV4QTVUMlU2V01LUVBGdUtWOUxrNjBSa0dO?= =?utf-8?B?U0NaYkw5YStKM3BGWi9Tc2prZ0RFbUU4Q2ErVXBwbDVKQzVIZm1INVU4bEdu?= =?utf-8?B?clZSYzlvVUZmZmhSZGI2TzloWUE5MFIyZWtLSlY4YlJMa0JJeG04UGZvOTlW?= =?utf-8?B?cnQyaGUrM3hGTExJT0JVSUM5NjU2YThHOW96UUlaanVwaHNVc0NaejUzdEVn?= =?utf-8?B?eHp2cEVZQWVUTEU0b0hkQTJVRFh5Q3h2YzFNQlVFRmdTOVl0V1RKR214WW0w?= =?utf-8?B?c1Q2c2Y4cDhnZ05NYlhHUTZVN1MxK0xBblVHSDluaFFNU09yanR0MDVxTk9V?= =?utf-8?B?Ykd3akM3V2tNSzF1akxrUkpUQWY3dFZPZ0VFRGVQSWJ0VmVnNiszV3g1Sncw?= =?utf-8?B?bzJrNlhkOEdLNU5vUGRhWFJ4bTNjclpzWXJJTlZsMEN4UmdaMGswYWNENjVy?= =?utf-8?B?eFBjeEdZWGFUTnRzSm9DZE1mN0szbW9tRHIwWlcrYkpaYUV1TldQbmZ1NlpW?= =?utf-8?B?anRWODJqdlRlQUVHUFBKN1dLNkZoeFIvZjFibTluc28vNi96WHg3L2VLcWhH?= =?utf-8?B?T0RLMXlpam9qeGJSTGkxQ1k1WjYzYmNycDR2YzA3NU52THhHNlBXSW14UDdB?= =?utf-8?B?LzU1RXdOQzcweHBNd2ZwMFkrMjlFU3Q3azZZY2pnNEF6eVZKUUNueDlzVU9a?= =?utf-8?B?Vmp6MENpTkhoVG1EWUlxTFVTME9nNWkza0Y2OTFHZVBiRDhqMTMxTE04REFk?= =?utf-8?B?VFVqZldUa05TKzA1T0Y2Ymk5eU94QnJZVGVqYjYwVGVDVE5paGNXTUc4QW05?= =?utf-8?B?RHJuYWJCaGNMN3d4bVQ5bGptL29odHFrVlpFeFhZU1E4SkJURDY0aHBROGgw?= =?utf-8?B?NHRCa1hWUUtoZ3VMZjVZQWZva1U5bXV6dklLSzlvQ3RYZUgyem1SdzdXZGgw?= =?utf-8?B?cDVZN3JvNUxjaFRkQVFtYjY0WVNIdDJ1c0YxU1BINlliNzU3UWJtSzdHZm9k?= =?utf-8?B?aHFzdVRBcHRRd3BIb1N2NlZkVkZKZ3lYRGdRV254eXNOM1dSdTdaeFprRk1i?= =?utf-8?Q?6F8t82oVMCGi3/gGFe9l1IkkSHQP8LzXcq60ipGHfFcY?= X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 6:NyRGk3lhvA1NAv2PEI1GjRdduWTu86AMOru1ihldiRZWUMuR387wnA1nbrBnpSeyUEQETE5NS9csFJ20tsHVaI1fK6hMcE2f5r5nu3qcbERMmSKbodaAqcPHevEHYPf8zc+grHhf9p7spcRkUlaTibjn/dqCHkZhq9ShhgWh9HvH6/CkEjAD/0uBmB/Vhx7gdDxgFvbw6dMG6VgEcNiRDpm2EFawbsQO8fEbVxsLSJjAjl2eiWpfnXnWXRrIBEbEM8mkPNs8KMKeyldkI5RSQqzU6s0vhraoFPxBiDP/R2fk3ta965PvWpaso2Sbv/HrrSa3xOgr16ILK99EshahbA==; 5:LJ3zXlYnEGMOYE77f7xoRwVtCgvSkrSYMsNnhpzVqEfY7nkHgJF09weJo/iZKvlgYz3HPkzV+jS6/U+HV6K+z2PJkj5ZYupbydjoo0mnUipIG0eDL+F9dfWZUP1SDm7r9MLy59/xov0IGg2btjJIUQ==; 24:WK/FN4rK6HZf4lr/4moitSrkRUCPhex6u9kvWmPG2eurZsaVnhgiLRf3tVK0KWABu5sxN+OhzFE2j07HWs1sCpYab0iBVFBnfOidAlRxMrw=; 7:m95AkfoPun5VMeaqDb64U+hMyJESL8bFdW3Zkx6OHx2gE4vhogQ0wRsLUP8emm02hRMvw3R/qcpucRlsVRHi7t5t/0tctUEZbHTa88Eixv8vPGg/5e2C7HKchQHlzks1dS5VZdRtXb9WbpXH+xAEsa4Fsnl8SvXRjf2jZZzvFlilLAAqHbZYPoOE18axEgR8ZkZY7Z6AEEtPMbwD/ZGiXTHffooibPazqz1D63gqbmE= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; SN1PR12MB0158; 20:IkBl+2udKaLcC66Up04evtz1Ny0U1eIaatqbVRmKeUnep4RsWmyUIHwTcBgcWm4kSwzJRJUErtsG44ycJksvb7txlb3XaBBwh0mUuobpEc4yZak0rQZKT7M88+ku+MXHZIGJ5wIkJpWeyJqD7mYR1Yfd+IUswMkHX8R7VSpwEd5ofC3ogn0c7DIZrRISO38Dpe0oxJL2Jlr1sjJss6Dd2LCLL9bWH3vZ7uq9STsvI4WzbSp79Oi+qc8OsBtCrG/N X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2017 18:23:28.8973 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN1PR12MB0158 Subject: Re: [PATCH v1 01/14] OvmfPkg/Virtio: Introduce new member functions in VIRTIO_DEVICE_PROTOCOL 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: Wed, 09 Aug 2017 18:21:14 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 08/09/2017 09:27 AM, Laszlo Ersek wrote: > On 08/07/17 13:58, Brijesh Singh wrote: >> The patch extends VIRTIO_DEVICE_PROTOCOL to provide the following new >> member functions: >> >> - AllocateSharedPages : allocate a memory region suitable for sharing >> between guest and hypervisor (e.g ring buffer). >> >> - FreeSharedPages: free the memory allocated using AllocateSharedPages (). >> >> - MapSharedBuffer: map a host address to device address suitable to share >> with device for bus master operations. >> >> - UnmapSharedBuffer: unmap the device address obtained through the >> MapSharedBuffer(). >> >> Suggested-by: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Tom Lendacky >> Cc: Laszlo Ersek >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Brijesh Singh >> --- >> OvmfPkg/Include/Protocol/VirtioDevice.h | 121 ++++++++++++++++++++ >> 1 file changed, 121 insertions(+) > > (1) I suggest the following subject: > > OvmfPkg: introduce IOMMU-like member functions to VIRTIO_DEVICE_PROTOCOL > > The current wording "new member functions" is a bit too generic IMO. > Will do. >> >> diff --git a/OvmfPkg/Include/Protocol/VirtioDevice.h b/OvmfPkg/Include/Protocol/VirtioDevice.h >> index 910a4866e7ac..ea5272165389 100644 >> --- a/OvmfPkg/Include/Protocol/VirtioDevice.h >> +++ b/OvmfPkg/Include/Protocol/VirtioDevice.h >> @@ -5,6 +5,7 @@ >> and should not be used outside of the EDK II tree. >> >> Copyright (c) 2013, ARM Ltd. All rights reserved.
>> + Copyright (c) 2017, AMD Inc, All rights reserved.
>> >> This program and the accompanying materials are licensed and made available >> under the terms and conditions of the BSD License which accompanies this >> @@ -31,6 +32,26 @@ >> >> typedef struct _VIRTIO_DEVICE_PROTOCOL VIRTIO_DEVICE_PROTOCOL; >> >> +// >> +// VIRTIO Operation for Map >> +// > > (2) "Map" can be made a bit more precise; please say either > "VIRTIO_MAP_SHARED" or MapSharedBuffer(). > Will do >> +typedef enum { >> + // >> + // A read operation from system memory by a bus master >> + // >> + EfiVirtIoOperationBusMasterRead, >> + // >> + // A write operation from system memory by a bus master >> + // >> + EfiVirtIoOperationBusMasterWrite, > > (3) s/from/to/ > Will update it. thanks >> + // >> + // Provides both read and write access to system memory by both the >> + // processor and a bus master >> + // >> + EfiVirtIoOperationBusMasterCommonBuffer, >> + EfiVirtIoOperationMaximum >> +} VIRTIO_MAP_OPERATION; >> + > > (4) Please drop the "Efi" prefix. > > (5) Please replace the remaining "VirtIo" prefix with "Virtio". Although > you can find both spellings in the source code, they are used in > different contexts. In function names and enumeration constants, we've > used Virtio* thus far, as far as I can see. > > I hope this won't cause too much churn for the series! > I was actually trying to follow the IOMMU protocol header file, but I will drop the Efi Prefix and also fix the "Virtio" prefix. Drivers don't use these macro's directly, I have a helper function in Patch 4 to map the shared buffers. Depending on your feedback on Patch #4, it may not be bad change at all. If we don't want those helper functions from patch 4 then I will anyway have to update the drivers. In summary, I will make the suggested changes in v2. >> /** >> >> Read a word from the device-specific I/O region of the Virtio Header. >> @@ -319,6 +340,100 @@ EFI_STATUS >> IN UINT8 DeviceStatus >> ); >> >> +/** >> + >> + Allocates pages that are suitable for sharing between guest and hypervisor. >> + >> + @param This The protocol instance pointer. >> + @param Pages The number of pages to allocate. >> + @param HostAddress A pointer to store the base system memory >> + address of the allocated range. >> + >> + @retval EFI_SUCCESS The requested memory pages were allocated. >> + @retval EFI_OUT_OF_RESOURCES The memory pages could not be allocated. >> + >> +**/ >> +typedef >> +EFI_STATUS >> +(EFIAPI *VIRTIO_ALLOCATE_SHARED)( >> + IN VIRTIO_DEVICE_PROTOCOL *This, >> + IN UINTN Pages, >> + IN OUT VOID **HostAddress >> + ); > > The typedef / function prototype looks good. Some comments for the > comment block: > > (6) s/base system memory address/system memory base address/, I'd think; > but I could be convinced otherwise > > (7) Please annotate each @param with: [in], [out], or [in,out], as > appropriate (see other examples in the same header file). > > (This affects the rest of the members added in this patch. > > It also likely affects all three implementations of this set of members > (via copy-pasted comment blocks).) > I will fix those, I was doing copy/paste of these functions from MdeModules/Include/Protocol/IoMmu.h hence missed updating the annotate of each param. >> + >> +/** >> + Frees memory that was allocated with SharedAllocateBuffer(). > > (8) s/SharedAllocateBuffer()/VIRTIO_ALLOCATE_SHARED/ > Will do thanks >> + >> + @param This The protocol instance pointer. >> + @param Pages The number of pages to free. >> + @param HostAddress The base system memory address of the allocated range. > > (9) Whatever you do for comment (6), please apply it here as well. > Will do >> + >> +**/ >> +typedef >> +VOID >> +(EFIAPI *VIRTIO_FREE_SHARED)( >> + IN VIRTIO_DEVICE_PROTOCOL *This, >> + IN UINTN Pages, >> + IN VOID *HostAddress >> + ); >> + >> +/** >> + Provides the shared addresses required to access system memory from a >> + DMA bus master. > > (10) what do you think of s/shared addresses/device address/, singular? > (Also, replace "shared" with "virtio device" or just "device" below?) > Just an idea. > device address works for me, I will update it. >> + >> + @param This The protocol instance pointer. >> + @param Operation Indicates if the bus master is going to >> + read or write to system memory. >> + @param HostAddress The system memory address to map to shared >> + buffer address. > > (11) Please point out the connection between > VirtioOperationBusMasterCommonBuffer, VIRTIO_ALLOCATE_SHARED, and > HostAddress. > > It doesn't have to be extremely verbose (feel free to steal and adapt > the woring of the PciIo chapter in the UEFI spec), but since this is a > protocol header file, we should say something about that connection. > > It's also fine if you just add a pointer to the UEFI spec (spec version, > chapter/section, interface), and say "this interface follows the same > usage pattern". > Again this was basically copy/paste from IoMmu.h hence I did not put anything regarding the VirtioOperationBusMasterCommonBuffer, I will mention the UEFI spec version and section. >> + @param NumberOfBytes On input the number of bytes to map. >> + On output the number of bytes that were >> + mapped. >> + @param DeviceAddress The resulting shared map address for the >> + bus master to access the hosts HostAddress. >> + @param Mapping A resulting value to pass to Unmap(). > > (12) What do you think of: "a resulting token to pass to > VIRTIO_UNMAP_SHARED"? > Works for me. thanks >> + >> + >> + @retval EFI_SUCCESS The range was mapped for the returned >> + NumberOfBytes. >> + @retval EFI_UNSUPPORTED The HostAddress cannot be mapped as a >> + common buffer. >> + @retval EFI_INVALID_PARAMETER One or more parameters are invalid. >> + @retval EFI_OUT_OF_RESOURCES The request could not be completed due to >> + a lack of resources. >> + @retval EFI_DEVICE_ERROR The system hardware could not map the >> + requested address. >> +**/ >> + >> +typedef >> +EFI_STATUS >> +(EFIAPI *VIRTIO_MAP_SHARED) ( >> + IN VIRTIO_DEVICE_PROTOCOL *This, >> + IN VIRTIO_MAP_OPERATION Operation, >> + IN VOID *HostAddress, >> + IN OUT UINTN *NumberOfBytes, >> + OUT EFI_PHYSICAL_ADDRESS *DeviceAddress, >> + OUT VOID **Mapping >> + ); >> + > > Looks OK. > >> +/** >> + Completes the Map() operation and releases any corresponding resources. > > (13) What do you think of s/Map()/VIRTIO_MAP_SHARED/? > Works for me >> + >> + @param This The protocol instance pointer. >> + @param Mapping The mapping value returned from Map(). > > (14) I suggest "the Mapping token output by VIRTIO_MAP_SHARED". > Will do >> + >> + @retval EFI_SUCCESS The range was unmapped. >> + @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by >> + Map(). > > (15) s/value/token/ > Will do > (16) Please remind the reader that the implementation is not required to > recognize and report all such cases. Invalid Mapping tokens can cause > undefined behavior. > Will do >> + @retval EFI_DEVICE_ERROR The data was not committed to the target >> + system memory. >> +**/ >> +typedef >> +EFI_STATUS >> +(EFIAPI *VIRTIO_UNMAP_SHARED)( >> + IN VIRTIO_DEVICE_PROTOCOL *This, >> + IN VOID *Mapping >> + ); > > OK. > > (In general I'm not sure if I like EFI_STATUS as return type here. I > would prefer VOID, I think -- my guess is that the rest of the patch set > will ignore the return value anywayy: > - EFI_SUCCESS fits VOID obviously. > - EFI_INVALID_PARAMETER is fully preventable if the caller sticks with > the valid use pattern (and if they don't they can get undefined > behavior anyway). > > The only reason I think we should still keep the EFI_STATUS return type > on the interface level is that Unmap() might do actual data movement > after BusMasterWrite. Before client code consumes such data, the client > code should really check whether Unmap() succeeded (i.e., see the > EFI_DEVICE_ERROR case). > > So, I guess let's keep the EFI_STATUS return type. (And check the > returns status everywhere in this series, wherever appropriate.) > Okay lets make sure we check the status when required >> >> /// >> /// This protocol provides an abstraction over the VirtIo transport layer >> @@ -353,6 +468,12 @@ struct _VIRTIO_DEVICE_PROTOCOL { >> // Functions to read/write Device Specific headers >> VIRTIO_DEVICE_WRITE WriteDevice; >> VIRTIO_DEVICE_READ ReadDevice; >> + >> + // Function to allocate, free, map and unmap shared buffer > > (17) s/Function/Functions/, plural > Will fix it > (18) I realize the current comments in this structure don't conform to > the coding style. Can you perhaps prepend a patch that fixes the comment > style for the following: > Sure will do > // VirtIo Specification Revision: ... > > /// VirtIo Specification Revision encoded with VIRTIO_SPEC_REVISION() > > /// From the Virtio Spec > > // Functions to read/write Device Specific headers > > They should all be spelled > > // > // comment > // > > (There is no general need to prefix or suffix such comments with > additional empty lines -- use your judgement. In actual code, the empty > lines are helpful; between fields of a structure, not necessarily.) > > (19) and then please adapt the new comment in this patch. > > (20) This is actually for the commit message, but I'm too lazy to > renumber my points :) In the commit message, please state that: > > We're free to extend the protocol structure without changing the > protocol GUID, or bumping any protocol version fields (of which we > currently have none), because VIRTIO_DEVICE_PROTOCOL is internal to edk2 > by design -- see the disclaimers in "VirtioDevice.h". > Sure, I will use the message in commit. Thanks Brijesh