From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM02-SN1-obe.outbound.protection.outlook.com (mail-sn1nam02on0049.outbound.protection.outlook.com [104.47.36.49]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 048CC208F7AB4 for ; Wed, 30 Aug 2017 05:18:49 -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=NWkBviVOxkTRUoYG2dglzPU2jqS4ndOxFZpD2U86bb8=; b=c6t5KrKoml1y+7ydAesrhkYopZHclQaKbwb7jv7U/XvuuNHcZtw4WEciQThNPzSEJSBNT4o9IvRqPMBJaanooidf/HEl3WQ0sKFz6EWcImiE6ZYpEkTkd2DcRovn+W/8t9jwKQkUHNEyagzyf+9sjl8DF1A4L4N/qMen2JzyCEE= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Received: from wsp093899wss.amd.com (165.204.77.1) by CY1PR12MB0151.namprd12.prod.outlook.com (10.161.173.21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1385.9; Wed, 30 Aug 2017 12:21:26 +0000 Cc: brijesh.singh@amd.com, Jordan Justen , Tom Lendacky , Ard Biesheuvel To: Laszlo Ersek , edk2-devel@lists.01.org References: <1503919610-26185-1-git-send-email-brijesh.singh@amd.com> <1503919610-26185-3-git-send-email-brijesh.singh@amd.com> <9f3256f3-6acd-e88b-7fb4-dccb594d6926@redhat.com> From: Brijesh Singh Message-ID: Date: Wed, 30 Aug 2017 07:21:24 -0500 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <9f3256f3-6acd-e88b-7fb4-dccb594d6926@redhat.com> X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: MWHPR08CA0059.namprd08.prod.outlook.com (10.173.236.33) To CY1PR12MB0151.namprd12.prod.outlook.com (10.161.173.21) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: f570f6a9-b4ce-47db-9e0b-08d4efa1a409 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:; BCL:0; PCL:0; RULEID:(300000500095)(300135000095)(300000501095)(300135300095)(22001)(300000502095)(300135100095)(2017030254152)(300000503095)(300135400095)(48565401081)(2017052603199)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:CY1PR12MB0151; X-Microsoft-Exchange-Diagnostics: 1; CY1PR12MB0151; 3:pIPm/cpVABDHMld1ZQJFFuzQKHOYdcBKgph2BgA8qo3sOTm33YgK0+BJzTaD/Y8yejnK/au8f4Cv+tccFCkbaStZ/CqgsFhNJxgbBD9iNAl6e9ZCAYmMhNGsku2mdA5PYT7nsq0aZMfrWUMmNgSdwBjTHR5zWaFzwy/M4xu9Ph6HLiDnC/pKnb3tUm6KjTn7jx9j6FYJUOfqSQRjz8xAuj8FdfPfmlSx3J7ctVgYihyRYvyCLm0sF4/zZZvu2eMq; 25:FX4UstVlQucq6FyvhCqXNWmmaxutlTd94ElEy9cogf27Sq/h6VA2wb13ew7zWmKkt3mDef6bkSQLPL9QNzlntaEcVexKT+QSlcgG+RYfMSIn5EXpI3T4jj1M+T8GG7bYOxRjI4ZyDoLpPBdTJU9A6JlfddpWSjYoHoaZNE+xqcr9MfYQT/4U1QP0FeWlbWAIcAx4d0m+cQfpnwETRAb8PSnc8mllkLvDx48hFU0n1AbjHtkcStXiIeYA2MVa0ermYvFI2S+e2OZ5EVIrlGVzWVYZDyo40cwO7uSfLXRHefVrivIV6jgqsX7xJq6O1C9iMxkoOP1rd52yBpJX4pc5yg==; 31:wKqIUkVOEH+QXZHmPeI2djUFCW6Nvwd1WL5zbA8OifWq69H7E+ATKAW4gEL8kRiRkNym8+7FMVQ+q7RUcyq/5wu6KJbSInf3YY9q5njSw5XZZqCKw+RQvlFF9yJDBhlPQ1xrgYR8LXbD55eg/6X8YzaaF5/j6xOk/0qsPs0XCMHerguOrKwe1ajIG5nMydC1lufG2NjVURszIH3HjP24NW2A5WiuypZxJ4OhcfBw32g= X-MS-TrafficTypeDiagnostic: CY1PR12MB0151: X-Microsoft-Exchange-Diagnostics: 1; CY1PR12MB0151; 20:CcpZ78qwUqNQREHXVzH8q39a/7+c3qyPV3azpUpxKXy5+swsvBKbjrck40qicHN6jg2/9jXo+7hzVoXO3nadZSbH/pwQNXVU6yy9Ybb75ouQ6urCSvUdOLUE6G2NaaySvYN75xtA7SnE+wsrkz2RyQKapdaCVUoZ8vrQjSxbpgmfetRBLor2KuRllLIa3xDMkTcnV3wQPSNZ2ukwLLWgu/2/sTZgRRdVa2Bxglr0Se9iovA/cbXAD3++ut1DfDFKkh8RC0+PAOljy5VyCyC1vWqQJV7v2nAoJ8ax+VB7rOBad9SiWsaPge94ZhjNazyGWX0lDKhfDDxnw/8NREZfsqHv1koW5BLgcQmY2UEP/ppGPmWEeedSIjkIV0UY6jJhr44F3nhgfZlSbd7N+9PUj094ryE3QNFj1Dep8ZRSZs1Bq9ZiOHkwOvNAHlQSQ7ko9N0p8rvr6AnI83MfGaO9AlKkg8YBXpn3tSm/WCpYkNxXmYr/vOpKHN3h/d8jFOPe; 4:Hg0sTlyrsrFQrx18GrxYv4y4FDj+ypF09kdCZkCktcD2VbMnh+LS7bndGpTtXiBztyoMJAliyy4oKwvtXsoSET+vBlTdMgkHttRhlNvyc0kalDX4ltzytYkoeZiJssjZHHY8PiVeToZ9gQn+Qw82vYVB+MQtEePZ/VN20zTnAkphHVp7HVdvZEolC65LikAjcCrTX8NpEbN3tQ+vj1nLpuuQ+IOFEg+3/zod7dDDLjcvUyAzl2bqC81R0GHRkUH2w/GDW+2A1OcEeV9aU/7USTTGqUlRJwctmNlWi/n3g32xReWjuuxsIt25h4KkTzJszs4wYotXQrYoGgnxR0Ff83SNbZquza7s66U0Yx9Puys= X-Exchange-Antispam-Report-Test: UriScan:(767451399110)(211171220733660)(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)(8121501046)(5005006)(3002001)(10201501046)(93006095)(93001095)(100000703101)(100105400095)(6055026)(6041248)(20161123562025)(20161123555025)(20161123564025)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123558100)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:CY1PR12MB0151; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:CY1PR12MB0151; X-Forefront-PRVS: 041517DFAB X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(6009001)(39860400002)(189002)(377424004)(199003)(24454002)(377454003)(6486002)(53416004)(2906002)(189998001)(23676002)(106356001)(33646002)(42186005)(305945005)(105586002)(4001350100001)(83506001)(47776003)(86362001)(65956001)(66066001)(97736004)(31696002)(478600001)(65806001)(36756003)(7736002)(6116002)(3846002)(8936002)(230700001)(81166006)(81156014)(8676002)(53546010)(68736007)(64126003)(54906002)(31686004)(50466002)(53936002)(110136004)(65826007)(6246003)(229853002)(2950100002)(25786009)(5660300001)(50986999)(76176999)(101416001)(4326008)(54356999); DIR:OUT; SFP:1101; SCL:1; SRVR:CY1PR12MB0151; H:wsp093899wss.amd.com; 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?MTtDWTFQUjEyTUIwMTUxOzIzOmtTbGJHYURaOEdKaUVOcnFYM1o1Wk5xcWRr?= =?utf-8?B?ZHRaRXhtK05MbDRWZ0RNWVpOODJ5ZUl4L1lUSWwxMHNRZXdFNzcvNVUyVk9h?= =?utf-8?B?QXlaSVc4QWt1RWpObnZINjJXYkcrK1VYdmYrblRPdUllMTBIdTNxTFpmcDBY?= =?utf-8?B?VVdORHkrd1A1L01rQk90TTVmSS9MTWJoaGNRR0dRTG9TcWMvdTF4QzN1QTM5?= =?utf-8?B?N0dxcytKNk9UaVlMV0pFVDNNR2tpWWFTYXdDdW9SMDVjeWlSUkNMZlY0WDNC?= =?utf-8?B?N1hjd1RmUG9wNjRpM1MzMlJNYzh5S2RTc1FUUUlnU01jdUNyVFhpM0ljRE14?= =?utf-8?B?Q1hJWUphUit6bUY0cktjWGpJWWZST1ExbzBQdGJUNWZkWGpQKzI1VWR0WEFW?= =?utf-8?B?Ryt4MGo1WVQ0VXJXRGhmK0tWT2tLaGxHbHltaXN2WXZRM0xvNjRxbXQ4VVU3?= =?utf-8?B?dFRQaXBjSC9TS1BkbTlLTjZQMTdsRVZ3cUt3a0lKUWJyckpydzRtb3JrK1pL?= =?utf-8?B?bGREZGFOVUdjRHYwdXlTQi9uSUc5RWVhcnovRHVDL01xSDY5QWlsYmpSZ05n?= =?utf-8?B?Nm5pRkhKRUx4NDVha0VEUk9iOEJ0Z1VtNlljdjlRZ2xVa3ZhMVRKb2dmVlFQ?= =?utf-8?B?WDhwa05hUlgrZGZXbTlnTTFjWHNmMEs4UDFlc2JBZVlSYVNKVGZEMVdkOXhI?= =?utf-8?B?WjYrTXgzdXhMeG5ycjJiUUFzM2NibnlKREZET3hhM2RRVlNLZ2xEZk5NRk5z?= =?utf-8?B?YjRhTDF1aVplcmVXTVhjcWFMZS9hQlJ2TGtHaUZld2c5ZnVsaFFTd1N4SUpG?= =?utf-8?B?bmU3ektDd0tUak5aRGNGbCtMRmxYb2NkY2t3MHZCRzJnWlZkSk9YbFRkVlJD?= =?utf-8?B?aGJPWHJ4SUZIVHhjMkhUaDRxbG5Zb2h3VUFKcnk5U1cvM2Fkbi9sKzVUcFZi?= =?utf-8?B?YWR1d0VUNTRyOU5paWRXZmRjTE1mZVNvOXdOYUtQMitxNGZoenBxcEppaVVk?= =?utf-8?B?eDZlTGZHMDJHRzZOMXhwaWNHdWNzMWI1WFlKZVpqaUQ5akxEMlF0VjUwTW56?= =?utf-8?B?WE1ZSnZZVjVzQXZWQm96b3NRM0tSQi9RTUxjVUxlRW9OU1o5QTJNVlpqdURD?= =?utf-8?B?bnE3N1BvcFJQYndYQTNTb1Y4Q1FXN0Y4Y2hMU3daL0Y3YUxPQ1NqVmtlQk45?= =?utf-8?B?d2g3L1h3bXdJYzNMRWlTVUVvcVZ2NkpnMzB0ekhrNzdleEJlYWp0TGNkRnVj?= =?utf-8?B?K2xiT1pPbEhlbEVPRUora3pOTDQxZEhxcjVpSFNPa0Q5d3B0dWlVaUVKVXFC?= =?utf-8?B?eGRTWlRTWWdJTVdPbW5aN2JWTDZ0cytPTUFKZ3JzVEN5SnpOWUNQTDhlQUxu?= =?utf-8?B?TnBMcTFHQUR6d09ZODk4eXpKMU91SWw0d1JlR0tKbXpEZnFRenNvSDJoTnlk?= =?utf-8?B?KzVQZUVFWUZ4UHJYUnhGMGpNZ3hzWGxzdWNKQ1dWMUcrMUxtV21wdWQwQnVG?= =?utf-8?B?a21wNHJOS0lkN1UrWUw3QTBTeHVnVXJ5Ly9qMnFacGM0UGx3N2wreDI4V3hQ?= =?utf-8?B?RVg5eThJWG96cjJ5QndTQXRzVmFkUW4rZms4MmVxT1JhVThwWDVRVUtBb3d3?= =?utf-8?B?cC9Ba0wxaXI4eVM5MHNPSGQzL1o1Z2lIL01Ed2tDQ0NPOGtTS1QwSldYNjhv?= =?utf-8?B?dlFlMUNYL3J4bytyc3R1RlgvcXFsOWFRWWZnQSszdFlsT09VbVk3MVBIWGtV?= =?utf-8?Q?M6zgJxedaOH5OPcdxafay9022OQDR/dj5XcTo=3D?= X-Microsoft-Exchange-Diagnostics: 1; CY1PR12MB0151; 6:ehw8Y/0b7YrWISBIqQ9QAp++xPfH7XlxiCCAtMqCjcqFJ+rXOdS37o0sBoa7sxdy5DnPfUeO7M2U2TztzspaPoUEuaL3fBF7lelP9hy/WTCFJUgczEjZ1O3k9YDktdegbCG8UYP//k0fgQF5jiaRtbhOT22nHR/nQPJEeAFXXMVlwylavWTaiE3qmwD7C7Dwnev539cA4QxO0eBB7U+toEzb119wARVUuhdN4YyQAoipUN1izbA7gmvyMHjrhmDW+e5ghxU21Hm6vRs2Lczpqajjq7k+pQaw3x0kIETqJheVmC0BJG3PScYDTyZF2q5mlV4a1OtnAl4aLTA/aAitFw==; 5:voj04IxwtsTxu9XtjSR7avWPmsA1NHfh/AEmUghaHy/KVjuTblhDL1SWU5s6klxDuQzi4eAQsJXyyu9Kfm+dTFofb0DSYwYH09a8Gh15J9527rxNc9WpPa9gbh+Fd5wCqtxUiwrmO6LW5CpiqMbWnA==; 24:NK6G+4drF9tn8PevWk2J6bDpagyva+avCdd8Aj3N+UuSWJgIzwKgl6/x4bBI6qDqb9h7q/8z2sM2Kze39drIIv4mFUvqubhbhQnOW2A+l4w=; 7:5631kIDisMFfeMbDPNhi6r4ZyNTiJB1gp6U+z1LqV5ekv+gzdyDQOrZ+Mmw0vNxZbPLWvYLwSIB3Th7ppe7yfo0xD19BWx8chGmF/znezdw/oGqmeSxJ5VGUcSJLlX93zoxaNHqlU+y9KDQQVfjjdsC90Sbvzz4YKzgCEb6HhMsmdcrn2LMEtK0ZrkbZ1zIdg9v7U6dqjq5uG9R/EzKHiPkHuHEIX6Fh1hrxJgHl4ew= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; CY1PR12MB0151; 20:alVYGUlCJAMTAeTnl8qKOUljjcKomtUtGGJHX4jIS0Ws+t90bwxNmxtcc1OcG6fLdzAAxK4Wr3oox8dLFTLq3thLwSTq6hZXo3qojjDVEJjhMbpKD1lwkjBYshG5RtpcQGk5n8h95jSpgKMxB0gq2KaDkXK3eXJnAuSX5Qic/NVbSjrSOe5ZoLA4zI65UPL2oWFJ1SOenXOVx9Fio2DJ4oNsLbeOk2KceAEYPIH1IBn4VmSyKocLby52OwkBgvxJ X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Aug 2017 12:21:26.8199 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR12MB0151 Subject: Re: [PATCH 2/3] Ovmfpkg/VirtioScsiDxe: map virtio-scsi request and response buffers 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, 30 Aug 2017 12:18:49 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US On 8/29/17 11:02 AM, Laszlo Ersek wrote: > Hi Brijesh, > > On 08/28/17 13:26, Brijesh Singh wrote: >> When device is behind the IOMMU, driver is require to pass the device >> address of virtio request, response and any memory referenced by those >> request/response to the bus master. >> >> The patch uses IOMMU-like member functions from VIRTIO_DEVICE_PROTOCOL to >> map request and response buffers system physical address to the device >> address. >> >> - If the buffer need to be accessed by both the processor and a bus >> master then map with BusMasterCommonBuffer. >> >> - If the buffer need to be accessed for a write operation by a bus master >> then map with BusMasterWrite. >> >> - If the buffer need to be accessed for a read operation by a bus master >> then map with BusMasterRead. >> >> 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/VirtioScsiDxe/VirtioScsi.c | 168 ++++++++++++++++++-- >> 1 file changed, 152 insertions(+), 16 deletions(-) > I have several comments here. I believe that this time my comments are > best kept together, so I'm not going to scatter them all over the patch. > > (1) You have a typo below, "scsi-blk". I think you meant "virtio-scsi" > instead. Will fix it, thanks > > (2) "ResponseBuffer" is not adequately zeroed before it is mapped. The > Response->Response field is set correctly, but the entire structure > should be zeroed, after it is allocated. (Practically in place of the > ZeroMem() that you remove at the top of the patch.) > > For VirtioBlkDxe, this wasn't an issue, because there the entire > response buffer was a single byte, "HostStatus". By setting that byte, > no stale data was left in the area that would be exposed to the host. Yes good catch. will fix in v2 > > (3) By reviewing this patch, I realized that I missed an error in commit > 3540f2cef57a ("Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response > buffers", 2017-08-27). > > In other words, this point is about the VirtioBlkDxe driver. > > Namely, when "RequestIsWrite" is FALSE -- i.e., the CPU wants data from > the device --, we map "Buffer" for VirtioOperationBusMasterWrite. In > this case, checking the return status of > > Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); > > is critical -- we must not do that unmapping on the error path only. If > the unmapping fails, then the device's data don't reach the caller, and > we must fail the request with EFI_DEVICE_ERROR. > > I believe the mitigation could be: I will send separate patch to handle this case. >> diff --git a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> index 6abd937f86c6..5a63986b3f39 100644 >> --- a/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> +++ b/OvmfPkg/VirtioBlkDxe/VirtioBlk.c >> @@ -260,6 +260,7 @@ SynchronousRequest ( >> EFI_PHYSICAL_ADDRESS HostStatusDeviceAddress; >> EFI_PHYSICAL_ADDRESS RequestDeviceAddress; >> EFI_STATUS Status; >> + EFI_STATUS UnmapStatus; >> >> BlockSize = Dev->BlockIoMedia.BlockSize; >> >> @@ -430,7 +431,13 @@ SynchronousRequest ( >> >> UnmapDataBuffer: >> if (BufferSize > 0) { >> - Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); >> + UnmapStatus = Dev->VirtIo->UnmapSharedBuffer (Dev->VirtIo, BufferMapping); >> + if (EFI_ERROR (UnmapStatus) && !RequestIsWrite && !EFI_ERROR (Status)) { >> + // >> + // Data from the bus master may not reach the caller; fail the request. >> + // >> + Status = EFI_DEVICE_ERROR; >> + } >> } >> >> UnmapRequestBuffer: > I'm very sorry about this. If you agree with the above suggestion, can > you please submit it as a standalone patch? > > > (4) Back to this patch. The EFI_EXT_SCSI_PASS_THRU_PROTOCOL.PassThru() > member function is required to implement elaborate error reporting, and > some output fields must be set even if we report EFI_DEVICE_ERROR. The > pre-patch code conforms to the UEFI spec's requirements, and we should > keep that up. > > Please locate the following code: > > // If kicking the host fails, we must fake a host adapter error. > // EFI_NOT_READY would save us the effort, but it would also suggest that the > // caller retry. > // > if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, > &Indices, NULL) != EFI_SUCCESS) { > Packet->InTransferLength = 0; > Packet->OutTransferLength = 0; > Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > Packet->SenseDataLength = 0; > return EFI_DEVICE_ERROR; > } > > This entire block of code should be factored out to a helper function, > in a separate patch: > > STATIC > EFI_STATUS > ReportHostAdapterError ( > OUT EFI_EXT_SCSI_PASS_THRU_SCSI_REQUEST_PACKET *Packet > ) > { > Packet->InTransferLength = 0; > Packet->OutTransferLength = 0; > Packet->HostAdapterStatus = EFI_EXT_SCSI_STATUS_HOST_ADAPTER_OTHER; > Packet->TargetStatus = EFI_EXT_SCSI_STATUS_TARGET_GOOD; > Packet->SenseDataLength = 0; > return EFI_DEVICE_ERROR; > } > > Then, in this patch, *whenever* we intend to return EFI_DEVICE_ERROR > from *before* VirtioFlush(), do: > > /* attempt shared pages allocation or mapping, as appropriate */ > /* ... */ > if (EFI_ERROR (Status)) { > Status = ReportHostAdapterError (Packet); > goto ErrorHandlingLabel; > } > > (The same should also be performed when VirtioFlush() itself fails, of > course.) > > The idea is, if PopulateRequest() succeeds, but we don't reach > VirtioFlush() for another -- new -- error scenario, or VirtioFlush() > itself fails (like before), then we must fake this kind of host adapter > error in *all* cases. The above helper function will simplify that. > > > (5) The same issue as (3) is present in this patch (i.e., for > VirtioScsiDxe); however, it is more complicated here. > > Assume that > > - the caller submits a bi-directional request, > > - we actually perform the request fine, > > - but then we fail to unmap "InDataMapping". > > In this case, flipping the return status to EFI_DEVICE_ERROR just > doesn't cut it: we'd have to update the Packet->xxxx fields accordingly, > so that they report the full loss of the incoming transfer. This would > be hellishly difficult; the update would have to be different for a > bidirectional transfer vs. for a simple read, and in general, who wants > to mess with SCSI sense data? > > Therefore we have to guarantee *in advance* that this error won't > present itself. Which means, of course, a CommonBuffer mapping for the > input transfer (if any): > > (5a) If "Packet->InTransferLength" is positive on input, allocate a > shared buffer, zero it (!), and finally map it for CommonBuffer > operation (--> InDataMapping). Proceed with the rest of the patch. > > (5b) If VirtioFlush() fails, then do as before (see point (4) above) -- > report a host adapter error. > > (5c) if VirtioFlush() succeeds, then call ParseResponse(), storing its > return value in "Status". > > (5d) Now, ParseResponse() will *always* update > "Packet->InTransferLength". Therefore, after ParseResponse() returns, if > "Packet->InTransferLength" is positive, then we have to CopyMem() > "Packet->InTransferLength" bytes from the common buffer to > "Packet->InDataBuffer". > > This way, we'll only have to unmap BusMasterCommonBuffer and > BusMasterRead mappings on the final path (both on success and error), > and we won't have to care about their return values. > > Also, on the final path (on both success and error), we don't have to > touch "Packet": > > - we either reached ParseResponse(), and then it set the output fields > appropriately, > > - or we jumped over ParseResponse() due to an error, but in all such > cases, we called ReportHostAdapterError(), so the output fields are > set again. I will go ahead and implement your feedback in v2 and send it possibly today. thanks