From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D42A921E11D75 for ; Fri, 28 Jul 2017 12:19:14 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 67882C08EA53; Fri, 28 Jul 2017 19:21:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 67882C08EA53 Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx07.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-27.phx2.redhat.com [10.3.116.27]) by smtp.corp.redhat.com (Postfix) with ESMTP id BA1BA7093A; Fri, 28 Jul 2017 19:21:13 +0000 (UTC) To: Brijesh Singh , Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Tom Lendacky , Jordan Justen , Jason Wang , "Michael S . Tsirkin" , Gerd Hoffmann References: <1500502151-13508-1-git-send-email-brijesh.singh@amd.com> <841bec5f-6f6e-8b1f-25ba-0fd37a915b72@redhat.com> <4e2fc623-3656-eea7-09a8-b5c6d2f694e1@amd.com> <4071596d-32c9-e6d9-8c93-0d43d28e9b5a@redhat.com> <217545ac-962d-089f-9c9a-d2bbfca6427e@amd.com> From: Laszlo Ersek Message-ID: <2b9361cc-03f9-a403-98aa-c1cf5bfd17c0@redhat.com> Date: Fri, 28 Jul 2017 21:21:12 +0200 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: <217545ac-962d-089f-9c9a-d2bbfca6427e@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 28 Jul 2017 19:21:19 +0000 (UTC) Subject: Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support 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: Fri, 28 Jul 2017 19:19:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/28/17 18:00, Brijesh Singh wrote: > > > On 07/28/2017 08:38 AM, Laszlo Ersek wrote: >> On 07/27/17 21:00, Brijesh Singh wrote: >>> Actually one of main reason why we cleared and restored the memory >>> encryption mask during allocate/free is because we also consume the >>> IOMMU protocol in QemuFwCfgLib as a method to allocate and free a >>> DMA buffer. I am certainly open to suggestions. >> >> Argh. That's again my fault then, I should have noticed it. :( I >> apologize, the Map/Unmap/AllocateBuffer/FreeBuffer APIs are a "first" >> for me as well. >> >> As discussed earlier (and confirmed by Ard), calling *just* >> AllocateBuffer() is never enough, Map()/Unmap() are always necessary. >> >> So here's what I suggest (to keep the changes localized): >> >> - Extend the prototype of InternalQemuFwCfgSevDmaAllocateBuffer() >> function to output a "VOID *Mapping" parameter as well, in addition >> to the address. >> >> - Modify the prototype of the InternalQemuFwCfgSevDmaFreeBuffer() >> function to take a "VOID *Mapping" parameter in addition to the >> buffer address. >> >> - In the DXE implementation of >> InternalQemuFwCfgSevDmaAllocateBuffer(), keep the current IOMMU >> AllocateBuffer() call, but also call IOMMU Map(), with >> CommonBuffer. Furthermore, propagate the Mapping output of Map() >> outwards. (Note that Map() may have to be called in a loop, >> dependent on "NumberOfBytes".) >> >> - In the DXE implementation of InternalQemuFwCfgSevDmaFreeBuffer(), >> call the IOMMU Unmap() function first (using the new Mapping >> parameter). >> > > I will update the code and send patch for review. thanks Here's an alternative, given that you mentioned being performance-conscious. I'm not "suggesting" this as preferred, just offering it for your consideration. Pick whichever you like more. In this approach, I'm going to go back on my original request, namely to keep the InternalQemuFwCfgDmaBytes() implementation centralized, in the "QemuFwCfgLib.c" file. I now realize that the differences are large enough that this may not have been a good idea. So here goes: * Internal APIs ("QemuFwCfgLibInternal.h"): - Remove the InternalQemuFwCfgSev*() function prototypes. - Introduce the InternalQemuFwCfgDmaBytes() function prototype. * SEC instance ("QemuFwCfgSec.c"): - Remove the InternalQemuFwCfgSev*() function definitions. - Add the InternalQemuFwCfgDmaBytes() function definition with ASSERT(FALSE) + CpuDeadLoop(). * PEI instance ("QemuFwCfgPei.c): - Remove the InternalQemuFwCfgSev*() function definitions. - Copy the InternalQemuFwCfgDmaBytes() function definition from the currently shared code ("QemuFwCfgLib.c"), and remove all branches that are related to the SEV enabled case. IOW, specialize the implementation to the plaintext case. - In QemuFwCfgInitialize(), replace the InternalQemuFwCfgSevIsEnabled() function call with a direct call to MemEncryptSevIsEnabled(). * DXE instance ("QemuFwCfgDxe.c"): - Remove the InternalQemuFwCfgSev*() function definitions. - Copy the InternalQemuFwCfgDmaBytes() function definition from the currently shared code ("QemuFwCfgLib.c"), as a starting point. - Replace the InternalQemuFwCfgSevIsEnabled() call with a direct call to MemEncryptSevIsEnabled(). - When SEV is enabled, split the control area ("FW_CFG_DMA_ACCESS") to a separate buffer. This control area should be allocated with IOMMU AllocateBuffer(), and Map()ped separately, as BusMasterCommonBuffer64. - For the data transfer buffer, use *only* Map() and Unmap(), without AllocateBuffer() and FreeBuffer(). Use BusMasterRead64 and BusMasterWrite64, dependent on FW_CFG_DMA_CTL_WRITE / FW_CFG_DMA_CTL_READ. The point is that the potentially large data area will be bounced only once, because Map()/Unmap() will own the bounce buffer, and the in-place (de)crypting can be avoided. (The fw_cfg DMA transfer is completed in one synchronous operation, as witnessed by the library client.) The explicit CopyMem() calls can be removed. - Remove the InternalQemuFwCfgSevDmaAllocateBuffer() and InternalQemuFwCfgSevDmaFreeBuffer() calls. * shared code ("QemuFwCfgLib.c"): - remove the InternalQemuFwCfgDmaBytes() function definition. Thanks, Laszlo