On 9 May 2016 at 18:37, Leif Lindholm <[email protected]> wrote: > On Tue, Apr 19, 2016 at 04:55:33PM +0200, Ard Biesheuvel wrote: >> DmaMap () operations of type MapOperationBusMasterCommonBuffer should >> return a mapping that is coherent between the CPU and the device. For >> this reason, the API only allows DmaMap () to be called with this operation >> type if the memory to be mapped was allocated by DmaAllocateBuffer (), >> which in this implementation guarantees the coherency by using uncached >> mappings on the CPU side. >> >> This means that, if we encounter a cached mapping in DmaMap () with this >> operation type, the code is either broken, or someone is violating the >> API, but simply proceeding with a double buffer makes no sense at all, >> and can only cause problems. >> >> So instead, actively reject this operation type for cached memory mappings. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <[email protected]> >> --- >> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 18 ++++++++++++++++-- >> 1 file changed, 16 insertions(+), 2 deletions(-) >> >> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c >> b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c >> index 7f6598318a91..7e518ed3b83e 100644 >> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c >> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c >> @@ -103,6 +103,18 @@ DmaMap ( >> // If the mapped buffer is not an uncached buffer >> if ((GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) != 0) { >> // >> + // Operations of type MapOperationBusMasterCommonBuffer are only >> allowed >> + // on uncached buffers. >> + // >> + if (Operation == MapOperationBusMasterCommonBuffer) { >> + DEBUG ((EFI_D_ERROR, >> + "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only >> supported\n" >> + "on memory regions that were allocated using DmaAllocateBuffer >> ()\n", >> + __FUNCTION__)); >> + return EFI_UNSUPPORTED; >> + } >> + >> + // >> // If the buffer does not fill entire cache lines we must double >> buffer into >> // uncached memory. Device (PCI) address becomes uncached page. >> // >> @@ -112,7 +124,7 @@ DmaMap ( >> return Status; >> } >> >> - if ((Operation == MapOperationBusMasterRead) || (Operation == >> MapOperationBusMasterCommonBuffer)) { >> + if (Operation == MapOperationBusMasterRead) { >> CopyMem (Buffer, HostAddress, *NumberOfBytes); >> } >> >> @@ -168,7 +180,9 @@ DmaUnmap ( >> Map = (MAP_INFO_INSTANCE *)Mapping; >> >> if (Map->DoubleBuffer) { >> - if ((Map->Operation == MapOperationBusMasterWrite) || (Map->Operation >> == MapOperationBusMasterCommonBuffer)) { >> + ASSERT (Map->Operation != MapOperationBusMasterCommonBuffer); > > Would it be more correct to return EFI_DEVICE_ERROR if this > condition became true? >
I don't think so. We should never create double buffer mappings for MapOperationBusMasterCommonBuffer operations, so in the unmap path, an ASSERT () is appropriate, since the code is doing something *very* if this ever occurs. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
