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 1F6C121E25720 for ; Fri, 28 Jul 2017 09:14:35 -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 A7D0969EF8; Fri, 28 Jul 2017 16:16:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A7D0969EF8 Authentication-Results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx04.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 49FBD5C8A6; Fri, 28 Jul 2017 16:16:32 +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: <22fccf66-4eed-6e03-0384-d8265a6c6dfa@redhat.com> Date: Fri, 28 Jul 2017 18:16:32 +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.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Fri, 28 Jul 2017 16:16:39 +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 16:14:35 -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: >>> I agree with you, there is a bug in AmdSevIoMmu.c. I will fix it. And >>> introduce list to track if the buffer was allocated by us. If buffer >>> was allocated by us then we can safely say that it does not require a >>> bounce buffer. While implementing the initial code I was not able to >>> see BusMasterCommonBuffer usage. But with virtio things are getting a >>> bit more clear in my mind. >> >> The purpose of the internal list is a little bit different -- it is a >> "free list", not a tracker list. >> >> Under the proposed scheme, a MAP_INFO structure will have to be >> allocated for *all* mappings: both for CommonBuffer (where the actual >> pages come from the caller, who retrieved them earlier with an >> AllocateBuffer() call), and for Read/Write (where the actual pages will >> be allocated internally to Map). Allocating the MAP_INFO structure in >> Map() is necessary in *both* cases because the Unmap() function can >> *only* consult this MAP_INFO structure to learn the address and the size >> at which it has to re-set the memory encryption mask. This is because >> the Unmap() function gets no other input parameter. >> >> Then, regardless of the original operation (CommonBuffer vs. >> Read/Write), the MAP_INFO structure has to be recycled in Unmap(). (For >> CommonBuffer, the actual pages are owned by the caller, so we don't free >> them here; for Read/Write, the actual pages are owned by Map/Unmap, so >> we free them in Unmap() *in addition* to recycling MAP_INFO.) The main >> point is that MAP_INFO cannot be released with FreePool() because that >> could change the UEFI memmap during ExitBootServices() processing. So >> MAP_INFO has to be "released" to an internal *free* list. >> >> And since we have an internal free list, the original MAP_INFO >> allocation in Map() should consult the free list first, and only if it >> is empty should it fall back to AllocatePool(). >> >> Whether the actual pages tracked by MAP_INFO were allocated internally >> by Map(), or externally by the caller (via AllocateBuffer()) is an >> independent question. (And it can be seen from the MAP_INFO.Operation >> field.) >> >> Does this make it clearer? >> > > It makes sense. thanks > > One question, do we need to return EFI_NOT_SUPPORTED error when we get > request to map a buffer with CommonBuffer but the buffer was not > allocated using "EFI_PCI_IO_PROTOCOL->AllocateBuffer (...)"? > > At least as per spec, it seems if caller wants to map a buffer with > CommonBuffer then it must have called > "EFI_PCI_IO_PROTOCOL->AllocateBuffer (...)" Yes, that is it, exactly: if the caller violates a requirement in the UEFI spec, covering the use of the APIs, then the firmware *may* make an attempt to detect that (and to reject it), but the firmware is not *required* to do so. A much simpler example: on a double-free programming error, the second gBS->FreePool() call is not *required* to detect the problem. ("The Buffer that is freed must have been allocated by AllocatePool().") So, I don't think we need to implement measures for checking that a CommonBuffer Map() actually refers to a previously returned, active, AllocateBuffer() address. (Snipping the rest, I've read it all, thanks for the answers. Nothing to add this time :) ) Cheers! Laszlo