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 ?
*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
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
