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.web11.1145.1610697601776339228 for ; Fri, 15 Jan 2021 00:00:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XksMV5US; 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=1610697601; 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=ji+uO3T1lKsnc32GFlpm6DN4PmLDaGGfIgjKM3eQGco=; b=XksMV5USk9WNoLMNuy/tzxPiVN1jCFy+HuHwmDulaXup78i9VysTQcMKJTInUTw/cAjtw5 NnvM/AXoDwzHqvHhQ8CoFPbXaHSllMK7YGzdSQx7QEhuf8wZ5mCkynIDOKR5ccmYmiCga4 F7KqYZmciSIwIL9hMs4JUGIoMdjRcjc= 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-484-z8nwYlnbMLmaNQSuMkc66Q-1; Fri, 15 Jan 2021 02:59:59 -0500 X-MC-Unique: z8nwYlnbMLmaNQSuMkc66Q-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 03C0057201; Fri, 15 Jan 2021 07:59:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-223.ams2.redhat.com [10.36.112.223]) by smtp.corp.redhat.com (Postfix) with ESMTP id C71E962951; Fri, 15 Jan 2021 07:59:53 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges To: Jiahui Cen , devel@edk2.groups.io 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-8-cenjiahui@huawei.com> <28c4d432-f281-91dc-1b93-faf67297f181@redhat.com> <9d0979a4-f35a-4114-c8ff-cede015556f1@huawei.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 15 Jan 2021 08:59:52 +0100 MIME-Version: 1.0 In-Reply-To: <9d0979a4-f35a-4114-c8ff-cede015556f1@huawei.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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/15/21 08:25, Jiahui Cen wrote: > Hi Laszlo, > > On 2021/1/14 18:46, Laszlo Ersek wrote: >>> @@ -124,6 +132,10 @@ PciHostBridgeUtilityGetRootBridges ( >>> UINTN *Count, >>> UINT64 Attributes, >>> UINT64 AllocationAttributes, >>> + BOOLEAN DmaAbove4G, >>> + BOOLEAN NoExtendedConfigSpace, >>> + UINT32 BusMin, >>> + UINT32 BusMax, >>> PCI_ROOT_BRIDGE_APERTURE *Io, >>> PCI_ROOT_BRIDGE_APERTURE *Mem, >>> PCI_ROOT_BRIDGE_APERTURE *MemAbove4G, >> (1) You forgot to annotate the new params with IN. (Also update the C >> file please, in sync.) >> >> (2) The BusMin / BusMax addition must absolutely be a separate patch, so >> that we can discuss and review it separately. It's not a simple data >> propagation change -- it generalizes the function internally. >> >> (3) BusMax should be documented as an inclusive maximum (but see more on >> this below). > > A little bit confused. IIUC, the original hard-coded bus ranges, from 0 to > PCI_MAX_BUS, are inclusive, as PCI_MAX_BUS = 255. So in my opinion, the > addition of BusMin/BusMax simply extends the parameters, like DmaAbove4G > and NoExtendedConfigSpace, and replaces [0, PCI_MAX_BUS] with [BusMin, BusMax]. > Please correct me if I misunderstand. > > Does it really generalize the function? Yes, I think so. Regarding DmaAbove4G and NoExtendedConfigSpace, you take those in PciHostBridgeUtilityGetRootBridges(), and simply forward them to PciHostBridgeUtilityInitRootBridge(). With BusMin / BusMax exposed, you are generalizing the behavior of a loop, which is tricky, because the actual processing of the ranges is completed after the loop (with the separate PciHostBridgeUtilityInitRootBridge() call). In fact, the loop body may even run zero times (if BusMin==BusMax), and then the only bridge object is created by the call *after* the loop. All of this requires a lot of separate thinking. Before the patch, the hard coded constants are 0, 1, and PCI_MAX_BUS, and seeing their correctness is a *lot* easier than the parametrized interval limits. The best evidence for the necessity of separating out BusMin/BusMax to their own patch is that you missed updating PCI_MAX_BUS in the PciHostBridgeUtilityInitRootBridge() call after the loop. I think you failed to realize the path through the code in which the loop body would be executed zero times. While that is not possible with the original code, it is definitely possible with the new (parametrized) code, and it needs additional thought. The code seemed OK there, after all, but thinking in parameters is always more abstract (hence more difficult) than thinking in specific constants. The replacement of [0, PCI_MAX_BUS] with [BusMin, BusMax] is not trivial at all, as a concept. I had to stop and think for several minutes about your change in the following condition: if (ExtraRootBridges > BusMax - BusMin) { If you think about this BusMin/BusMax change simply as a formal update, then I believe you are missing part of the picture. I can tell you that when I originally wrote this code, I absolutely didn't think in terms of BusMin/BusMax, so this generalization is actually a kind of repurposing, and it definitely deserves its own 15 minutes of spotlight, so to speak. This is also why I requested the new sanity checks near the top of the function: (BusMin > BusMax || BusMax > PCI_MAX_BUS) --> FAIL Obviously this controlling expression evaluates to constant FALSE on the original code, which is why it doesn't *exist* in the original code (i.e. why it never occurred to me to express the condition in any shape or form). You are basically formalizing properties that have *implicitly* existed in the pre-patch code. This is a bit similar to theorem proving in mathematics (you recognize emergent properties of a construct, drag them into the sunlight, and prove them). We need to concentrate on such thought processes without being disturbed by other topics. ... Sorry about this wall of text, I really cannot express any better how big of a difference there is between (a) simple BOOLEAN parameter forwarding, and (b) turning constants that control a loop, and a post-loop coda, into caller-controlled parameters. If it's still hard to accept, please do it just because I'm asking for it. I'm not asking for it *spuriously*, I promise you that. This review is difficult for me as well, it's not in my interest to prolong it needlessly. Thanks Laszlo > > Thanks, > Jiahui >