From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web08.1772.1610501300534504189 for ; Tue, 12 Jan 2021 17:28:21 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=eMq2f1Bh; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1610501299; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=91lVNXcGg/7A2rXSOF4xxsf6gKDTw5Xbfw7rgS8boj4=; b=eMq2f1BhHwYdeDCb1j4kkOWl16D4URMMNav2Ku0U2eB52sjMSrzZHjNOIgbmxuSHbfJ5XK 7uBb6jDVYPlfwcqTN6s9GOv/xvOcUOT+sZlkKwT6vE1kvFfTtmFt1qMWAr2NQ8GonQu0sh Q4KqagKDbONk6LpWVKyvr+UiGZiAyHI= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-412-wp8m1e-xNHGgfQh1HSvqVg-1; Tue, 12 Jan 2021 20:28:15 -0500 X-MC-Unique: wp8m1e-xNHGgfQh1HSvqVg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 9F952802B45; Wed, 13 Jan 2021 01:28:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-31.ams2.redhat.com [10.36.112.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 675346F134; Wed, 13 Jan 2021 01:28:10 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 3/9] OvmfPkg/PciHostBridgeLib: Extract InitRootBridge/UninitRootBridge To: devel@edk2.groups.io, cenjiahui@huawei.com Cc: Jordan Justen , Ard Biesheuvel , Rebecca Cran , Peter Grehan , Anthony Perard , Julien Grall , Leif Lindholm , Sami Mujawar , xieyingtai@huawei.com, wu.wubin@huawei.com, Yubo Miao References: <20210112094549.10238-1-cenjiahui@huawei.com> <20210112094549.10238-4-cenjiahui@huawei.com> From: "Laszlo Ersek" Message-ID: <30a121d3-97b0-d8f3-6089-68ed4ca8465b@redhat.com> Date: Wed, 13 Jan 2021 02:28:09 +0100 MIME-Version: 1.0 In-Reply-To: <20210112094549.10238-4-cenjiahui@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. (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().) With the above small warts fixed, I'm ready to give R-b. Thanks Laszlo