From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from NAM03-BY2-obe.outbound.protection.outlook.com (mail-by2nam03on0080.outbound.protection.outlook.com [104.47.42.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BDD5A21E11D34 for ; Sun, 27 Aug 2017 16:15:47 -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=fiw9o1GcJthRfSPnCZFfsPkTfSXyeYgP+3VMC96Goss=; b=w+OY/hAPPyobw2HkZ7EfGK5NV8VHom+YPRT3CJcZ9gN2td89nP81zDmSR9TVE1n+5CdGJVE/pM4Bjs9qwac3snh/8fe4rcAz6drALNHB7BEjZkIG0eL86FZQ0wKxj1HlY9AoyN8zGxsMDQyFrjwIIi0IC0D20xLkSGSvne7EOdk= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=brijesh.singh@amd.com; Received: from wsp094247wss.amd.com (165.204.77.1) by BY2PR12MB0145.namprd12.prod.outlook.com (10.162.82.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA384_P256) id 15.1.1362.18; Sun, 27 Aug 2017 23:18:23 +0000 Cc: brijesh.singh@amd.com, Jordan Justen , Ard Biesheuvel , Tom Lendacky To: Laszlo Ersek , edk2-devel@lists.01.org References: <1503697414-6830-1-git-send-email-brijesh.singh@amd.com> <1503697414-6830-3-git-send-email-brijesh.singh@amd.com> <61e3119c-0e51-ea05-96e6-55c4908dadd4@redhat.com> <1d6bade7-578e-c0e7-5aa6-2ca6af1185e6@redhat.com> From: Brijesh Singh Message-ID: Date: Sun, 27 Aug 2017 18:18:15 -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: <1d6bade7-578e-c0e7-5aa6-2ca6af1185e6@redhat.com> X-Originating-IP: [165.204.77.1] X-ClientProxiedBy: CO1PR15CA0061.namprd15.prod.outlook.com (10.175.176.29) To BY2PR12MB0145.namprd12.prod.outlook.com (10.162.82.18) X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 178ac48f-5167-4ba0-5cfb-08d4eda1ea3b 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:BY2PR12MB0145; X-Microsoft-Exchange-Diagnostics: 1; BY2PR12MB0145; 3:4eTNo5UA6cC7vlrHRFrzjqbnMpw55XoFsqiGRpN+VUx/n1Q4rtmrAvrdtenbv7D0bSLcISHWABbGwm9d+XhcT0oN/AFBq+DdlP7BQr2bIrj43fFQxXyYNM/qJCjJY5UsKQAKz2wypILK2MhCyl7YeRy+b7LZIaodtZ9/1XDJABkEnQdPFgS1RCfiS3pjns7GvVE1+vhEYvLaeqvEGsAeZ8Evc0TM7cBC+wIO8NY+IEoFF+Ft3anZEBdsgwrCObFs; 25:Ai+fAIGpQjv39xC1QozMUmqHUDXyKy1YMJaW1cnzgQVQbRZiOJx8FF4NUCPuuBRKI++UrWbJtd3Ra6ATJ9nWXebDp9IIG2gMzO2EpyfOlkhqjxHCoLIrfNPLqjfbrY2W3xRMA7oXzqSxnIDABWkJYAJ6zHEoVoRvcj/osjGoK0srscCwq8sNNWuHhKJviiJQyrq6g11OLRmEnaQrLFuN/t1TGShjKf2pVIlrFsbNPQbfIr4s4uRnnSwUyWC73wkf/39kbB+9jeKS7PxahsWObuMwUfl709yMaY+ctJPb5abRHWv+6Utji9NYQAIiwQq2ElxHN6d5f3rdapO/sQtsXA==; 31:fntIL69wuFbqjWOYlDMJ9wumlFvh7fdgR2usuowgw004+vBKKphDdiZPzX2/t7/28zHR9m6EyrVCOVk7Pd9jknqd4pL1m/9zvt8GahfCjeEQhKUgUu/1o3IQ7+4RKMrTRcQEUmNSfcOEYzMe2mxbgDePA59S8QloTrAl+8fVBnYRq5qw8Vl7/4sMgVvGQrOsAjhFvPv3iK05YqG5jivWc+cZ0BYRnwh5ZuEFXdVOVGA= X-MS-TrafficTypeDiagnostic: BY2PR12MB0145: X-Microsoft-Exchange-Diagnostics: 1; BY2PR12MB0145; 20:edLFTpMunWk7BdRFlE4//SxXLfes9wSztEbLY6iL23/xWmbj1iLIMrcGEHSWghPx2CEaJlp7qBPTTNPtMsO9SwgfHm6FMcwjDI+DtobziGzAIgFgbVpnA+fSFmONDfBLTeSoVRFUmesmsJZXVDnP0inZ1FktJiTOAKpe9o4OCWtGvwsFRtP/k6OUP1o5qeMnXkr0FlxNHY1skM8Mu/1a5cYViMR3yOJ/nV1iO/3Cl48cqmHzcvrbQcwYjEpCkGfuIQsr8Xtqk8fd+ShYusR+PlRaTcmkPWHRUsppyRwUgdg40reWPRV2ysYvf4VSpp5ke8b+5feh6b7DAtnIBsKUaOT8Aahtxz+B6QH4qO1W36lJDocABy6/5sHc/oFUOzKoPy1kvZrvmqYkAzpVhgz2idYhMBOL17lxqA7qSwG4JYAcRVkwXS/QV02OfGl+3bJkaPepSe6VygibJfSobZlLeyXA1BZMv9NBhvlmMcLK3D9fXPpq8BYjfVI65s6Pwvan; 4:mkL1EliIuwTZjGqH2bgDMHTD1u/q8QAYn7UrI+35az2B/hBqxluBZlI0HLgBC326KuRY07BBOK4oiuLNB4d4UQ4D+nCfMnR/I7ACUrX1dqZoqqKS68/iV2DXMZICqV67XyXGzGuUNE0sNggzy1TET8UeAxU06yZ/8Zs6Uqc8ZBj7T1X2P+Szd+XyNkUsY4SjkPqug71v6n+LwuUcrv1i2WSKvJw4Qi6Z7wQn5PpM0TwhQI3+u6BJiEFvi1ug3CaJ X-Exchange-Antispam-Report-Test: UriScan:; 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)(10201501046)(3002001)(100000703101)(100105400095)(93006095)(93001095)(6055026)(6041248)(20161123562025)(20161123558100)(20161123560025)(201703131423075)(201702281528075)(201703061421075)(201703061406153)(20161123564025)(20161123555025)(6072148)(201708071742011)(100000704101)(100105200095)(100000705101)(100105500095); SRVR:BY2PR12MB0145; BCL:0; PCL:0; RULEID:(100000800101)(100110000095)(100000801101)(100110300095)(100000802101)(100110100095)(100000803101)(100110400095)(100000804101)(100110200095)(100000805101)(100110500095); SRVR:BY2PR12MB0145; X-Forefront-PRVS: 0412A98A59 X-Forefront-Antispam-Report: SFV:NSPM; SFS:(10009020)(7370300001)(4630300001)(6009001)(39860400002)(51444003)(189002)(199003)(377454003)(24454002)(64126003)(50466002)(86362001)(65826007)(110136004)(54356999)(7350300001)(42186005)(53936002)(54906002)(6246003)(5660300001)(6666003)(6116002)(97736004)(189998001)(4001350100001)(76176999)(50986999)(3846002)(36756003)(2906002)(23676002)(31696002)(6306002)(230700001)(305945005)(106356001)(81166006)(68736007)(101416001)(6486002)(229853002)(7736002)(2950100002)(33646002)(81156014)(25786009)(53546010)(4326008)(66066001)(47776003)(65956001)(31686004)(83506001)(478600001)(8676002)(105586002)(65806001)(53416004)(93886005); DIR:OUT; SFP:1101; SCL:1; SRVR:BY2PR12MB0145; H:wsp094247wss.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?MTtCWTJQUjEyTUIwMTQ1OzIzOk9VWFBBOERXeEt4S21RNExRZ054OWJmc1pj?= =?utf-8?B?S3V5VUE3SDdUSDhlVlF4azJxTmR3bzVLL3VUeGY2bFBLVnBxWVo3RVd6Q1pX?= =?utf-8?B?d3EvRGp5QzlyZUdSckZTcjgyZTRHKzJWUnRiL1lTVUh1a21sd09Sdld6SjZx?= =?utf-8?B?M08xUmIwdnVPRnZMUWd0MmFtamRKRER4blowR29OOEhSWEQ2NFRSQ3NyVWMr?= =?utf-8?B?dGFzVFNHcllWZDhKd0hiTktHMlp0T1g4ZEJnbDZJUzgzU0dQZEk0OWx0SEd5?= =?utf-8?B?WTFDaUQvTGVhY09JYXNVdytLVUhYemFvUUhXSmZEM0o4a2VxNGVqNjdaNWdQ?= =?utf-8?B?QVg0VWlSYTUzVG9CT0NiZnFSbVl0dWo5QkNkYWNpMlppWWwvMHVQMXNDc214?= =?utf-8?B?YzdDMSttKzRUamkyOE11MFpnQndzUkRRa1Judmd6bU5GSE9Dem5KcmJrVElq?= =?utf-8?B?eFlnbUlXY3BTa20wbnVMQTlsT1drMFdPazI4R1NRK3UyU0xPQTh3NkpBS0Rz?= =?utf-8?B?WmZIbDNPZCtrdmdYMStURHA1Zm9wMkRQODhrN0Q3SWdjK3NGaUJ5aXVvOEdo?= =?utf-8?B?REJFTU83Wm5ZQ3UxTkJBenlWK00rUmpDMEgyVDU2Y3hXTVRPcEdHSnZBTEsx?= =?utf-8?B?UzZLdXJBTVRBemNWL2JPTGZOMWdsNEN6eDMrREYxNXlEenhuaFVManpMd0Rw?= =?utf-8?B?R2xTMmk1OE5ndUNXY3o2U3ZxRjlMbU1wb3FsUUVHWkpEbmNqOTJUV3V4bDFP?= =?utf-8?B?Y21PeEcycjN5eVhzT0Rwdk14MlNWcmI5Nk8vc2RiVDNGQXdXcjhqUlJUWW16?= =?utf-8?B?TWJia3ZIMkVaeVFoVWxOVmNhWk1MbThJbzg3WFIrWndBRklCWVpBQUFtZTBi?= =?utf-8?B?a3B5NjNtcEFHTXV3VE1GYm96UENjd3NPOGk3RW93bnZDL3dBdTVIK2NnYXBm?= =?utf-8?B?UDR6UXp5QUYrNUM1Mzk3SkhHOGt5QnJTUVZqY2FTdFQyTnY0UkJDdXZ0SFYx?= =?utf-8?B?NG5yNXdyd1M3djV4QlV1cVpxSEMrbFRSc1M3dlFRclJBK1ZEdUlUaDdPYjBs?= =?utf-8?B?ZElXTHRFUmxzYTlTaWpjVXFwRDJpVnZRb1JEMW5MZkFENzB0L0pRbCs3bGJy?= =?utf-8?B?TmJEVlFFOFljQWN1YmJLNE4vZ3BJUFd5eWlQc0lkN0tlTjIySGxpbzVTZVJ2?= =?utf-8?B?dFFmUGQ1ZDR1RnNPbWhiZitMZENETjR0Rjl5OG9UN1FQTm1lRjBlTHliVzRp?= =?utf-8?B?cG5TcEw3UnRwWEhhdnUxRjlEcWRmRmtnUkdVdE5abzFzRklDdUZ4M0wzc0pJ?= =?utf-8?B?UDV0c3lQMll2TklWaHc0REpCRkEwVHJXQjVURVQ0cXJxQnpXODkyWG9LQnNq?= =?utf-8?B?YTNTSTIycmwxOVdHaGNCZXhKTUF5T1VDRFg3QnVGTk9iZDQweEJtdUlMWkNC?= =?utf-8?B?S1BuSEZ2RjArRzAwL0tBYTFsRk9KWnN0QnlVRERyZklFOUxJcUFNV2lFVkUv?= =?utf-8?B?RUdIZDNYdkpMMldFYmpJUzRkOUtPbnhLcEp6V3hZRTQ2SUt0eG91SUNIU0pq?= =?utf-8?B?MGl6V1ZMQ0duTEtZeFR2bEZqbENHTllLNEU0L0ZlNXN4S0VlRGFHcHZlZGdK?= =?utf-8?B?V3lNbUttTmZVRWhCQlh0cFUvSEFmZmQyVVgzaEhPaHRHaWUyc3Z6RFJqK3FB?= =?utf-8?B?N1BwMFhBaHF5RUpjanQwVFhiWmx5R1R3QlVDQUZtUStNVkRCV3VDa29EQloz?= =?utf-8?B?NE0wcnRodlQ2VlJySGFWZmQ4clhVcUhTWUlzSm01dlVVY0dYYWQvaE9GL2E4?= =?utf-8?B?TVhpZUJqU1dTa0o2cjNXSjFCRlIxU2p4bXVMOENZSCtaQ1A5WUNvTm1Bcjgv?= =?utf-8?B?dkxXUXpQRS9TNFRLN2s4b2RIdlFtcDloeW9IVTRiNXg5dFBkc3JmcmNiQVlP?= =?utf-8?Q?SoN0UBYp3A+i6bbT7xZAfMTxwOSyMs=3D?= X-Microsoft-Exchange-Diagnostics: 1; BY2PR12MB0145; 6:jOLNfUvBTr8VsVsBj5qehPpcdSz3KIU5ZmvTP8mHBliZVQgeW5soe3AZ8i6xFPNgeU9pI6wNeIFUpk2icOgB8VwnAKbmqiF8lzy20xnmKO8L8v87rOMaewktpa++Q2Aam6GtifckmTxA2eemY27VDLbqe2Cm2DAaYoriIDbkEDwE3l8VZEPPQCIYLqswDzCSsAcrrvWhlwiypJG7xFrNDPSJlC7bIvyXf91Fd4ydpTr8jj5rbhUtVTw4URWR4LwYRo0GYV0OXfcq026IOZGpk5imttpZQn2PMKvaF3dVGc4hYHyDE/1ZhWgW7ZNPkpeutrKQzMs0KoRpAXFGgJoLwQ==; 5:BhqT6FaCAN6aalOyq+Z5MtQBtyzM/pcbn8LTvKDtmcttU/X8D+3qod9bWcBYHoO5Obt4VTNGdsCoZw2PniGv3Nh0ipgt3xtBTYi+cKv4Yg3Odtf9W8++VBQpSTeG3Qlg6TXvc0f3+hN2DVDoG5npwA==; 24:4T2pa7oAxH/PUNxT8gBI31XukKx08XyybkWGBrJlsIhA9dCy01vPXlpFpvewl4wjD5AU/0vpMpCOR2Jeo6w7Og/EaCvRR753BURrRU0Vf60=; 7:bGqGMFEEX0tljmGhy1KGlRteu9bwmdOmxwSO4f+40K2RJuq8yHn5OBYBHLMSxReY+Wf+SXbW7275BBytp9FZkpTXvmW/BhBB0cgJ8am0+FMcb0Tr7pkMF5ltidZnm06scin8UIP8YXMziiFU++3Twp23rotgL+eWK/1YbiYqgxMeGSK/AF9OSV7xQVH2Shlbw3n3ReYQneJxpFLq2bkpIh7DoPg+240mB+jeWDofsTo= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1; BY2PR12MB0145; 20:rkGxde2S2zTVjuYhes8t34tBRy/OswsQAm0X4gblh77vlPD9EpWDMXD/p/fsi0eZTQC6F5ELggibiFo/i0mPuOiD3vQLvqZuAI1R9G8CxoMeDvGMR1dvVgeCMd5tzaXgjhTU53Svo4EqarZBqow0XoBYrEReTKzzOKrHYzzdC3mQ2PyuHSto4Zke5RumAvuDjEmk2dNbFtgJ6BYP90oMLVMYu3KpF8qibhJCst3qn3aCils1fYoIMsZ/3wKFgYaG X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Aug 2017 23:18:23.1202 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY2PR12MB0145 Subject: Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk 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: Sun, 27 Aug 2017 23:15:48 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Content-Language: en-US On 8/27/17 5:05 PM, Laszlo Ersek wrote: [...] > (4) I think we should be careful with BusMasterCommonBuffer operations > similarly to BusMasterWrite operations -- populate first, map second. > > This is to avoid exposing any stale data, even for a short window, to > the device. > > Speaking in SEV terms, IoMmuMap() will decrypt NumberOfBytes in-place -- > which is by-design, but then it should decrypt what we just put there, > not whatever used to be there (until we overwrite it). > > IOW, please move the mapping just under the *HostStatus assignment. > > ... I've now checked all the VirtioOperationBusMasterCommonBuffer > mappings in the tree, and the rest is fine -- in fact there is only one > (outside of this patch) at the moment: in VirtioRingMap(). And that is > fine, because VirtioRingInit() zeroes out the entire ring, after > allocating it. > > Probably a good idea to attend to this in the other drivers (wherever > they use BusMasterCommonBuffer). Will do. [...] >> >>> + if (EFI_ERROR (Ret)) { >>> + Status = EFI_DEVICE_ERROR; >>> + } >>> + >>> + if (!EFI_ERROR (Status) && >>> + *HostStatus == VIRTIO_BLK_S_OK) { >>> + Status = EFI_SUCCESS; >>> + } else { >>> + Status = EFI_DEVICE_ERROR; >>> } >> should be written like this: >> >> if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) || >> *HostStatus != VIRTIO_BLK_S_OK) { >> Status = EFI_DEVICE_ERROR; >> } >> >> Namely, >> >> - this block will ensure that we only look at *HostStatus when we're >> allowed to (i.e., after both VirtioFlush() and Unmap succeeded), Actually since the HostStatus is mapped with BusMasterCommonBuffer hence we can safely move the unmapping of HostStatus buffer later in the code. In summary, I can still retain the original if statement and do the unmapping outside the check. In v2 I will try to move the code around. thanks >> >> - If VirtioFlush() fails and sets Status to some error code, then we >> coerce Status to EFI_DEVICE_ERROR, >> >> - If the entire condition evaluates to FALSE, then Status is already >> EFI_SUCCESS, so no need to touch it. >> >> >> Regarding the VirtioScsiDxe driver... in this patch, we get away with >> the above shortcut (without making a mess of the code), but in the >> VirtioScsiDxe driver, we won't. In that driver, the bus master device >> can produce *two* output buffers, so you will have to unmap at least one >> of those under an error-handling label -- when you mapped the first >> successfully, and failed to map the second. >> >> (In this patch you managed to unmap StatusMapping in shared code, but >> that only worked because you had only one output buffer, and you could >> put its mapping last.) >> >> And once you unmap an output buffer on both the success path and the >> failure path, things get more complex. I think that's OK, we should just >> be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I >> laid out in >> .