From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.126; helo=mga18.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 8338321154861 for ; Mon, 24 Sep 2018 20:14:56 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga106.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2018 20:14:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,300,1534834800"; d="scan'208";a="91611578" Received: from shzintpr02.sh.intel.com (HELO [10.253.24.48]) ([10.239.4.160]) by fmsmga004.fm.intel.com with ESMTP; 24 Sep 2018 20:13:44 -0700 To: "Ni, Ruiyu" , Laszlo Ersek , edk2-devel@lists.01.org References: <20180921072539.268068-1-ruiyu.ni@intel.com> <20180921072539.268068-4-ruiyu.ni@intel.com> <12daedaa-952f-fc73-3ed5-62970f869ba9@redhat.com> <976f49ab-75e4-2009-ec78-e73a52212552@intel.com> Cc: star.zeng@intel.com From: "Zeng, Star" Message-ID: <222e8a1a-72c9-e125-66c1-05737ba6a360@intel.com> Date: Tue, 25 Sep 2018 11:13:14 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Subject: Re: [PATCH 3/3] MdeModulePkg/PciHostBridge: Add RESOURCE_VALID() to simplify code X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Sep 2018 03:14:56 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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 >>>> Cc: Star Zeng >>>> --- >>>>   .../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 >>> >>> Thanks >>> Laszlo >>> >> > >