From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.81]) by mx.groups.io with SMTP id smtpd.web10.8678.1586340948084764207 for ; Wed, 08 Apr 2020 03:15:48 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=hr6GruRA; spf=pass (domain: redhat.com, ip: 207.211.31.81, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1586340947; 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=xIioUOTwsHkDCxRrmL+qI6flIPsTS3sGK1gLqa4AZJM=; b=hr6GruRAkbHhtWTr1PVxXcqJQxhxYEOXB3xqI2S/zxB6S8vYkyToz6JY5RJp2lL/+nReL2 +k73rhIQ0N+p0RGqwjQQPy02jjUpfQAXVlY9N3DQ23vbmEqKpUOvKk406GKlSvXqV0luqu fHojdn7lgByOJDURFSgVSDtA3vQYiVE= 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-351-CvdxHFNfMSmfNYWt9yUKhw-1; Wed, 08 Apr 2020 06:15:43 -0400 X-MC-Unique: CvdxHFNfMSmfNYWt9yUKhw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 32468800D50; Wed, 8 Apr 2020 10:15:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-122.ams2.redhat.com [10.36.114.122]) by smtp.corp.redhat.com (Postfix) with ESMTP id C058D1001281; Wed, 8 Apr 2020 10:15:40 +0000 (UTC) Subject: Re: [PATCH] ArmVirtPkg/FdtPciHostBridgeLib: add extra roots for Arm. To: miaoyubo , "ard.biesheuvel@linaro.org" , "leif@nuviainc.com" Cc: "devel@edk2.groups.io" , Xiexiangyou References: <20200402120803.1482-1-miaoyubo@huawei.com> <62080872605a4a36a9df99313350b1f2@huawei.com> From: "Laszlo Ersek" Message-ID: <57fd0043-d63a-55b0-9c55-4ca079331885@redhat.com> Date: Wed, 8 Apr 2020 12:15:39 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <62080872605a4a36a9df99313350b1f2@huawei.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 04/08/20 05:58, miaoyubo wrote: >>> (2) This code is way too large for my taste to duplicate between >>> ArmVirtPkg and OvmfPkg. I would strongly prefer if we could factor the >>> logic in OvmfPkg out to a separate library, and use that from both >> consumer sites. >>> > > Where should I put the extracted logic?(Still put in OvmfPkg and direct use it in ArmVirtPkg or > put them in MdeModulePkg/Include/Library/PciHostBridgeLib.h? If in PciHostBridgeLib.h, > should I create a new .c file as a common implementation?) > There are some other codes duplicated between FdtPciHostBridgeLib.c and PciHostBridgeLib.c, > should I extract them? Good questions, I've kind of expected them. Thanks for asking them. There are at least two ways to approach this. * One would be to move the ArmVirtPkg/Library/FdtPciHostBridgeLib/ stuff under OvmfPkg/Library/PciHostBridgeLib/, have two INF files in the same directory, and separate out exactly those bits of functionality that are shared, using module-internal header files. This is usually a good thing if much of the logic is shared. In this case, I don't like it however, as the ArmVirtPkg lib instance INF file itself lists some dependencies that come from "ArmPkg.dec" and "ArmVirtPkg.dec": - PciPcdProducerLib class - PcdPciMmio32Translation, PcdPciMmio64Translation, PcdPciIoTranslation - gFdtClientProtocolGuid So we'd either have to move those into OvmfPkg, or else make OvmfPkg content depend on ArmVirtPkg.dec / ArmPkg.dec. None of those are good ideas. * Therefore, please go the more winding route, and introduce a new lib class, and lib instance, under OvmfPkg. For the lib class name, I suggest "PciHostBridgeUtilityLib". Files: OvmfPkg/OvmfPkg.dec OvmfPkg/Include/Library/PciHostBridgeUtilityLib.h OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.inf OvmfPkg/Library/PciHostBridgeUtilityLib/PciHostBridgeUtilityLib.c Then both OvmfPkg's and ArmVirtPkg's PciHostBridgeLib instances can consume this. I'd prefer four patches: - #1: OvmfPkg patch. Introduce the new lib class and lib instance, with functionality that the ArmVirtPkg and OvmfPkg PciHostBridgeLib instances already share. Regarding the OvmfPkg/Library/PciHostBridgeLib instace, you can at once move code from that lib instance to the new OvmfPkg/Library/PciHostBridgeUtilityLib, in this patch. - #2: ArmVirtPkg patch. Elminate the currently duplicated code, and put the newly extracted code to use, through consuming the new lib class. - #3: OvmfPkg patch. Move more code from PciHostBridgeLib to PciHostBridgeUtilityLib: code that you will need to use in the next patch, for enabling multiple root bridges in ArmVirtPkg. -#4: ArmVirtPkg patch. Consume the code exposed in patch#3 for enabling the feature. In theory, the code extraction / movement described in patches #1 and #3 could be done in a single unified step, at the start of the series. However, we certainly want to keep #2 and #4 separate (#2 is refactoring, #4 is feature enablement). And then I prefer keeping #1 and #3 separate too -- while both move code from the same old lib instance to the same new lib instance, their motivations are different. The ultimate goal is of course just the one "share as much code as possible between ArmVirtPkg and OvmfPkg", but, when I run "git-blame" on the new lib class header, I'd like to understand why exactly any given function prototype was declared there. Thanks! Laszlo