On Mon, May 09, 2016 at 06:44:42PM +0200, Ard Biesheuvel wrote: > 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.
Yeah, that was kind of my meaning: The only way it could happen would be through * memory corruption * intentional manipulation of the structure both of which would make it hard to guarantee that the data had been "committed to the target system memory". Anyway, it's not a hard requirement, more of a discussion point. Reviewed-by: Leif Lindholm <[email protected]> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
