On 07/31/2017 02:49 PM, Ard Biesheuvel wrote:
On 31 July 2017 at 20:31, Brijesh Singh <[email protected]> wrote:The current implementation was making assumption that AllocateBuffer() returns a buffer with C-bit cleared. Hence when we were asked to Map() with BusMasterCommonBuffer, we do not change the C-bit on host buffer. In previous patch, we changed the AllocateBuffer() to not clear C-bit during allocation. The patch adds support for handling the BusMasterCommonBuffer operations when SEV is active. A typical DMA Bus master Common Operation follows the below step: 1. Client calls AllocateBuffer() to allocate a common buffer 2. Client fill some data in common buffer (optional) 3. Client calls Map() with BusMasterCommonBuffer 4. Programs the DMA bus master with the device address returned by Map() 5. The common buffer can now be accessed equally by the processor and the DMA bus master. 6. Client calls Unmap() 7. Client calls FreeBuffer() In order to handle steps #2 (in which common buffer may contain data), we perform in-place encryption to ensure that device address returned by the Map() contains the correct data after we clear the C-bit during Map(). In my measurement I do not see any noticable perform degradation when performing in-place encryption/decryption on common buffer. Suggested-by: Laszlo Ersek <[email protected]> Contributed-under: TianoCore Contribution Agreement 1.0 Cc: Laszlo Ersek <[email protected]> Cc: Jordan Justen <[email protected]> Signed-off-by: Brijesh Singh <[email protected]> --- OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 190 +++++++++++++++++--- 1 file changed, 164 insertions(+), 26 deletions(-)Hello Brijesh, I haven't looked in detail at the existing code, but please don't conflate the device address with the address of a bounce buffer. These are very different things, although the confusion is understandable (and precedented) when not used to dealing with non-1:1 DMA. The device address is what gets programmed into the device's DMA registers. If there is a fixed [non-zero] offset between the device's view of memory and the host's (as may be the case with PCI, or generally when using an IOMMU), then the device is the only one who should attempt to perform memory accesses using this address. So please void SetMem() or other CPU dereferences involving the device address, and treat it as an opaque handle instead.
In your case, you are dealing with a bounce buffer. So call it bounce buffer in the MapInfo struct. Imagine when dealing with a non-linear host to PCI mapping, you will still need to perform an additional translation to derive the device address from the bounce buffer address.
Agreed. Initially, AmdSevIoMmu.c code was derived from PciRootBridgeIo and MAP_INFO structure was literally copied. I will probably send a separate patch to fix the structure member and update the comments to reflect its true meaning. Thanks Brijesh _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
