From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM01-BY2-obe.outbound.protection.outlook.com (mail-by2nam01on0078.outbound.protection.outlook.com [104.47.34.78]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A5F3121EB529C for ; Thu, 31 Aug 2017 07:41:55 -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=Kz6jg+6pUA/+/oAEy3tqGpNRDXATFShJ2vpI5MKoM7Y=; b=YLT+eY+PdPZKkL8VdH2cWHXOiSMRHF0z2OHZhCbciaVDTHZoYGtzoCVMDjeiqaKAV85x8/z4Po8zaKJLjBQWZWxSPSvjZR32/kuMJ6F3qpCB38VfeFft1rtnCd7HJtKnkxBrtWs44Ll6QKMQVT8r79zshgcKlL67Bi3k1EKxPm4= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Received: from [10.236.136.62] (165.204.77.1) by DM2PR12MB0156.namprd12.prod.outlook.com (2a01:111:e400:50ce::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.20.13.10; Thu, 31 Aug 2017 14:44:35 +0000 Cc: brijesh.singh@amd.com, Jordan Justen , Tom Lendacky , Ard Biesheuvel To: Laszlo Ersek , edk2-devel@lists.01.org References: <1504125903-29816-1-git-send-email-brijesh.singh@amd.com> <1504125903-29816-4-git-send-email-brijesh.singh@amd.com> <3e3e8174-616d-8f34-7682-02f6e492fb24@redhat.com> <885cf87f-eab8-a429-cf40-eb6905d8378e@redhat.com> From: Brijesh Singh Message-ID: <9716c503-28a2-9de7-64ed-92768343d53b@amd.com> Date: Thu, 31 Aug 2017 09:44:30 -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: <885cf87f-eab8-a429-cf40-eb6905d8378e@redhat.com> X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: BN6PR10CA0041.namprd10.prod.outlook.com (2603:10b6:404:109::27) To DM2PR12MB0156.namprd12.prod.outlook.com (2a01:111:e400:50ce::19) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: d297a34c-4c08-4338-332f-08d4f07ecd71 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)(2017052603199)(201703131423075)(201703031133081)(201702281549075)(300000504095)(300135200095)(300000505095)(300135600095)(300000506095)(300135500095); SRVR:DM2PR12MB0156; X-Microsoft-Exchange-Diagnostics: 1; DM2PR12MB0156; 3:HzrkLOCS2sRVMCkNaQGJvK5Yjq6SY6TpKRufSlKSOWSa/ojQn+7ZWzmy2kLvazC7Rwlx1LBUtbPzSfV7AhbWB17Tnk1U7oS111FxKdZ6HoPThQcyE1K/ddotCkPg234yOQ8RrIzeALuwxW0nDjUl6840HG4+PHpHW78U5oTtaaexIUrtR/LIGH1eoUUbt18UNUvfcGkCxAsm8mFct/J3ZZ/qDWoVwrVxDUvTpdXJglhc29JjDURCtL3NcT+CVgfk; 25:yuIyRuaYhWDzewLCcT1QVy1ox8/+/uWh9gAoennxgVTNA8AAqYN5FrXh0PJNw6WN8a9uvP/YV1QF9ur6FecIn4eo6r84J19qdt7bW3vHTAa7nxKc43qgi2hgnFyIu0ONtn88oCKZMlP1E8FE3+phdYNZcEaailUqoTlZwRIc4Psjt3rj7vvlZx/+UfJzCdK0yMHkaD5pzh/jPYerDhpdJio7t3CBunkiSWJDWiccrrphrxmChYONN1SGHsBjxvmqmYt7gKbUw32j8Tpj342mp8BFhdX3auOPQMaZ/EXbdK4jFeSmnx21NlTeBFIbe9r+kXcT6ITsbdIj1UhGy9NzoQ==; 31:YumV2H1V6GyuoYEhSVgyQIXieSVpS10dO3cJGFxCg8lFk12tgphbN4dat0iGRTUaQr6aCvlQHTMMf3xgjGyhxVATmOYCuWxm0g53xdQFthlWhEhsNm4Yx2BjWw6egZ4i0guWBkCFUKSYocHg38RpV9sKCCxx57sutTU2V1wYaJycDN+e6DO/CapWLlTXdGN754vBQbFh6r/tFplCdtpK8Z8ZlyWoL4BCqn4G9TdDsVw= X-MS-TrafficTypeDiagnostic: DM2PR12MB0156: X-Microsoft-Exchange-Diagnostics: 1; DM2PR12MB0156; 20:iBOT0LgL4+TKrh6cbVLlKFyN0uryz7JC/aYeEkP6gOLXNu3n9hbJaRUIz6gVb5FBmZClU+S68VYG8tg0kIXIUL3zFtdlh0aME3u0IXHmWSdF5jSqTP87LtlxbCKNTTKT0VCZJSsF8N2veIS2g76uxe/C6oSbKJvUtuTKuCCBsCn5LR/4W7GQO3+n0vvWB3W4FwfyaD9kX7pxZx2Uwe1dsM7e7SmQ9++45HjH+Fno/YIMymIzYZSp7G4bVrJ45P0lvVhbMHz30B6PczaHWprwi4YDuv4YgrW4Wi16UX/gQzkFuMDnxjz8UpBOUc7FeqmpK2KoNc1LVvnCzHS33emoTEZsqmlyKUPngEH6vbnIaJfIVuPyx4qKLFmFoe5V3O7ZyD+zU395N/UXiT4pjNfTLusu81+DXClj0vbNur9OIjRIk25Gez1F0zlSJTfnxh8s2TuC+Y80maY9GpiAPUoxufOA0Ss4Ub1wy+ECnXuar74kVmzvJNc/X0RqVGiwBihx; 4:EIIZ7nsdqhXeenTu1w0GbNIsmMnTeBOE0y8RqryqIhKtseAW1WVQibL1eVtu4VNYrgXjy/mTj17XnfWWv0nBQxpejBSZXYpUOr0whiskgKGc1pccV8LpKPQVpXJzFSR8q2bgXB1un6jULwJ5FUWmmu6OaQfvz9vTLuw3p9NqO1ZYwsjblcBvq1qVPlNfEJeRqa9PVCdY1+anH4tYN99WP6nXQRFsXiB1Ms0lWTchINYVs1AsyfCuqMhTjtLzHMDk4Mq+YGpheC46nBBvOr7RbKSg9O/SZdDzsUp0DrzBFRY= X-Exchange-Antispam-Report-Test: UriScan:(20558992708506); 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)(93006095)(93001095)(3002001)(100000703101)(100105400095)(10201501046)(6055026)(6041248)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123555025)(20161123560025)(20161123564025)(20161123562025)(20161123558100)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:DM2PR12MB0156; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:DM2PR12MB0156; X-Forefront-PRVS: 04163EF38A X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(6049001)(6009001)(39860400002)(24454002)(189002)(377454003)(199003)(76176999)(54356999)(5660300001)(8936002)(97736004)(65826007)(50986999)(86362001)(6486002)(47776003)(65956001)(65806001)(66066001)(4326008)(42186005)(81156014)(53936002)(110136004)(81166006)(93886005)(23676002)(8676002)(83506001)(7736002)(77096006)(229853002)(6246003)(305945005)(4001350100001)(54906002)(2906002)(3846002)(230700001)(6116002)(105586002)(2950100002)(189998001)(101416001)(50466002)(36756003)(6666003)(64126003)(31696002)(33646002)(31686004)(53546010)(68736007)(478600001)(25786009)(106356001); DIR:OUT; SFP:1101; SCL:1; SRVR:DM2PR12MB0156; 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?MTtETTJQUjEyTUIwMTU2OzIzOlVZamRqREkvMDdEalNnMUhoWkVKbjdXMWFC?= =?utf-8?B?bDk1QkRRSlNZdGdnQUhOZDZtL1UvQU1ZdWRNbjdQdlJOZUZxclU0S09WV1JG?= =?utf-8?B?NXcrSjY3RU50WkhPYisrOFk4VGIvZXloRVphOFR0NlJNM25uUmdIWEVXb0gx?= =?utf-8?B?akxTOFUzekNNbkdxSFZBZWlMbUxwNmlWZm4xRXNIL0RLQ0N0TVVwWENhUktt?= =?utf-8?B?ODlqM2pMdkZUaEREMU5sUmdRaFRySnd5SjRjeUx1RFNlZ1AvU3VqcVo4b0Jp?= =?utf-8?B?S0gzNzlzaVlkbVJId2FRaXVXRW5SV05iMzNrWndTS2xGRVIwRStoNnVzSlgx?= =?utf-8?B?dHNwOTQxMFZ0My9odEpndUFIUTJPS1VmYTl6REFUZlRiOVlOL1BIdEFDaUY3?= =?utf-8?B?YUQ3Z2xkWDFINnM1d0p1RVY4RFdxTnY4TnNCTGtrNGR5Nys2dG1CK3ZFeWRE?= =?utf-8?B?SG9kd3FHdmMxSk52TEZCQ1BMbnNmcVV5YnZ5T0hkQVV6M1pvTjRmNE1GNzVa?= =?utf-8?B?bXhCdWVtRk1DWjlNdkhLb2tpeWhKQUoreU9BditieHdIak9pc2puL01SU2dC?= =?utf-8?B?blQxRUxobjRuNm1STHVuTWxlc3c4MnBNVVRGMGFtT2M5eXFwYithK1ZseGRV?= =?utf-8?B?Nkg3NWo4R3VTdDBCV05KTElhem4rUS91dHdDMmx5K3VMbGUzZHlSc2dwZHFP?= =?utf-8?B?dloyWDFBdkZtSDF6ZG54ZTIxRzRDS1NUOGtIQXo1ZWlnL0I4S3o0eWc5cW1s?= =?utf-8?B?SUtMNTBFcmNwSnZsVFlPdklWR1VyVXZGeE04K3lFOFF2NE00YXpIcnJCYjhr?= =?utf-8?B?d2pRRUQ1T2FpOVhpQ2NZN3E1YW80YitGUzNUV0k0eVNkUVBMVHFZNWRSMEpY?= =?utf-8?B?WTJsK0Y4UGd6a0o5L2NTL2VtZzVwVXB0cG9SS2tWbk5NMnYvWWoxY21rM2JS?= =?utf-8?B?bXQ2eUFmQzh5eDQwbGhiMU1rRTlCZ09kYTRRay9tQ21ZcU8va1BMWmJIWGoy?= =?utf-8?B?dEFrVUlwVVNGaVdkUm9YKzNsbmlEL3pVRmRRQmFXdng0T0pPaUFQVitxNXp6?= =?utf-8?B?QVNZWnQ3YVVpOVpybGdqWXJmZnlmZ2N3ME1QNWM4MUZQcDhFL0FNb3R0dXdw?= =?utf-8?B?QjBnZ1d2aUN4MzlnYWJqMjlSejNBcXlBWCtxaFhWRGs4aXdlSzhwNDdxUDRW?= =?utf-8?B?YTlEWHFoeFFpQmZabG9yMXV0YUNycTFGODRIQ2luOEtJTHZuNDF5cUM2aWNZ?= =?utf-8?B?eXI4RE1lZUVMZW9WUyt3M1hMRDNidTlGdlJ6enM0d1oxaWQ1UlBIa0YrUFg4?= =?utf-8?B?K0N2YzdUai9aNlROaXRQOGZ0U1dhdElGMTNSQWgwVmtJc0swTmFYc0c0c2FU?= =?utf-8?B?cnZ3RkNxQlFPbzZqbFAxNDRpTUdqSnhpaUhPMXFYQVlPdGxDVU1PS3MwOVFH?= =?utf-8?B?dDFISnJZQVVONEQ5b3FtWHRBQXozUWt2cEdVWHh5YUZlS2pYSHNybDBNbzZR?= =?utf-8?B?NU14OXRsQmJwY3BzZ3VWb1V2UCtMcXlhcjRoU2NMRUhiclcvbXhuN2hNSTR2?= =?utf-8?B?cyt3WC9kQlZzSjB1TmNsRzNBbW9uZ3JIVVk0NnB3VUVMNmYzV1JreHZiZTNN?= =?utf-8?B?WnlWZXpZVFNpSG5LWnpiaStRYXdOQnZUYlZsMlpkRHJOSVFYN2dUcU9UeDJ1?= =?utf-8?B?YmpSUmhwcUM1RmJ1bThYL2hXQUIxa2dvcmZ5M09tYlRxZDdDRWRDaFdUazdW?= =?utf-8?B?SHVhQldudE4vS0lCOUZKZG5VWCtScjgvczhGdmc0dStlTG1hbWg0cUoxRjIw?= =?utf-8?B?RTlUQTgrY2I2YUdSQnE5TUFsRnZVaGZ6L25DL3N1ZHBnUmc9PQ==?= X-Microsoft-Exchange-Diagnostics: 1; DM2PR12MB0156; 6:FsmvlTPiWD3rIE6k8xZcfugn2HcSIVsEESEJBZqhF7GGl8/ZPQoaedTurOYpzlsSbTRx6P9MTYQHxVHcNQH05znuOfDTMpU4xTjRmKwOIfrDcuU/bB9yYdPgugmBiHd9sOoxBodSXLCspt8dbfDt6A5989tAtfEkcDBbxqq6qyzTmdsuAT0q2WXQfyR4pSd9DClbXUVO8YMuTqC6BJsmItNabyVvuzC98N7wfW7FqKNL8hqyFs+uqXUADZogQZ8nUIYFzhjo4Ryq5hfDFQT6EfUqRPu7Mg9dexMbiDsiC0ZvJMfcCIFlZa3x78FJwgHGyUYLEJV81BlMWPR8sD3jRA==; 5:2vVCSmfS8hGWjVLRIf3jEOghJwhxhRkK2HDpI+cYvurdu2XZPGx1iuVG5b70ZXhcmq+v6qrzxv3j0quHv9GPQn5Bhs+LiKtcIvcdFI5LhbWIQUH5BeQo5zzsAZ7JpYRCy6zih8K6OQHZLxY8LRfSjQ==; 24:55CWY7U1niiUXtKWry8Zk5Kt3BnXnOyCnIBv8yxIKLQZOtqDYKTIKV41FG/OFYjer+aZpM3EVq4wmKin9C5bcaCBccW5YbCtaz0BegO7z3o=; 7:H0pp5z15WcRSK0I4ZKWKUm0Gw5X1SiI9YUTuqTEvFWi7IUwk+089UD/XCHJVDxn38aomShuAedjE/Lz7qVsxV31OYMjS4BHFCN6qo9sfp7PmuwdRIJI8fMP3cQirKoB/yn4zg84aUZtuw7MsGrDc2FN8uI8nNeg3eVTgieGoXgyqew/IPFWpL552gywExcoJviD6IDNK8chMaRdleNa1q2IKZNQhDZXzqoLBRmKLl3Q= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; DM2PR12MB0156; 20:Efr9zZ9RWFkbyusmOEqEk0bG++XXUwKoAqUG+FMCcESayf5yZXEkxqET1k6SGzilG9RJNgnebvJG8BiH6Ece5LoUfJRi8mw06DmKzI6T7KEGY0E6JFA6xqXyuLJgeiiPGVRFpdRtrlAv/8wyDt+INwX4WQ2Omw6sY5XK+LacQF+2DW6xeGQn91PKeI9sOlHzG+MNYQoRtf65eJeibqqF4b3OiKhbYi99u0WDLoZU36u9yfPD9bTHK7EPH5lwKCRz X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Aug 2017 14:44:35.6407 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM2PR12MB0156 Subject: Re: [PATCH v2 3/4] 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: Thu, 31 Aug 2017 14:41:56 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Thanks for quick feedback Laszlo. I am addressing all your review comments and submitting the v3 soon -Brijesh On 08/31/2017 08:49 AM, Laszlo Ersek wrote: > On 08/31/17 15:23, Laszlo Ersek wrote: >> On 08/30/17 22:45, Brijesh Singh wrote: > >>> @@ -492,10 +645,50 @@ VirtioScsiPassThru ( >>> // >>> if (VirtioFlush (Dev->VirtIo, VIRTIO_SCSI_REQUEST_QUEUE, &Dev->Ring, >>> &Indices, NULL) != EFI_SUCCESS) { >>> - return ReportHostAdapterError (Packet); >>> + Status = ReportHostAdapterError (Packet); >>> + goto UnmapResponseBuffer; >>> } >>> >>> - return ParseResponse (Packet, &Response); >>> + Status = ParseResponse (Packet, Response); >>> + >>> + // >>> + // If virtio request was successful and it was a CPU read request then we >>> + // have used an intermediate buffer. Copy the data from intermediate buffer >>> + // to the final buffer. >>> + // >>> + if (!EFI_ERROR (Status) && (Packet->InTransferLength > 0)) { >>> + CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); >>> + } >> >> (7) The comment is exactly right, but the condition that you check >> after is incorrect. >> >> The right thing to do is to call CopyMem() *unconditionally*. >> >> Namely, at this point we are past ParseResponse(). As I wrote before, >> ParseResponse() updates the Packet->... fields in every case, even if >> it reports an EFI_STATUS that is different from EFI_SUCCESS. And >> whatever we expose to the caller through "Packet->InTransferLength" >> *must* be reflected in "Packet->InDataBuffer" regardless of return >> status. >> >> Therefore the Status check must be dropped. And then we need not check >> (Packet->InTransferLength>0) either, because the CopyMem() will deal >> with it internally. >> >> Think of it like this: the "worst" that can happen, on error, is that >> "Packet->InTransferLength" is unchanged from its "input" value, and we >> overwrite the caller's "Packet->InDataBuffer" entirely. What is the >> data we are going to put there? It's all zeroes, from your >> >> ZeroMem (InDataBuffer, Packet->InTransferLength); >> >> higher up. >> >> So, again, this CopyMem() needs to be unconditional -- as the comment >> says, if the *virtio* request was successful (== we talked to the >> virtio-scsi adapter), then we have to copy the data, even if the >> *SCSI* request produced an error status in ParseResponse. > > I have to correct myself a little bit -- although I think you would have > caught me anyway :) --, namely we should keep the "if", but the > condition should be: > > InDataBuffer != NULL > > Admittedly, it is likely that none of the CopyMem() implementations > would have problems with a NULL "SourceBuffer", if "Length" was zero. > > Nonetheless, the interface contract in > > MdePkg/Include/Library/BaseMemoryLib.h > > does not mark SourceBuffer OPTIONAL -- neither does the UEFI spec, for > the similar gBS->CopyMem() boot service --, for the case when Length==0, > so we should do an explicit check: > > if (InDataBuffer != NULL) { > CopyMem (Packet->InDataBuffer, InDataBuffer, Packet->InTransferLength); > } > > Thank you, > Laszlo >