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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 27 July 2017 at 23:10, Brijesh Singh <brijesh.singh@amd.com> wrote:
>
>
> On 07/27/2017 04:31 PM, Ard Biesheuvel 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
>
>
> 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