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 05AD721D2DCE8 for ; Tue, 1 Aug 2017 13:50:19 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 228C419F7E4; Tue, 1 Aug 2017 20:52:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 228C419F7E4 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=lersek@redhat.com Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-153.phx2.redhat.com [10.3.116.153]) by smtp.corp.redhat.com (Postfix) with ESMTP id BA7EE5C550; Tue, 1 Aug 2017 20:52:26 +0000 (UTC) To: Brijesh Singh , Ard Biesheuvel Cc: Tom Lendacky , "edk2-devel@lists.01.org" , Jordan Justen References: <1501529474-20550-1-git-send-email-brijesh.singh@amd.com> <1501529474-20550-3-git-send-email-brijesh.singh@amd.com> <5f2b17f8-9872-c1ff-715d-96e3158e6df1@amd.com> From: Laszlo Ersek Message-ID: <2456b6bd-5788-68ea-f688-6eb13494a9e5@redhat.com> Date: Tue, 1 Aug 2017 22:52:25 +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: <5f2b17f8-9872-c1ff-715d-96e3158e6df1@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 01 Aug 2017 20:52:28 +0000 (UTC) Subject: Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation 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: Tue, 01 Aug 2017 20:50:19 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 07/31/17 22:27, Brijesh Singh wrote: > > > On 07/31/2017 02:49 PM, Ard Biesheuvel wrote: >> On 31 July 2017 at 20:31, Brijesh Singh wrote: >>> The current implementation was making assumption that AllocateBuffer() >>> returns a buffer with C-bit cleared. Hence when we were asked to >>> Map() with BusMasterCommonBuffer, we do not change the C-bit on >>> host buffer. >>> >>> In previous patch, we changed the AllocateBuffer() to not clear >>> C-bit during allocation. The patch adds support for handling the >>> BusMasterCommonBuffer operations when SEV is active. >>> >>> A typical DMA Bus master Common Operation follows the below step: >>> >>> 1. Client calls AllocateBuffer() to allocate a common buffer >>> 2. Client fill some data in common buffer (optional) >>> 3. Client calls Map() with BusMasterCommonBuffer >>> 4. Programs the DMA bus master with the device address returned by Map() >>> 5. The common buffer can now be accessed equally by the processor and >>> the DMA bus master. >>> 6. Client calls Unmap() >>> 7. Client calls FreeBuffer() >>> >>> In order to handle steps #2 (in which common buffer may contain >>> data), we perform in-place encryption to ensure that device >>> address returned by the Map() contains the correct data after >>> we clear the C-bit during Map(). >>> >>> In my measurement I do not see any noticable perform degradation when >>> performing in-place encryption/decryption on common buffer. >>> >>> Suggested-by: Laszlo Ersek >>> Contributed-under: TianoCore Contribution Agreement 1.0 >>> Cc: Laszlo Ersek >>> Cc: Jordan Justen >>> Signed-off-by: Brijesh Singh >>> --- >>> OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++--- >>> 1 file changed, 164 insertions(+), 26 deletions(-) >>> >> >> Hello Brijesh, >> >> I haven't looked in detail at the existing code, but please don't >> conflate the device address with the address of a bounce buffer. These >> are very different things, although the confusion is understandable >> (and precedented) when not used to dealing with non-1:1 DMA. >> >> The device address is what gets programmed into the device's DMA >> registers. If there is a fixed [non-zero] offset between the device's >> view of memory and the host's (as may be the case with PCI, or >> generally when using an IOMMU), then the device is the only one who >> should attempt to perform memory accesses using this address. So >> please void SetMem() or other CPU dereferences involving the device >> address, and treat it as an opaque handle instead. >> > >> In your case, you are dealing with a bounce buffer. So call it bounce >> buffer in the MapInfo struct. Imagine when dealing with a non-linear >> host to PCI mapping, you will still need to perform an additional >> translation to derive the device address from the bounce buffer >> address. >> > > Agreed. > > Initially, AmdSevIoMmu.c code was derived from PciRootBridgeIo and MAP_INFO > structure was literally copied. I will probably send a separate patch > to fix the structure member and update the comments to reflect its true > meaning. Yes, we should call it "PlainTextAddress" or something similar. Thanks Laszlo