On 07/27/2017 04:31 PM, Ard Biesheuvel wrote:
On 27 Jul 2017, at 21:55, Brijesh Singh <brijesh.si...@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?)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
