| Message ID | 1461077734-4327-4-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 <ard.biesheuvel@linaro.org> > --- > 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? > + > + if (Map->Operation == MapOperationBusMasterWrite) { > CopyMem ((VOID *)(UINTN)Map->HostAddress, (VOID *)(UINTN)Map->DeviceAddress, Map->NumberOfBytes); > } > > -- > 2.5.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 9 May 2016 at 18:37, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> >> --- >> 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Mon, May 09, 2016 at 06:44:42PM +0200, Ard Biesheuvel wrote: > On 9 May 2016 at 18:37, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> > >> --- > >> 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 <leif.lindholm@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 9 May 2016 at 22:17, Leif Lindholm <leif.lindholm@linaro.org> wrote: > On Mon, May 09, 2016 at 06:44:42PM +0200, Ard Biesheuvel wrote: >> On 9 May 2016 at 18:37, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> >> >> --- >> >> 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 <leif.lindholm@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, May 10, 2016 at 12:58:08PM +0200, Ard Biesheuvel wrote: > On 9 May 2016 at 22:17, Leif Lindholm <leif.lindholm@linaro.org> wrote: > > On Mon, May 09, 2016 at 06:44:42PM +0200, Ard Biesheuvel wrote: > >> On 9 May 2016 at 18:37, Leif Lindholm <leif.lindholm@linaro.org> 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 <ard.biesheuvel@linaro.org> > >> >> --- > >> >> 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 <leif.lindholm@linaro.org> _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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); + + if (Map->Operation == MapOperationBusMasterWrite) { CopyMem ((VOID *)(UINTN)Map->HostAddress, (VOID *)(UINTN)Map->DeviceAddress, Map->NumberOfBytes); }
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 <ard.biesheuvel@linaro.org> --- ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel