Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support

[Andrew Fish <afish@apple.com>]

> On Jul 27, 2017, at 2:31 PM, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
>> 
>> On 27 Jul 2017, at 21:55, Brijesh Singh <brijesh.singh@amd.com> 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

Brijesh,

If you look in the UEFI Spec 13.4 EFI PCI I/O Protocol there is a good write on DMA. 

DMA Bus Master Read Operation
=========================
Call Map() for EfiPciIoOperationBusMasterRead.
Program the DMA Bus Master with the DeviceAddress returned by Map(). Start the DMA Bus Master.
Wait for DMA Bus Master to complete the read operation.
Call Unmap().


DMA Bus Master Write Operation
==========================
Call Map() for EfiPciOperationBusMasterWrite.
Program the DMA Bus Master with the DeviceAddress returned by Map().
Start the DMA Bus Master.
Wait for DMA Bus Master to complete the write operation.
Perform a PCI controller specific read transaction to flush all PCI write buffers (See PCI Specification Section 3.2.5.2) .
Call Flush().
Call Unmap().

DMA Bus Master Common Buffer Operation
==================================
Call AllocateBuffer() to allocate a common buffer.
Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
Program the DMA Bus Master with the DeviceAddress returned by Map().
The common buffer can now be accessed equally by the processor and the DMA bus master. Call Unmap().
Call FreeBuffer().

Thanks,

Andrew Fish


>> 
>>>>>  *Regardless* of BusMaster operation, the following actions are carried
>>>>>  out unconditionally:
>>>>> 
>>>>>  . the memory encryption mask is cleared in this function (and in this
>>>>>    function only),
>>>>> 
>>>>>  . An attempt is made to grab a MAP_INFO structure from an internal
>>>>>    free list (to be introduced!). The head of the list is a new static
>>>>>    variable. If the free list is empty, then a MAP_INFO structure is
>>>>>    allocated with AllocatePool(). The NO_MAPPING macro becomes unused
>>>>>    and can be deleted from the source code.
>>>>> 
>>>>> - IoMmuUnmap() clears the encryption mask unconditionally. (For this, it
>>>>>  has to consult the MAP_INFO structure that is being passed in from the
>>>>>  caller.) In addition:
>>>>> 
>>>>>  . If MapInfo->Operation is BusMasterCommonBuffer, then we know the
>>>>>    allocation was done separately in AllocateBuffer, so we do not
>>>>>    release the pages. Otherwise, we do release the pages.
>>>>> 
>>>>>  . MapInfo is linked back on the internal free list (see above). It is
>>>>>    *never* released with FreePool().
>>>>> 
>>>>>  This approach guarantees that IoMmuUnmap() can de-program the IOMMU (=
>>>>>  re-set the memory encryption mask) without changing the UEFI memory
>>>>>  map. (I trust that MemEncryptSevSetPageEncMask() will not split page
>>>>>  tables internally when it *re*sets the encryption mask -- is that
>>>>>  correct?)
>> 
>> 
>> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel