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