On 07/31/17 22:27, Brijesh Singh wrote: > > > 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.
Yes, we should call it "PlainTextAddress" or something similar. Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
