From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mx.groups.io with SMTP id smtpd.web08.4456.1610517638315809568 for ; Tue, 12 Jan 2021 22:00:39 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: huawei.com, ip: 45.249.212.191, mailfrom: cenjiahui@huawei.com) Received: from DGGEMS404-HUB.china.huawei.com (unknown [172.30.72.58]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4DFxbQ1dnyzMHbL; Wed, 13 Jan 2021 13:59:14 +0800 (CST) Received: from [10.174.184.155] (10.174.184.155) by DGGEMS404-HUB.china.huawei.com (10.3.19.204) with Microsoft SMTP Server id 14.3.498.0; Wed, 13 Jan 2021 14:00:25 +0800 Subject: Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge To: Laszlo Ersek , CC: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , "Julien Grall" , Leif Lindholm , Sami Mujawar , , , "Yubo Miao" References: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-4-cenjiahui@huawei.com> <30a121d3-97b0-d8f3-6089-68ed4ca8465b@redhat.com> From: "Jiahui Cen" Message-ID: Date: Wed, 13 Jan 2021 14:00:24 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <30a121d3-97b0-d8f3-6089-68ed4ca8465b@redhat.com> X-Originating-IP: [10.174.184.155] X-CFilter-Loop: Reflected Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit Hi Laszlo, On 2021/1/13 9:28, Laszlo Ersek wrote: > On 01/12/21 10:45, Jiahui Cen via groups.io wrote: >> Extract InitRootBridge/UninitRootBridge to PciHostBridgeUtilityLib as >> common utility functions. No change of functionality. >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3059 >> >> Cc: Jordan Justen >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Anthony Perard >> Cc: Julien Grall >> Signed-off-by: Jiahui Cen >> Signed-off-by: Yubo Miao >> --- >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf | 2 - >> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf | 7 + >> OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h | 75 ++++++++++ >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridge.h | 56 ------- >> OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c | 158 +------------------- >> OvmfPkg/Library/PciHostBridgeLib/XenSupport.c | 3 +- >> OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c | 155 +++++++++++++++++++ >> 7 files changed, 243 insertions(+), 213 deletions(-) > > (1) Renaming OVMF_PCI_ROOT_BRIDGE_DEVICE_PATH to > PCI_ROOT_BRIDGE_DEVICE_PATH is not only unnecessary, it carries a risk > of conflicting with something that PciHostBridgeLib in MdeModulePkg > might introduce in the future. Please undo this rename. > > (2) Renaming "mRootBridgeDevicePathTemplate" to "mRootBridgeDevicePath" > is not necessary. Although this rename is not risky, it makes the review > (before/after comparison) of this patch more difficult than needed. > Please undo this rename. The word "template" is correct BTW, because we > modify the device path after we allocate & copy it from the template -- > see UID. > Right. I will fix them. > (3) Unfortunately, the original PciHostBridgeLib instance fails to list > its PcdLib dependency, both between the #include directives, and in the > INF file. I'm not asking you to fix that up, but please do spell out the > PcdLib #include and INF file depencency in the new > PciHostBridgeUtilityLib instance. (See the PcdGet16() call in > PciHostBridgeUtilityInitRootBridge().) > For the original PciHostBridgeLib, should I fix the dependency in a new patch? Or just in this patch? Thanks, Jiahui > > With the above small warts fixed, I'm ready to give R-b. > > Thanks > Laszlo > > . >