On 07/28/17 10:39, Ard Biesheuvel wrote: > On 27 July 2017 at 23:10, Brijesh Singh <[email protected]> wrote: >> >> >> On 07/27/2017 04:31 PM, Ard Biesheuvel wrote: >>> >>> >>>> On 27 Jul 2017, at 21:55, Brijesh Singh <[email protected]> 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".
Brijesh, thanks for the clarification. Previously I replied (at length) to your paragraph that said "trying to wrap my head around...", and it wasn't clear what you meant by "allocated by us". In my previous response, I assumed that you meant a distinction between "allocated in Map()" vs. "allocated in AllocateBuffer()". I stand by my earlier answer for that (assumed) distinction, but now I see that you meant something else. >> >> 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 > You (Brijesh and Ard) are both right. This question depends on the outermost interface that the specific virtio driver provides. In this case, VirtioBlkReadBlocks() implements EFI_BLOCK_IO_PROTOCOL.ReadBlocks(). The buffer is owned by an independent agent, and it is guaranteed by the ReadBlocks() interface that the transfer is unidirectional. So a standalone Map() call with BusMasterWrite is appropriate, followed by a standalone Unmap() call *before* VirtioBlkReadBlocks() returns. In this sense, my earlier request to "*always* use AllocateBuffer + Map" was too strict. However, in the *general* case, the recommendation remains the same. For the virtio-net driver for example, the interfaces are not synchronous. E.g., while EFI_BLOCK_IO_PROTOCOL.WriteBlocks() is synchronous, EFI_SIMPLE_NETWORK_PROTOCOL.Transmit() is not. So, although in VirtioNetTransmit() we might be tempted to use BusMasterRead, that's not right, because ExitBootServices() could be called before the SNP client collects the buffer with VirtioNetGetStatus(). The ExitBootServices() callback will have to Unmap() the area without freeing it, and that's only possible if BusMasterCommonBuffer is used in VirtioNetTransmit() to begin with. This means that we'll have to save the client's data -- after updating it according to HeaderSize -- with AllocateBuffer() in VirtioNetTransmit(), Map() *that* as BusMasterCommonBuffer, and undo both steps in VirtioNetGetStatus(). And, in the exit-boot-services callback, it has to be Unmap()ped only, but not freed. Basically it depends upon whether you can complete the entire operation synchronously, before the outermost protocol interface returns. I recommend that we work on the IoMmuDxe and QemuFwCfgLib updates first. (And, my apologies again for not catching these issues immediately; as I said, this is my first time doing non-1:1 DMA.) Thanks, Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
