Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address

[Ryan Harkin <ryan.harkin@linaro.org>]

On 15 November 2016 at 09:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Sat, Nov 12, 2016 at 02:02:27PM +0100, Ard Biesheuvel wrote:
>>> In preparation of adding support to ArmDmalib for DMA bus masters whose
>>> view of memory is offset by a constant compared to the CPU's view, clean
>>> up some abuse of the device address.
>>>
>>> The device address is not defined in terms of the CPU's address space,
>>> and so it should not be used in CopyMem () or cache maintenance operations
>>> that require a valid mapping. This not only applies to the above use case,
>>> but also to the DebugUncachedMemoryAllocationLib that unmaps the
>>> primary, cached mapping of an allocation, and returns a host address
>>> which is an uncached alias offset by a constant.
>>>
>>> Since we should never access the device address from the CPU, there is
>>> no need to record it in the MAPINFO struct. Instead, record the buffer
>>> address in case of double buffering, since we do need to copy the contents
>>> (in case of a bus master write) and free the buffer (in all cases) when
>>> DmaUnmap() is called.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> For the fix itself:
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> However, can we wait for a few Tested-by:s to ensure this fix does not
>> reveal any companion bugs?
>>
>
> Perhaps, yes.
>
> In case anyone is up to doing that, please find the branch here
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset
>

I tested your branch on the usual victims (R0/1/2, FVP Foundation &
AEMv8 and TC2) and they all work fine for me.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>


> However, given that the split CPU/bus master view is introduced in the
> next patch, the only use case where the device address differs from
> the host address is when using the DebugUncachedMemoryAllocationLib,
> which is currently broken AFAICT (it attempts to unmap the linear
> mapping of the allocation by setting the memory attributes to '0',
> which triggers an assert in the ArmPkg MMU code)