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.20; helo=mga02.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 EAA9D21AE30DB for ; Mon, 24 Sep 2018 19:46:56 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2018 19:46:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,300,1534834800"; d="scan'208";a="75912888" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.8]) ([10.239.9.8]) by orsmga008.jf.intel.com with ESMTP; 24 Sep 2018 19:46:51 -0700 To: "Zeng, Star" , 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> From: "Ni, Ruiyu" Message-ID: Date: Tue, 25 Sep 2018 10:47:47 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <976f49ab-75e4-2009-ec78-e73a52212552@intel.com> 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 02:46:57 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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. 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 >> >> Thanks >> Laszlo >> > -- Thanks, Ray