Re: [edk2-devel] [PATCH v4 7/9] OvmfPkg/PciHostBridgeUtilityLib: Extend parameter list of GetRootBridges

["Jiahui Cen" <cenjiahui@huawei.com>]

Hi Laszlo,

On 2021/1/15 15:59, Laszlo Ersek wrote:
> 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.
> 

Thanks so much for detailed explanation. Now it is much clearer to me.
The addition introduces some new cases that the original hard coded
constants never meet, and makes the patch much more complex. I'm sorry
that I did not notice it before. I'll think more carefully in the future.

> 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.

I will modify the patch as what you point out. Thanks again for the review.

Thanks!
Jiahui

> 
> Thanks
> Laszlo
> 
>>
>> Thanks,
>> Jiahui
>>
> 
> .
>