[PATCH 0/3] Fix a bug that prevents PMEM access

[PATCH 0/3] Fix a bug that prevents PMEM access

[Ruiyu Ni]

The patch sets fix a bug in PciHostBridge driver that prevents PMEM access
sometimes. Additionally, it enhances the boundary check and add a new macro
to simplify the code.

Ruiyu Ni (3):
  MdeModulePkg/PciHostBridge: Enhance boundary check in
    Io/Mem.Read/Write
  MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access
  MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

 .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 64 ++++++++++++++--------
 1 file changed, 42 insertions(+), 22 deletions(-)

-- 
2.16.1.windows.1

[PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 +++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f8a1239ceb..0b6b56f846 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
   UINT64                                       Base;
   UINT64                                       Limit;
   UINT32                                       Size;
+  UINT64                                       Length;
 
   //
   // Check to see if Buffer is NULL
@@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
   }
 
   //
-  // For FIFO type, the target address won't increase during the access,
+  // For FIFO type, the device address won't increase during the access,
   // so treat Count as 1
   //
   if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) {
@@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
   Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
   Size  = 1 << Width;
 
+  //
+  // Make sure (Count * Size) doesn't exceed MAX_UINT64
+  //
+  if (Count > DivU64x32 (MAX_UINT64, Size)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   //
   // Check to see if Address is aligned
   //
@@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
     return EFI_UNSUPPORTED;
   }
 
+  //
+  // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
+  //
+  Length = MultU64x32 (Count, Size);
+  if (Address > MAX_UINT64 - Length) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   RootBridge = ROOT_BRIDGE_FROM_THIS (This);
 
   //
@@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
     //
     // Allow Legacy IO access
     //
-    if (Address + MultU64x32 (Count, Size) <= 0x1000) {
+    if (Address + Length <= 0x1000) {
       if ((RootBridge->Attributes & (
            EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
            EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
@@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
     //
     // Allow Legacy MMIO access
     //
-    if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
+    if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
       if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
         return EFI_SUCCESS;
       }
@@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
     // By comparing the Address against Limit we know which range to be used
     // for checking
     //
-    if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
+    if (Address + Length <= RootBridge->Mem.Limit + 1) {
       Base = RootBridge->Mem.Base;
       Limit = RootBridge->Mem.Limit;
     } else {
@@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
       return EFI_INVALID_PARAMETER;
   }
 
-  if (Address + MultU64x32 (Count, Size) > Limit + 1) {
+  if (Address + Length > Limit + 1) {
     return EFI_INVALID_PARAMETER;
   }
 
-- 
2.16.1.windows.1

Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

[Laszlo Ersek]

On 09/21/18 09:25, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 +++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index f8a1239ceb..0b6b56f846 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
>    UINT64                                       Base;
>    UINT64                                       Limit;
>    UINT32                                       Size;
> +  UINT64                                       Length;
>  
>    //
>    // Check to see if Buffer is NULL
> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
>    }
>  
>    //
> -  // For FIFO type, the target address won't increase during the access,
> +  // For FIFO type, the device address won't increase during the access,
>    // so treat Count as 1
>    //
>    if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) {
> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
>    Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>    Size  = 1 << Width;
>  
> +  //
> +  // Make sure (Count * Size) doesn't exceed MAX_UINT64
> +  //
> +  if (Count > DivU64x32 (MAX_UINT64, Size)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    //
>    // Check to see if Address is aligned
>    //
> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
>      return EFI_UNSUPPORTED;
>    }
>  
> +  //
> +  // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
> +  //
> +  Length = MultU64x32 (Count, Size);
> +  if (Address > MAX_UINT64 - Length) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
>    RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>  
>    //
> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
>      //
>      // Allow Legacy IO access
>      //
> -    if (Address + MultU64x32 (Count, Size) <= 0x1000) {
> +    if (Address + Length <= 0x1000) {
>        if ((RootBridge->Attributes & (
>             EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
>             EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
>      //
>      // Allow Legacy MMIO access
>      //
> -    if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
> +    if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
>        if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
>          return EFI_SUCCESS;
>        }
> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
>      // By comparing the Address against Limit we know which range to be used
>      // for checking
>      //
> -    if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
> +    if (Address + Length <= RootBridge->Mem.Limit + 1) {
>        Base = RootBridge->Mem.Base;
>        Limit = RootBridge->Mem.Limit;
>      } else {
> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
>        return EFI_INVALID_PARAMETER;
>    }
>  
> -  if (Address + MultU64x32 (Count, Size) > Limit + 1) {
> +  if (Address + Length > Limit + 1) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> 

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

[Kirkendall, Garrett]

GARRETT KIRKENDALL
SMTS Firmware Engineer | CTE
7171 Southwest Parkway, Austin, TX 78735 USA 
AMD   facebook  |  amd.com

-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ruiyu Ni
Sent: Friday, September 21, 2018 2:26 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 +++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f8a1239ceb..0b6b56f846 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
   UINT64                                       Base;
   UINT64                                       Limit;
   UINT32                                       Size;
+  UINT64                                       Length;
 
   //
   // Check to see if Buffer is NULL
@@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
   }
 
   //
-  // For FIFO type, the target address won't increase during the access,
+  // For FIFO type, the device address won't increase during the 
+ access,
   // so treat Count as 1
   //
   if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) { @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
   Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
   Size  = 1 << Width;
 
+  //
+  // Make sure (Count * Size) doesn't exceed MAX_UINT64  //  if (Count 
+ > DivU64x32 (MAX_UINT64, Size)) {
+    return EFI_INVALID_PARAMETER;
+  }
+
   //
   // Check to see if Address is aligned
   //
@@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
     return EFI_UNSUPPORTED;
   }
 
+  //
+  // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64  //  
+ Length = MultU64x32 (Count, Size);  if (Address > MAX_UINT64 - Length) 
+ {
+    return EFI_INVALID_PARAMETER;
+  }
+
   RootBridge = ROOT_BRIDGE_FROM_THIS (This);
 
   //
@@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
     //
     // Allow Legacy IO access
     //
-    if (Address + MultU64x32 (Count, Size) <= 0x1000) {
+    if (Address + Length <= 0x1000) {
       if ((RootBridge->Attributes & (
            EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
            EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO | @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
     //
     // Allow Legacy MMIO access
     //
-    if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
+    if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
       if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
         return EFI_SUCCESS;
       }
@@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
     // By comparing the Address against Limit we know which range to be used
     // for checking
     //
-    if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
+    if (Address + Length <= RootBridge->Mem.Limit + 1) {
       Base = RootBridge->Mem.Base;
       Limit = RootBridge->Mem.Limit;
     } else {
@@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
       return EFI_INVALID_PARAMETER;
   }
 
-  if (Address + MultU64x32 (Count, Size) > Limit + 1) {
+  if (Address + Length > Limit + 1) {
     return EFI_INVALID_PARAMETER;
   }
 
--
2.16.1.windows.1

Reviewed-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

[Zeng, Star]

Two very small comments are added below.

On 2018/9/21 15:25, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 +++++++++++++++++-----
>   1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index f8a1239ceb..0b6b56f846 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
>     UINT64                                       Base;
>     UINT64                                       Limit;
>     UINT32                                       Size;
> +  UINT64                                       Length;
>   
>     //
>     // Check to see if Buffer is NULL
> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
>     }
>   
>     //
> -  // For FIFO type, the target address won't increase during the access,
> +  // For FIFO type, the device address won't increase during the access,
>     // so treat Count as 1
>     //
>     if (Width >= EfiPciWidthFifoUint8 && Width <= EfiPciWidthFifoUint64) {
> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
>     Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>     Size  = 1 << Width;
>   
> +  //
> +  // Make sure (Count * Size) doesn't exceed MAX_UINT64
> +  //
> +  if (Count > DivU64x32 (MAX_UINT64, Size)) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +

Mark as "Code Block 1".

>     //
>     // Check to see if Address is aligned
>     //
> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
>       return EFI_UNSUPPORTED;
>     }
>   
> +  //
> +  // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
> +  //
> +  Length = MultU64x32 (Count, Size);
> +  if (Address > MAX_UINT64 - Length) {
> +    return EFI_INVALID_PARAMETER;
> +  }
> +

Is there some reason this code block is not put together with the "Code 
Block 1"? Both are checking integer overflow.

How about also enhancing the function description a little to add one 
line for describing the overflow invalid parameter cases?

   @retval EFI_INVALID_PARAMETER  XXX.

or just updating the line below?

   @retval EFI_UNSUPPORTED        The address range specified by 
Address, Width,
                                  and Count is not valid for this PI system.


Thanks,
Star

>     RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>   
>     //
> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
>       //
>       // Allow Legacy IO access
>       //
> -    if (Address + MultU64x32 (Count, Size) <= 0x1000) {
> +    if (Address + Length <= 0x1000) {
>         if ((RootBridge->Attributes & (
>              EFI_PCI_ATTRIBUTE_ISA_IO | EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
>              EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
>       //
>       // Allow Legacy MMIO access
>       //
> -    if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) <= 0xC0000) {
> +    if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
>         if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 0) {
>           return EFI_SUCCESS;
>         }
> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
>       // By comparing the Address against Limit we know which range to be used
>       // for checking
>       //
> -    if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 1) {
> +    if (Address + Length <= RootBridge->Mem.Limit + 1) {
>         Base = RootBridge->Mem.Base;
>         Limit = RootBridge->Mem.Limit;
>       } else {
> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
>         return EFI_INVALID_PARAMETER;
>     }
>   
> -  if (Address + MultU64x32 (Count, Size) > Limit + 1) {
> +  if (Address + Length > Limit + 1) {
>       return EFI_INVALID_PARAMETER;
>     }
>   
> 

Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

[Ni, Ruiyu]

On 9/25/2018 10:14 AM, Zeng, Star wrote:
> Two very small comments are added below.
> 
> On 2018/9/21 15:25, Ruiyu Ni wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> ---
>>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 
>> +++++++++++++++++-----
>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index f8a1239ceb..0b6b56f846 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
>>     UINT64                                       Base;
>>     UINT64                                       Limit;
>>     UINT32                                       Size;
>> +  UINT64                                       Length;
>>     //
>>     // Check to see if Buffer is NULL
>> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
>>     }
>>     //
>> -  // For FIFO type, the target address won't increase during the access,
>> +  // For FIFO type, the device address won't increase during the access,
>>     // so treat Count as 1
>>     //
>>     if (Width >= EfiPciWidthFifoUint8 && Width <= 
>> EfiPciWidthFifoUint64) {
>> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
>>     Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>>     Size  = 1 << Width;
>> +  //
>> +  // Make sure (Count * Size) doesn't exceed MAX_UINT64
>> +  //
>> +  if (Count > DivU64x32 (MAX_UINT64, Size)) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
> 
> Mark as "Code Block 1".
> 
>>     //
>>     // Check to see if Address is aligned
>>     //
>> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
>>       return EFI_UNSUPPORTED;
>>     }
>> +  //
>> +  // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
>> +  //
>> +  Length = MultU64x32 (Count, Size);
>> +  if (Address > MAX_UINT64 - Length) {
>> +    return EFI_INVALID_PARAMETER;
>> +  }
>> +
> 
> Is there some reason this code block is not put together with the "Code 
> Block 1"? Both are checking integer overflow.

This code block is to check whether the Address is valid.
I group the code by the parameter. If you check the original code, you 
will see the checks performed on parameters: Buffer, Width, Count, Address.


> 
> How about also enhancing the function description a little to add one 
> line for describing the overflow invalid parameter cases?
> 
>    @retval EFI_INVALID_PARAMETER  XXX.

Sure, I will send V2 with the updated function description.

> 
> or just updating the line below?
> 
>    @retval EFI_UNSUPPORTED        The address range specified by 
> Address, Width,
>                                   and Count is not valid for this PI 
> system.
> 
> 
> Thanks,
> Star
> 
>>     RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>     //
>> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
>>       //
>>       // Allow Legacy IO access
>>       //
>> -    if (Address + MultU64x32 (Count, Size) <= 0x1000) {
>> +    if (Address + Length <= 0x1000) {
>>         if ((RootBridge->Attributes & (
>>              EFI_PCI_ATTRIBUTE_ISA_IO | 
>> EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
>>              EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | 
>> EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
>> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
>>       //
>>       // Allow Legacy MMIO access
>>       //
>> -    if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) 
>> <= 0xC0000) {
>> +    if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
>>         if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) != 
>> 0) {
>>           return EFI_SUCCESS;
>>         }
>> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
>>       // By comparing the Address against Limit we know which range to 
>> be used
>>       // for checking
>>       //
>> -    if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit + 
>> 1) {
>> +    if (Address + Length <= RootBridge->Mem.Limit + 1) {
>>         Base = RootBridge->Mem.Base;
>>         Limit = RootBridge->Mem.Limit;
>>       } else {
>> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
>>         return EFI_INVALID_PARAMETER;
>>     }
>> -  if (Address + MultU64x32 (Count, Size) > Limit + 1) {
>> +  if (Address + Length > Limit + 1) {
>>       return EFI_INVALID_PARAMETER;
>>     }
>>
> 


-- 
Thanks,
Ray

Re: [PATCH 1/3] MdeModulePkg/PciHostBridge: Enhance boundary check in Io/Mem.Read/Write

[Zeng, Star]

On 2018/9/25 10:43, Ni, Ruiyu wrote:
> On 9/25/2018 10:14 AM, Zeng, Star wrote:
>> Two very small comments are added below.
>>
>> On 2018/9/21 15:25, Ruiyu Ni wrote:
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> ---
>>>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 
>>> +++++++++++++++++-----
>>>   1 file changed, 21 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index f8a1239ceb..0b6b56f846 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -321,6 +321,7 @@ RootBridgeIoCheckParameter (
>>>     UINT64                                       Base;
>>>     UINT64                                       Limit;
>>>     UINT32                                       Size;
>>> +  UINT64                                       Length;
>>>     //
>>>     // Check to see if Buffer is NULL
>>> @@ -337,7 +338,7 @@ RootBridgeIoCheckParameter (
>>>     }
>>>     //
>>> -  // For FIFO type, the target address won't increase during the 
>>> access,
>>> +  // For FIFO type, the device address won't increase during the 
>>> access,
>>>     // so treat Count as 1
>>>     //
>>>     if (Width >= EfiPciWidthFifoUint8 && Width <= 
>>> EfiPciWidthFifoUint64) {
>>> @@ -347,6 +348,13 @@ RootBridgeIoCheckParameter (
>>>     Width = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_WIDTH) (Width & 0x03);
>>>     Size  = 1 << Width;
>>> +  //
>>> +  // Make sure (Count * Size) doesn't exceed MAX_UINT64
>>> +  //
>>> +  if (Count > DivU64x32 (MAX_UINT64, Size)) {
>>> +    return EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>
>> Mark as "Code Block 1".
>>
>>>     //
>>>     // Check to see if Address is aligned
>>>     //
>>> @@ -354,6 +362,14 @@ RootBridgeIoCheckParameter (
>>>       return EFI_UNSUPPORTED;
>>>     }
>>> +  //
>>> +  // Make sure (Address + Count * Size) doesn't exceed MAX_UINT64
>>> +  //
>>> +  Length = MultU64x32 (Count, Size);
>>> +  if (Address > MAX_UINT64 - Length) {
>>> +    return EFI_INVALID_PARAMETER;
>>> +  }
>>> +
>>
>> Is there some reason this code block is not put together with the 
>> "Code Block 1"? Both are checking integer overflow.
> 
> This code block is to check whether the Address is valid.
> I group the code by the parameter. If you check the original code, you 
> will see the checks performed on parameters: Buffer, Width, Count, Address.

Got it about the checking sequence.
But even this code block is moved to before "Check to see if Address is 
aligned" and after "Make sure (Count * Size) doesn't exceed MAX_UINT64", 
the checking sequence is kept. Only difference is first checking 
unsupported or first checking invalid for Address parameter.

Anyway, I have no strong opinion for that. You can decide. :)

> 
> 
>>
>> How about also enhancing the function description a little to add one 
>> line for describing the overflow invalid parameter cases?
>>
>>    @retval EFI_INVALID_PARAMETER  XXX.
> 
> Sure, I will send V2 with the updated function description.

Thanks. You may consider just updating the below EFI_UNSUPPORTED to 
EFI_INVALID_PARAMETER.

   @retval EFI_UNSUPPORTED        The address range specified by 
Address, Width,
                                  and Count is not valid for this PI system.



Star

> 
>>
>> or just updating the line below?
>>
>>    @retval EFI_UNSUPPORTED        The address range specified by 
>> Address, Width,
>>                                   and Count is not valid for this PI 
>> system.
>>
>>
>> Thanks,
>> Star
>>
>>>     RootBridge = ROOT_BRIDGE_FROM_THIS (This);
>>>     //
>>> @@ -372,7 +388,7 @@ RootBridgeIoCheckParameter (
>>>       //
>>>       // Allow Legacy IO access
>>>       //
>>> -    if (Address + MultU64x32 (Count, Size) <= 0x1000) {
>>> +    if (Address + Length <= 0x1000) {
>>>         if ((RootBridge->Attributes & (
>>>              EFI_PCI_ATTRIBUTE_ISA_IO | 
>>> EFI_PCI_ATTRIBUTE_VGA_PALETTE_IO | EFI_PCI_ATTRIBUTE_VGA_IO |
>>>              EFI_PCI_ATTRIBUTE_IDE_PRIMARY_IO | 
>>> EFI_PCI_ATTRIBUTE_IDE_SECONDARY_IO |
>>> @@ -386,7 +402,7 @@ RootBridgeIoCheckParameter (
>>>       //
>>>       // Allow Legacy MMIO access
>>>       //
>>> -    if ((Address >= 0xA0000) && (Address + MultU64x32 (Count, Size)) 
>>> <= 0xC0000) {
>>> +    if ((Address >= 0xA0000) && (Address + Length) <= 0xC0000) {
>>>         if ((RootBridge->Attributes & EFI_PCI_ATTRIBUTE_VGA_MEMORY) 
>>> != 0) {
>>>           return EFI_SUCCESS;
>>>         }
>>> @@ -395,7 +411,7 @@ RootBridgeIoCheckParameter (
>>>       // By comparing the Address against Limit we know which range 
>>> to be used
>>>       // for checking
>>>       //
>>> -    if (Address + MultU64x32 (Count, Size) <= RootBridge->Mem.Limit 
>>> + 1) {
>>> +    if (Address + Length <= RootBridge->Mem.Limit + 1) {
>>>         Base = RootBridge->Mem.Base;
>>>         Limit = RootBridge->Mem.Limit;
>>>       } else {
>>> @@ -427,7 +443,7 @@ RootBridgeIoCheckParameter (
>>>         return EFI_INVALID_PARAMETER;
>>>     }
>>> -  if (Address + MultU64x32 (Count, Size) > Limit + 1) {
>>> +  if (Address + Length > Limit + 1) {
>>>       return EFI_INVALID_PARAMETER;
>>>     }
>>>
>>
> 
> 

[PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

[Ruiyu Ni]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196

RootBridgeIoCheckParameter() verifies that the requested MMIO access
can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
somehow only checks the requested access against MEM 32/64 ranges.

It should also check the requested access against PMEM 32/64 ranges.

The patch fixes this issue.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 0b6b56f846..f6234b5d11 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
     // By comparing the Address against Limit we know which range to be used
     // for checking
     //
-    if (Address + Length <= RootBridge->Mem.Limit + 1) {
-      Base = RootBridge->Mem.Base;
+    if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
+      Base  = RootBridge->Mem.Base;
       Limit = RootBridge->Mem.Limit;
-    } else {
-      Base = RootBridge->MemAbove4G.Base;
+    } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
+      Base  = RootBridge->PMem.Base;
+      Limit = RootBridge->PMem.Limit;
+    } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
+      Base  = RootBridge->MemAbove4G.Base;
       Limit = RootBridge->MemAbove4G.Limit;
+    } else {
+      Base  = RootBridge->PMemAbove4G.Base;
+      Limit = RootBridge->PMemAbove4G.Limit;
     }
   } else {
     PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
-- 
2.16.1.windows.1

Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

[Laszlo Ersek]

On 09/21/18 09:25, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
> 
> RootBridgeIoCheckParameter() verifies that the requested MMIO access
> can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
> somehow only checks the requested access against MEM 32/64 ranges.
> 
> It should also check the requested access against PMEM 32/64 ranges.
> 
> The patch fixes this issue.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
> ---
>  MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 0b6b56f846..f6234b5d11 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
>      // By comparing the Address against Limit we know which range to be used
>      // for checking
>      //
> -    if (Address + Length <= RootBridge->Mem.Limit + 1) {
> -      Base = RootBridge->Mem.Base;
> +    if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
> +      Base  = RootBridge->Mem.Base;
>        Limit = RootBridge->Mem.Limit;
> -    } else {
> -      Base = RootBridge->MemAbove4G.Base;
> +    } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
> +      Base  = RootBridge->PMem.Base;
> +      Limit = RootBridge->PMem.Limit;
> +    } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
> +      Base  = RootBridge->MemAbove4G.Base;
>        Limit = RootBridge->MemAbove4G.Limit;
> +    } else {
> +      Base  = RootBridge->PMemAbove4G.Base;
> +      Limit = RootBridge->PMemAbove4G.Limit;
>      }
>    } else {
>      PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
> 

The interesting thing about this patch is that, if any one of the first
three branches is taken, then the final checks will automatically pass.
That's because, on the first three branches, we select the base & the
limit *because* the access falls between them. Therefore, in the end,
when we check whether the access falls between base and end, they
miraculously happen to do so. :)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

[Ni, Ruiyu]

On 9/21/2018 7:06 PM, Laszlo Ersek wrote:
> On 09/21/18 09:25, Ruiyu Ni wrote:
> 
> The interesting thing about this patch is that, if any one of the first
> three branches is taken, then the final checks will automatically pass.
> That's because, on the first three branches, we select the base & the
> limit *because* the access falls between them. Therefore, in the end,
> when we check whether the access falls between base and end, they
> miraculously happen to do so. :)

The code was written like this to maximally share the final check code:)

> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray

Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

[Kirkendall, Garrett]

GARRETT KIRKENDALL
SMTS Firmware Engineer | CTE
7171 Southwest Parkway, Austin, TX 78735 USA 
AMD   facebook  |  amd.com

-----Original Message-----
From: Ruiyu Ni <ruiyu.ni@intel.com> 
Sent: Friday, September 21, 2018 2:26 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>; Kirkendall; Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
Subject: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196

RootBridgeIoCheckParameter() verifies that the requested MMIO access can fit in any of the MEM/PMEM 32/64 ranges. But today's logic somehow only checks the requested access against MEM 32/64 ranges.

It should also check the requested access against PMEM 32/64 ranges.

The patch fixes this issue.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>
---
 MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index 0b6b56f846..f6234b5d11 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
     // By comparing the Address against Limit we know which range to be used
     // for checking
     //
-    if (Address + Length <= RootBridge->Mem.Limit + 1) {
-      Base = RootBridge->Mem.Base;
+    if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
+      Base  = RootBridge->Mem.Base;
       Limit = RootBridge->Mem.Limit;
-    } else {
-      Base = RootBridge->MemAbove4G.Base;
+    } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
+      Base  = RootBridge->PMem.Base;
+      Limit = RootBridge->PMem.Limit;
+    } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
+      Base  = RootBridge->MemAbove4G.Base;
       Limit = RootBridge->MemAbove4G.Limit;
+    } else {
+      Base  = RootBridge->PMemAbove4G.Base;
+      Limit = RootBridge->PMemAbove4G.Limit;
     }
   } else {
     PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
--
2.16.1.windows.1

Reviewed-by: Garrett Kirkendall <garrett.kirkendall@amd.com>

Re: [PATCH 2/3] MdeModulePkg/PciHostBridge: Fix a bug that prevents PMEM access

[Zeng, Star]

On 2018/9/21 15:25, Ruiyu Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1196
> 
> RootBridgeIoCheckParameter() verifies that the requested MMIO access
> can fit in any of the MEM/PMEM 32/64 ranges. But today's logic
> somehow only checks the requested access against MEM 32/64 ranges.
> 
> It should also check the requested access against PMEM 32/64 ranges.
> 
> The patch fixes this issue.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Kirkendall, Garrett <Garrett.Kirkendall@amd.com>

Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star

> ---
>   MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index 0b6b56f846..f6234b5d11 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -411,12 +411,18 @@ RootBridgeIoCheckParameter (
>       // By comparing the Address against Limit we know which range to be used
>       // for checking
>       //
> -    if (Address + Length <= RootBridge->Mem.Limit + 1) {
> -      Base = RootBridge->Mem.Base;
> +    if ((Address >= RootBridge->Mem.Base) && (Address + Length <= RootBridge->Mem.Limit + 1)) {
> +      Base  = RootBridge->Mem.Base;
>         Limit = RootBridge->Mem.Limit;
> -    } else {
> -      Base = RootBridge->MemAbove4G.Base;
> +    } else if ((Address >= RootBridge->PMem.Base) && (Address + Length <= RootBridge->PMem.Limit + 1)) {
> +      Base  = RootBridge->PMem.Base;
> +      Limit = RootBridge->PMem.Limit;
> +    } else if ((Address >= RootBridge->MemAbove4G.Base) && (Address + Length <= RootBridge->MemAbove4G.Limit + 1)) {
> +      Base  = RootBridge->MemAbove4G.Base;
>         Limit = RootBridge->MemAbove4G.Limit;
> +    } else {
> +      Base  = RootBridge->PMemAbove4G.Base;
> +      Limit = RootBridge->PMemAbove4G.Limit;
>       }
>     } else {
>       PciRbAddr = (EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL_PCI_ADDRESS*) &Address;
> 

[PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Ruiyu Ni]

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 ++++++++++------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f6234b5d11..916709e276 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
 
 #define NO_MAPPING  (VOID *) (UINTN) -1
 
+#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
+
 //
 // Lookup table for increment values based on transfer widths
 //
@@ -122,25 +124,25 @@ CreateRootBridge (
   //
   // Make sure Mem and MemAbove4G apertures are valid
   //
-  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
+  if (RESOURCE_VALID (Bridge->Mem)) {
     ASSERT (Bridge->Mem.Limit < SIZE_4GB);
     if (Bridge->Mem.Limit >= SIZE_4GB) {
       return NULL;
     }
   }
-  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
+  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
     ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
     if (Bridge->MemAbove4G.Base < SIZE_4GB) {
       return NULL;
     }
   }
-  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
+  if (RESOURCE_VALID (Bridge->PMem)) {
     ASSERT (Bridge->PMem.Limit < SIZE_4GB);
     if (Bridge->PMem.Limit >= SIZE_4GB) {
       return NULL;
     }
   }
-  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
+  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
     ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
     if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
       return NULL;
@@ -157,11 +159,9 @@ CreateRootBridge (
       // support separate windows for Non-prefetchable and Prefetchable
       // memory.
       //
-      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
-      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
-      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
-          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
-          ) {
+      ASSERT (!RESOURCE_VALID (Bridge->PMem));
+      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
         return NULL;
       }
     }
@@ -171,11 +171,9 @@ CreateRootBridge (
       // If this bit is not set, then the PCI Root Bridge does not support
       // 64 bit memory windows.
       //
-      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
-      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
-      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
-          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
-          ) {
+      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
+      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
         return NULL;
       }
     }
-- 
2.16.1.windows.1

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Laszlo Ersek]

On 09/21/18 09:25, Ruiyu Ni wrote:
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> ---
>  .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 ++++++++++------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> index f6234b5d11..916709e276 100644
> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
>  
>  #define NO_MAPPING  (VOID *) (UINTN) -1
>  
> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
> +
>  //
>  // Lookup table for increment values based on transfer widths
>  //
> @@ -122,25 +124,25 @@ CreateRootBridge (
>    //
>    // Make sure Mem and MemAbove4G apertures are valid
>    //
> -  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
> +  if (RESOURCE_VALID (Bridge->Mem)) {
>      ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>      if (Bridge->Mem.Limit >= SIZE_4GB) {
>        return NULL;
>      }
>    }
> -  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
> +  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>      ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>      if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>        return NULL;
>      }
>    }
> -  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
> +  if (RESOURCE_VALID (Bridge->PMem)) {
>      ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>      if (Bridge->PMem.Limit >= SIZE_4GB) {
>        return NULL;
>      }
>    }
> -  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
> +  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>      ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>      if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>        return NULL;
> @@ -157,11 +159,9 @@ CreateRootBridge (
>        // support separate windows for Non-prefetchable and Prefetchable
>        // memory.
>        //
> -      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
> -      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
> -          ) {
> +      ASSERT (!RESOURCE_VALID (Bridge->PMem));
> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
> +      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
>          return NULL;
>        }
>      }
> @@ -171,11 +171,9 @@ CreateRootBridge (
>        // If this bit is not set, then the PCI Root Bridge does not support
>        // 64 bit memory windows.
>        //
> -      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
> -      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
> -          ) {
> +      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
> +      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
>          return NULL;
>        }
>      }
> 

Two superficial comments:

- edk2 prefers long parameter names, so I suggest replacing "R" in the
macro definition with "Resource"

- taking the parameter as a pointer is frequently considered more flexible.

#define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
...
if (RESOURCE_VALID (&Bridge->Mem)) {
...

Up to you -- if you like these, feel free to update the patch before
pushing it (from my side anyway; you do need MdeModulePkg maintainer
review as well).

With or without changes:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Ni, Ruiyu]

On 9/21/2018 7:12 PM, Laszlo Ersek wrote:
> On 09/21/18 09:25, Ruiyu Ni wrote:
> 
> Two superficial comments:
> 
> - edk2 prefers long parameter names, so I suggest replacing "R" in the
> macro definition with "Resource"
> 
> - taking the parameter as a pointer is frequently considered more flexible.
> 
> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
> ...
> if (RESOURCE_VALID (&Bridge->Mem)) {
> ...
> 
> Up to you -- if you like these, feel free to update the patch before
> pushing it (from my side anyway; you do need MdeModulePkg maintainer
> review as well).

Good comments. I will update the patch then commit.

> 
> With or without changes:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Zeng, Star]

On 2018/9/21 19:12, Laszlo Ersek wrote:
> On 09/21/18 09:25, Ruiyu Ni wrote:
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Star Zeng <star.zeng@intel.com>
>> ---
>>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 ++++++++++------------
>>   1 file changed, 12 insertions(+), 14 deletions(-)
>>
>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> index f6234b5d11..916709e276 100644
>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
>>   
>>   #define NO_MAPPING  (VOID *) (UINTN) -1
>>   
>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>> +
>>   //
>>   // Lookup table for increment values based on transfer widths
>>   //
>> @@ -122,25 +124,25 @@ CreateRootBridge (
>>     //
>>     // Make sure Mem and MemAbove4G apertures are valid
>>     //
>> -  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>> +  if (RESOURCE_VALID (Bridge->Mem)) {
>>       ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>       if (Bridge->Mem.Limit >= SIZE_4GB) {
>>         return NULL;
>>       }
>>     }
>> -  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>> +  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>>       ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>>       if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>>         return NULL;
>>       }
>>     }
>> -  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>> +  if (RESOURCE_VALID (Bridge->PMem)) {
>>       ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>       if (Bridge->PMem.Limit >= SIZE_4GB) {
>>         return NULL;
>>       }
>>     }
>> -  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>> +  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>       ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>>       if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>>         return NULL;
>> @@ -157,11 +159,9 @@ CreateRootBridge (
>>         // support separate windows for Non-prefetchable and Prefetchable
>>         // memory.
>>         //
>> -      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>> -      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>> -          ) {
>> +      ASSERT (!RESOURCE_VALID (Bridge->PMem));
>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>> +      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>           return NULL;
>>         }
>>       }
>> @@ -171,11 +171,9 @@ CreateRootBridge (
>>         // If this bit is not set, then the PCI Root Bridge does not support
>>         // 64 bit memory windows.
>>         //
>> -      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>> -      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>> -          ) {
>> +      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>> +      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>           return NULL;
>>         }
>>       }
>>
> 
> Two superficial comments:
> 
> - edk2 prefers long parameter names, so I suggest replacing "R" in the
> macro definition with "Resource"
> 
> - taking the parameter as a pointer is frequently considered more flexible.
> 
> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
> ....
> if (RESOURCE_VALID (&Bridge->Mem)) {
> ....
> 
> Up to you -- if you like these, feel free to update the patch before
> pushing it (from my side anyway; you do need MdeModulePkg maintainer
> review as well).

I have no strong preference here. Let Ray to make the choice.
I have another very small comment.
Is it better to add "#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)" 
in PciRootBridge.h?
Also move "#define NO_MAPPING  (VOID *) (UINTN) -1" into PciRootBridge.h?
And also move "extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;" into 
PciHostBridge.h?

Thanks,
Star

> 
> With or without changes:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Ni, Ruiyu]

On 9/25/2018 10:35 AM, Zeng, Star wrote:
> On 2018/9/21 19:12, Laszlo Ersek wrote:
>> On 09/21/18 09:25, Ruiyu Ni wrote:
>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>> Cc: Star Zeng <star.zeng@intel.com>
>>> ---
>>>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 
>>> ++++++++++------------
>>>   1 file changed, 12 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> index f6234b5d11..916709e276 100644
>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
>>>   #define NO_MAPPING  (VOID *) (UINTN) -1
>>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>>> +
>>>   //
>>>   // Lookup table for increment values based on transfer widths
>>>   //
>>> @@ -122,25 +124,25 @@ CreateRootBridge (
>>>     //
>>>     // Make sure Mem and MemAbove4G apertures are valid
>>>     //
>>> -  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->Mem)) {
>>>       ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>>       if (Bridge->Mem.Limit >= SIZE_4GB) {
>>>         return NULL;
>>>       }
>>>     }
>>> -  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>>>       ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>>>       if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>>>         return NULL;
>>>       }
>>>     }
>>> -  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->PMem)) {
>>>       ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>>       if (Bridge->PMem.Limit >= SIZE_4GB) {
>>>         return NULL;
>>>       }
>>>     }
>>> -  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>>> +  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>>       ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>>>       if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>>>         return NULL;
>>> @@ -157,11 +159,9 @@ CreateRootBridge (
>>>         // support separate windows for Non-prefetchable and 
>>> Prefetchable
>>>         // memory.
>>>         //
>>> -      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>> -      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>> -          ) {
>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMem));
>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>> +      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID 
>>> (Bridge->PMemAbove4G)) {
>>>           return NULL;
>>>         }
>>>       }
>>> @@ -171,11 +171,9 @@ CreateRootBridge (
>>>         // If this bit is not set, then the PCI Root Bridge does not 
>>> support
>>>         // 64 bit memory windows.
>>>         //
>>> -      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>> -      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>> -          ) {
>>> +      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>> +      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID 
>>> (Bridge->PMemAbove4G)) {
>>>           return NULL;
>>>         }
>>>       }
>>>
>>
>> Two superficial comments:
>>
>> - edk2 prefers long parameter names, so I suggest replacing "R" in the
>> macro definition with "Resource"
>>
>> - taking the parameter as a pointer is frequently considered more 
>> flexible.
>>
>> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
>> ....
>> if (RESOURCE_VALID (&Bridge->Mem)) {
>> ....
>>
>> Up to you -- if you like these, feel free to update the patch before
>> pushing it (from my side anyway; you do need MdeModulePkg maintainer
>> review as well).
> 
> I have no strong preference here. Let Ray to make the choice.
> I have another very small comment.
> Is it better to add "#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)" 
> in PciRootBridge.h?
> Also move "#define NO_MAPPING  (VOID *) (UINTN) -1" into PciRootBridge.h?
> And also move "extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;" into 
> PciHostBridge.h?

The NO_MAPPING, RESOURCE_VALID macros are only used in this C file.
I prefer to put them in PciRootBridge.h only when another C file needs 
to reference these macros.

I agree moving mIoMmuProtocol to PciHostBridge.h.
I am happy to do that in a separate patch in V2.

Agree?

> 
> Thanks,
> Star
> 
>>
>> With or without changes:
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Thanks
>> Laszlo
>>
> 


-- 
Thanks,
Ray

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Zeng, Star]

On 2018/9/25 10:47, Ni, Ruiyu wrote:
> On 9/25/2018 10:35 AM, Zeng, Star wrote:
>> On 2018/9/21 19:12, Laszlo Ersek wrote:
>>> On 09/21/18 09:25, Ruiyu Ni wrote:
>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>> ---
>>>>   .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 
>>>> ++++++++++------------
>>>>   1 file changed, 12 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c 
>>>> b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> index f6234b5d11..916709e276 100644
>>>> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
>>>> @@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
>>>>   #define NO_MAPPING  (VOID *) (UINTN) -1
>>>> +#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
>>>> +
>>>>   //
>>>>   // Lookup table for increment values based on transfer widths
>>>>   //
>>>> @@ -122,25 +124,25 @@ CreateRootBridge (
>>>>     //
>>>>     // Make sure Mem and MemAbove4G apertures are valid
>>>>     //
>>>> -  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
>>>> +  if (RESOURCE_VALID (Bridge->Mem)) {
>>>>       ASSERT (Bridge->Mem.Limit < SIZE_4GB);
>>>>       if (Bridge->Mem.Limit >= SIZE_4GB) {
>>>>         return NULL;
>>>>       }
>>>>     }
>>>> -  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
>>>> +  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
>>>>       ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
>>>>       if (Bridge->MemAbove4G.Base < SIZE_4GB) {
>>>>         return NULL;
>>>>       }
>>>>     }
>>>> -  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
>>>> +  if (RESOURCE_VALID (Bridge->PMem)) {
>>>>       ASSERT (Bridge->PMem.Limit < SIZE_4GB);
>>>>       if (Bridge->PMem.Limit >= SIZE_4GB) {
>>>>         return NULL;
>>>>       }
>>>>     }
>>>> -  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
>>>> +  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
>>>>       ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
>>>>       if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
>>>>         return NULL;
>>>> @@ -157,11 +159,9 @@ CreateRootBridge (
>>>>         // support separate windows for Non-prefetchable and 
>>>> Prefetchable
>>>>         // memory.
>>>>         //
>>>> -      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
>>>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>>> -      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
>>>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>>> -          ) {
>>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMem));
>>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>>> +      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID 
>>>> (Bridge->PMemAbove4G)) {
>>>>           return NULL;
>>>>         }
>>>>       }
>>>> @@ -171,11 +171,9 @@ CreateRootBridge (
>>>>         // If this bit is not set, then the PCI Root Bridge does not 
>>>> support
>>>>         // 64 bit memory windows.
>>>>         //
>>>> -      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
>>>> -      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
>>>> -      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
>>>> -          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
>>>> -          ) {
>>>> +      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
>>>> +      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
>>>> +      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID 
>>>> (Bridge->PMemAbove4G)) {
>>>>           return NULL;
>>>>         }
>>>>       }
>>>>
>>>
>>> Two superficial comments:
>>>
>>> - edk2 prefers long parameter names, so I suggest replacing "R" in the
>>> macro definition with "Resource"
>>>
>>> - taking the parameter as a pointer is frequently considered more 
>>> flexible.
>>>
>>> #define RESOURCE_VALID(Resource) ((Resource)->Base <= (Resource)->Limit)
>>> ....
>>> if (RESOURCE_VALID (&Bridge->Mem)) {
>>> ....
>>>
>>> Up to you -- if you like these, feel free to update the patch before
>>> pushing it (from my side anyway; you do need MdeModulePkg maintainer
>>> review as well).
>>
>> I have no strong preference here. Let Ray to make the choice.
>> I have another very small comment.
>> Is it better to add "#define RESOURCE_VALID(R) ((R).Base <= 
>> (R).Limit)" in PciRootBridge.h?
>> Also move "#define NO_MAPPING  (VOID *) (UINTN) -1" into PciRootBridge.h?
>> And also move "extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;" 
>> into PciHostBridge.h?
> 
> The NO_MAPPING, RESOURCE_VALID macros are only used in this C file.
> I prefer to put them in PciRootBridge.h only when another C file needs 
> to reference these macros.

But then there will be a little inconsistent, for example OPERATION_TYPE 
is only used by PciRootBridgeIo.c.


> 
> I agree moving mIoMmuProtocol to PciHostBridge.h.
> I am happy to do that in a separate patch in V2.

Thanks. If we will only move mIoMmuProtocol, it can be in a separated patch.


Star

> 
> Agree?
> 
>>
>> Thanks,
>> Star
>>
>>>
>>> With or without changes:
>>>
>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>
>>> Thanks
>>> Laszlo
>>>
>>
> 
> 

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Ni, Ruiyu]

On 9/25/2018 11:13 AM, Zeng, Star wrote:
> On 2018/9/25 10:47, Ni, Ruiyu wrote:
> 
> But then there will be a little inconsistent, for example OPERATION_TYPE 
> is only used by PciRootBridgeIo.c.

Since coding style document doesn't define clear rule for this (I also 
don't like a coding style document with so many detailed restrictions. 
This removes the fun from coding.), I agree there is inconsistency.

> 
> 
>>
>> I agree moving mIoMmuProtocol to PciHostBridge.h.
>> I am happy to do that in a separate patch in V2.
> 
> Thanks. If we will only move mIoMmuProtocol, it can be in a separated 
> patch.
> 
> 
> Star
> 
>>
>> Agree?
>>
>>>
>>> Thanks,
>>> Star
>>>
>>>>
>>>> With or without changes:
>>>>
>>>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>
>>
>>
> 


-- 
Thanks,
Ray

Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

[Kirkendall, Garrett]

GARRETT KIRKENDALL
SMTS Firmware Engineer | CTE
7171 Southwest Parkway, Austin, TX 78735 USA 
AMD   facebook  |  amd.com

-----Original Message-----
From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Ruiyu Ni
Sent: Friday, September 21, 2018 2:26 AM
To: edk2-devel@lists.01.org
Cc: Star Zeng <star.zeng@intel.com>
Subject: [edk2] [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
---
 .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c     | 26 ++++++++++------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
index f6234b5d11..916709e276 100644
--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
@@ -21,6 +21,8 @@ extern EDKII_IOMMU_PROTOCOL        *mIoMmuProtocol;
 
 #define NO_MAPPING  (VOID *) (UINTN) -1
 
+#define RESOURCE_VALID(R) ((R).Base <= (R).Limit)
+
 //
 // Lookup table for increment values based on transfer widths  // @@ -122,25 +124,25 @@ CreateRootBridge (
   //
   // Make sure Mem and MemAbove4G apertures are valid
   //
-  if (Bridge->Mem.Base <= Bridge->Mem.Limit) {
+  if (RESOURCE_VALID (Bridge->Mem)) {
     ASSERT (Bridge->Mem.Limit < SIZE_4GB);
     if (Bridge->Mem.Limit >= SIZE_4GB) {
       return NULL;
     }
   }
-  if (Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) {
+  if (RESOURCE_VALID (Bridge->MemAbove4G)) {
     ASSERT (Bridge->MemAbove4G.Base >= SIZE_4GB);
     if (Bridge->MemAbove4G.Base < SIZE_4GB) {
       return NULL;
     }
   }
-  if (Bridge->PMem.Base <= Bridge->PMem.Limit) {
+  if (RESOURCE_VALID (Bridge->PMem)) {
     ASSERT (Bridge->PMem.Limit < SIZE_4GB);
     if (Bridge->PMem.Limit >= SIZE_4GB) {
       return NULL;
     }
   }
-  if (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit) {
+  if (RESOURCE_VALID (Bridge->PMemAbove4G)) {
     ASSERT (Bridge->PMemAbove4G.Base >= SIZE_4GB);
     if (Bridge->PMemAbove4G.Base < SIZE_4GB) {
       return NULL;
@@ -157,11 +159,9 @@ CreateRootBridge (
       // support separate windows for Non-prefetchable and Prefetchable
       // memory.
       //
-      ASSERT (Bridge->PMem.Base > Bridge->PMem.Limit);
-      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
-      if ((Bridge->PMem.Base <= Bridge->PMem.Limit) ||
-          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
-          ) {
+      ASSERT (!RESOURCE_VALID (Bridge->PMem));
+      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+      if (RESOURCE_VALID (Bridge->PMem) || RESOURCE_VALID 
+ (Bridge->PMemAbove4G)) {
         return NULL;
       }
     }
@@ -171,11 +171,9 @@ CreateRootBridge (
       // If this bit is not set, then the PCI Root Bridge does not support
       // 64 bit memory windows.
       //
-      ASSERT (Bridge->MemAbove4G.Base > Bridge->MemAbove4G.Limit);
-      ASSERT (Bridge->PMemAbove4G.Base > Bridge->PMemAbove4G.Limit);
-      if ((Bridge->MemAbove4G.Base <= Bridge->MemAbove4G.Limit) ||
-          (Bridge->PMemAbove4G.Base <= Bridge->PMemAbove4G.Limit)
-          ) {
+      ASSERT (!RESOURCE_VALID (Bridge->MemAbove4G));
+      ASSERT (!RESOURCE_VALID (Bridge->PMemAbove4G));
+      if (RESOURCE_VALID (Bridge->MemAbove4G) || RESOURCE_VALID 
+ (Bridge->PMemAbove4G)) {
         return NULL;
       }
     }
--
2.16.1.windows.1

With Laszlo's comments
Reviewed-by: Garrett Kirkendall <garrett.kirkendall@amd.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel