Re: [PATCH v1 2/4] OvmfPkg: IommuDxe: Provide support for mapping BusMasterCommonBuffer operation

[Brijesh Singh <brijesh.singh@amd.com>]

On 07/31/2017 02:49 PM, Ard Biesheuvel wrote:
> On 31 July 2017 at 20:31, Brijesh Singh <brijesh.singh@amd.com> 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 <lersek@redhat.com>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>   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