From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io0-x234.google.com (mail-io0-x234.google.com [IPv6:2607:f8b0:4001:c06::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 603F521D4918F for ; Fri, 28 Jul 2017 01:37:06 -0700 (PDT) Received: by mail-io0-x234.google.com with SMTP id g13so87490520ioj.5 for ; Fri, 28 Jul 2017 01:39:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=sIKLXyNkmwrexpVxFCtZiSds/5WHUcTWO6jkE0FnvZI=; b=gI0PB9iJfc74KMCd7K9hXesImP1Yl7jUS6NmEYIkmT6WvM5XsxWBjW0ppbX4ImAMAn GlGzCaucG9KMCf128Hx7HxMNIaPvRfT0gYXOfxZULvtNd0La9EUjqRSGIuMd3NZgRrUT WrvpnDYqaKn6yAEbFqfDQ6MuWzZANhDI+NAFs= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=sIKLXyNkmwrexpVxFCtZiSds/5WHUcTWO6jkE0FnvZI=; b=HHnrtf5/o8oObkrYDT3o5V/Y/qV7McZrAkv+/6xoH5kr+wI+fnYqdNa55Hda+JBaJD uHWTM1dfWktET1z2RCteaVA6OA3NFT5QPPizCHFk0EQcmvkIcAaaoeWcoDdkK28mfU/M CzpmnlVSjpJymCA/ITxStTDqW7M6W8KYxRbErtHN1QnLtu6MAzZPGy/WHeNQecpct8uR mo6jRE5lpSXritGsV2iHHNLCy8W3Sl6onC5g2xTXwyyioehswwR7VEMAxzhRNGU0l0wh rI+Lea7SQhrewowwH3bScav2Sb83pNmgwAe11Nk7PV4q3sP56rK3p/iApffDXVrg1N5l Olow== X-Gm-Message-State: AIVw110XZIpn7684F2LSj0h7M20BeDR+eZs7KZnUQkALwltY+O5u8QxD z7NnHFhQvr6yllxhqNo6Vw+AQgrb7vcd X-Received: by 10.107.43.131 with SMTP id r125mr8328560ior.76.1501231150524; Fri, 28 Jul 2017 01:39:10 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.162.1 with HTTP; Fri, 28 Jul 2017 01:39:09 -0700 (PDT) In-Reply-To: <2bbe4119-ed19-d594-741b-cb92cd4f93f2@amd.com> 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> <6517a7f8-5564-35e1-dc27-1b85a23c815e@amd.com> <9C4ABF62-4018-4014-A3C4-0A8B3B3CE1C2@linaro.org> <2bbe4119-ed19-d594-741b-cb92cd4f93f2@amd.com> From: Ard Biesheuvel Date: Fri, 28 Jul 2017 09:39:09 +0100 Message-ID: To: Brijesh Singh Cc: Laszlo Ersek , "edk2-devel@lists.01.org" , Tom Lendacky , Jordan Justen , Jason Wang , "Michael S . Tsirkin" , Gerd Hoffmann 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 08:37:06 -0000 Content-Type: text/plain; charset="UTF-8" On 27 July 2017 at 23:10, Brijesh Singh wrote: > > > On 07/27/2017 04:31 PM, Ard Biesheuvel wrote: >> >> >>> On 27 Jul 2017, at 21:55, Brijesh Singh wrote: >>> >>> >>> >>> On 07/27/2017 02:00 PM, Brijesh Singh wrote: >>> >>>>>> This distribution of operations seems wrong. The key point is that >>>>>> AllocateBuffer() *need not* result in a buffer that is immediately >>>>>> usable, and that client code is required to call Map() >>>>>> *unconditionally*, even if BusMasterCommonBuffer is the desired >>>>>> operation. Therefore, the right distribution of operations is: >>>>>> >>>>>> - IoMmuAllocateBuffer() allocates pages and does not touch the >>>>>> encryption mask.. >>>>>> >>>>>> - IoMmuFreeBuffer() deallocates pages and does not touch the >>>>>> encryption >>>>>> mask. >>>>>> >>>> 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. >>>> [1] >>>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c#L159 >>>> [2] >>>> https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgDxe.c#L197 >>>>>> >>>>>> - IoMmuMap() does not allocate pages when BusMasterCommonBuffer is >>>>>> requested, and it allocates pages (bounce buffer) otherwise. >>>>>> >>>> I am trying to wrap my head around how we can support >>>> BusMasterCommonBuffer >>>> when buffer was not allocated by us. Changing the memory encryption mask >>>> in >>>> a page table will not update the contents. Also since the memory >>>> encryption >>>> mask works on PAGE_SIZE hence changing the encryption mask on not our >>>> allocated >>>> buffer could mess things up (e.g if NumberOfBytes is not PAGE_SIZE >>>> aligned). >>> >>> >>> I may be missing something in my understanding. Here is a flow I have in >>> my >>> mind, please correct me. >>> >>> OvmfPkg/VirtIoBlk.c: >>> >>> VirtioBlkInit() >>> .... >>> .... >>> VirtioRingInit >>> Virtio->AllocateSharedPages(RingSize, &Ring->Base) >>> PciIo->AllocatePages(RingSize, &RingAddress) >>> Virtio->MapSharedPages(...,BusMasterCommonBuffer, Ring->Base, >>> RingSize, &RingDeviceAddress) >>> ..... >>> ..... >>> >>> This case is straight forward and we can easily maps. No need for bounce >>> buffering. >>> >>> VirtioBlkReadBlocks(..., BufferSize, Buffer,) >>> ...... >>> ...... >>> SynchronousRequest(..., BufferSize, Buffer) >>> .... >>> Virtio->MapSharedPages(..., BusMasterCommonBuffer, Buffer, >>> BufferSize, &DeviceAddress) >>> VirtioAppendDesc(DeviceAddress, BufferSize, ...) >>> VirtioFlush (...) >>> In the above case, "Buffer" was not allocated by us hence we will not >>> able to change the >>> memory encryption attributes. Am I missing something in the flow ? >>> >> >> >> Common buffer mappings may only be created from buffers that were >> allocated by AllocateBuffer(). In fact, that is its main purpose > > > Yes, that part is well understood. If the buffer was allocated by us (e.g > vring, request/status > structure etc) then those should be mapped as "BusMasterCommonBuffer". > > But I am trying to figure out, how to map a data buffers before issuing a > virtio request. e.g when > VirtioBlkReadBlocks() is called, "Buffer" pointer is not a DMA address hence > we need to map it. > I think it should be mapped using "BusMasterWrite" not > "BusMasterCommonBuffer" before adding into vring. > If the transfer is strictly unidirectional, then that should work. If the transfer goes both ways, you may need to map/unmap for read and then map/unmap for write