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

[Laszlo Ersek <lersek@redhat.com>]

On 07/28/17 10:39, Ard Biesheuvel wrote:
> 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".

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