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

[Laszlo Ersek <lersek@redhat.com>]

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 <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.

Yes, we should call it "PlainTextAddress" or something similar.

Thanks
Laszlo