[edk2,4/5] ArmPkg/ArmDmaLib: reject consistent DMA mappings of cached memory

Message ID 1461077734-4327-4-git-send-email-ard.biesheuvel@linaro.org
State New
Headers show

Commit Message

Ard Biesheuvel April 19, 2016, 2:55 p.m.
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

Comments

Leif Lindholm May 9, 2016, 4:37 p.m. | #1
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
Ard Biesheuvel May 9, 2016, 4:44 p.m. | #2
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
Leif Lindholm May 9, 2016, 8:17 p.m. | #3
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
Ard Biesheuvel May 10, 2016, 10:58 a.m. | #4
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
Leif Lindholm May 10, 2016, 11:18 a.m. | #5
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

Patch

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);
     }