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

["Zeng, Star" <star.zeng@intel.com>]

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