On Tue, May 10, 2016 at 12:58:08PM +0200, Ard Biesheuvel wrote: > On 9 May 2016 at 22:17, Leif Lindholm <[email protected]> wrote: > > 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". > > > > Indeed. So an ASSERT () is appropriate here, but I guess you were > looking for an equivalent in RELEASE builds? In that case, > EFI_INVALID_PARAMETER seems more appropriate
Yeah - so could we have both? If so, just change and push. > > 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
