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

[Brijesh Singh <brijesh.singh@amd.com>]

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.


>>
>>>>>    *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?)
>>
>>
>>