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.100; helo=mga07.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) (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 91F6421A02937 for ; Mon, 24 Sep 2018 19:39:04 -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 orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Sep 2018 19:39:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,300,1534834800"; d="scan'208";a="91604889" Received: from shzintpr03.sh.intel.com (HELO [10.253.24.48]) ([10.239.4.100]) by fmsmga004.fm.intel.com with ESMTP; 24 Sep 2018 19:35:32 -0700 To: Laszlo Ersek , Ruiyu Ni , 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> From: "Zeng, Star" Cc: star.zeng@intel.com Message-ID: <976f49ab-75e4-2009-ec78-e73a52212552@intel.com> Date: Tue, 25 Sep 2018 10:35:01 +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: <12daedaa-952f-fc73-3ed5-62970f869ba9@redhat.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:39:04 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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? Thanks, Star > > With or without changes: > > Reviewed-by: Laszlo Ersek > > Thanks > Laszlo >