On 07/27/2017 12:56 PM, Ard Biesheuvel wrote:
On 27 July 2017 at 18:16, Laszlo Ersek <[email protected]> wrote:
On 07/27/17 16:21, Brijesh Singh wrote:
Hi Laszlo,
(5.3) All virtio driver code should treat VIRTIO_F_IOMMU_PLATFORM
simply in parallel with VIRTIO_F_VERSION_1, and don't act upon
VIRTIO_F_IOMMU_PLATFORM in any special shape or form. So basically
this is just my point (3) from above.
(5.4) There are three VIRTIO_DEVICE_PROTOCOL implementations in edk2:
- "OvmfPkg/VirtioPciDeviceDxe" binds legacy-only and transitional
virtio-pci devices, and offers virtio 0.9.5 semantics.
- "ArmVirtPkg/VirtioFdtDxe" (via
"OvmfPkg/Library/VirtioMmioDeviceLib") binds virtio-mmio devices,
and offers virtio 0.9.5 semantics.
- "OvmfPkg/Virtio10Dxe" binds modern-only virtio-pci devices, and
offers virtio 1.0.0 semantics.
The first two drivers should implement the AllocateSharedPages() and
FreeSharedPages() member functions simply with the corresponding
MemoryAllocationLib functions (using BootServicesData type memory),
and implement the MapSharedPages() and UnmapSharedPages() member
functions as no-ops (return the input addresses transparently).
The third driver should implement all four new member functions by
respectively delegating the job to:
- EFI_PCI_IO_PROTOCOL.AllocateBuffer() -- with BootServicesData --
- EFI_PCI_IO_PROTOCOL.FreeBuffer()
- EFI_PCI_IO_PROTOCOL.Map() -- with BusMasterCommonBuffer64 --
- EFI_PCI_IO_PROTOCOL.Unmap()
I have working to implement patch per your recommendation. I assume
you mean map the buffers with EfiPciIoOperationBusMasterCommonBuffer
[1].
Yes.
If so, I see one issue with SEV guest. When SEV is active, IOMMU
driver uses a bounce buffer to map host address to a device address.
While creating bounce buffer we can map it either for
EfiPciIoOperationBusMasterRead or EfiPciIoOperationBusMasterWrite
Operation. If caller wants to map
EfiPciIoOperationBusMasterCommonBuffer then it must allocate the
buffer using EFI_PCI_IO_PROTOCOL.AllocateBuffer() [2] otherwise we
will fail to map.
Correct.
I see that PciRootBridgeIo.c has similar check when using a bounce
buffer for < 4GB use cases [3].
Correct.
Do you see any issue if we use EfiPciIoOperationBusMasterRead or
EfiPciIoOperationBusMasterWrite instead of
EfiPciIoOperationBusMasterCommonBuffer ?
Yes, I do. The BusMasterRead and BusMasterWrite operations are suitable
for one-shot, uni-directional transfers, where the bounce buffer
contents and the client code contents are exchanged on Map/Unmap.
However virtio transfers, generally speaking, are not like this. While
the individual memory areas pointed-to by separate virtio descriptors
can me associated with one specific transfer direction (see the
VRING_DESC_F_WRITE flag), the virtio ring area itself is long-lived, and
it is simultaneously read and written by both host and guest,
asynchronously and repeatedly. This calls for BusMasterCommonBuffer.
Once we implement BusMasterCommonBuffer, we can use it for everything
else.
Another reason why separate allocation and mapping (and conversely,
separate unmapping and deallocation) are required is the
ExitBootServices() callbacks. In that callback, unmapping must happen
*without* memory allocation or deallocation (because the IOMMU must be
de-programmed, but the UEFI memmap must not be changed), covering all
memory areas that are at that point shared between host and guest
(regardless of transfer direction).
Normally, Map allocates the bounce buffer internally, and Unmap releases
the bounce buffer internally (for BusMasterRead and BusMasterWrite),
which is not right here. If you use AllocateBuffer() separately, then
Map() -- with BusMasterCommonBuffer -- will not allocate internally, and
Unmap() will also not deallocate internally. So, in the
ExitBootServices() callback, you will be able to call Unmap() only, and
then *not* call FreeBuffer() at all.
This is why I suggest introducing all four functions (Allocate, Free,
Map, Unmap) to the VIRTIO_DEVICE_PROTOCOL, and why all virtio drivers
should use all four functions explicitly, not just Map and Unmap.
... Actually, I think there is a bug in
"OvmfPkg/IoMmuDxe/AmdSevIoMmu.c". You have the following distribution of
operations at the moment:
- IoMmuAllocateBuffer() allocates pages and clears the memory encryption
mask
- IoMmuFreeBuffer() re-sets the memory encryption mask, and deallocates
pages
- IoMmuMap() does nothing at all when BusMasterCommonBuffer is requested
(and IoMmuAllocateBuffer() was called previously). Otherwise,
IoMmuMap() allocates pages, allocates MAP_INFO, and clears the memory
encryption mask.
- IoMmuUnmap() does nothing when cleaning up a BusMasterCommonBuffer
operation (--> NO_MAPPING). Otherwise, IoMmuUnmap() clears the
encryption mask, and frees both the pages and MAP_INFO.
I agree with you, there is a bug in AmdSevIoMmu.c. I will fix it. And introduce
list to track if the buffer was allocated by us. If buffer was allocated by
us then we can safely say that it does not require a bounce buffer. While
implementing the initial code I was not able to see BusMasterCommonBuffer
usage. But with virtio things are getting a bit more clear in my mind.
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).
*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?)
Yes, MemEncryptSevSetPageEncMask() will not split pages when it restores mask.
I'm sorry that I didn't catch this earlier in your commit f9d129e68a45
("OvmfPkg: Add IoMmuDxe driver", 2017-07-06), but as you see, I gave you
only an Acked-by on that, not a Reviewed-by. I've really had to think it
through from the virtio perspective; I didn't foresee this use case back
then (I only felt that I wasn't getting the full picture about the IOMMU
protocol details).
I have to concur with Laszlo here: the respective semantics of the
allocate/map/unmap/free operations should be identical to their
counterparts in the PCI I/O protocol, and in the DmaLib in
EmbeddedPkg.
Note that this is likely to cause problems with proprietary x86
drivers in option ROMs, which are used to getting away with using host
addresses for DMA, and fail to invoke Map() for common buffers (or
fail to invoke AllocateBuffer() altogether). The API is clear, and
drivers that abide by the rules should operate correctly even when
using encrypted memory or non-1:1 mapping between the host and PCI
address space (which is how I ran into these issues)
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
