On 04/11/16 15:43, Ard Biesheuvel wrote:
> On 11 April 2016 at 15:34, Laszlo Ersek <[email protected]> wrote:
>> On 04/11/16 14:17, Ard Biesheuvel wrote:
>>> On 8 April 2016 at 21:15, Laszlo Ersek <[email protected]> wrote:
>>
>> [snip]
>>
>>>> I think this patch is not the right approach.
>>>>
>>>> I would like to preserve the ability to write a DXE_DRIVER that accesses
>>>> PCI config space through this PciExpressLib instance, regardless of its
>>>> own dispatch order against PciHostBridgeDxe.
>>>>
>>>> (Of course, given that PCI may not be available at all in the guest,
>>>> such a DXE_DRIVER will have to check PCI presence explicitly in its
>>>> entry point function, similarly to how PciHostBridgeDxe looks at
>>>> PcdPciExpressBaseAddress at startup.)
>>>>
>>>> Therefore, I suggest the following:
>>>>
>>>> - In our DSC files, set PcdPciExpressBaseAddress to MAX_UINT64. This
>>>> value is never a valid MMCONFIG base address. It shall mean that
>>>> PcdPciExpressBaseAddress has not been detected yet.
>>>>
>>>> We should continue to interpret the value 0 as "detected but
>>>> unavailable".
>>>>
>>>> Any other value means "detected and available".
>>>>
>>>> - Please keep the structure of this library instance (constructor etc).
>>>> In the constructor, if the value of PcdPciExpressBaseAddress is not
>>>> MAX_UINT64, then assign it to mPciExpressBaseAddress, and return
>>>> immediately with success.
>>>>
>>>> Otherwise, parse the DTB as minimally as possible, using the client
>>>> protocol (-> depex needed), just to determine the value of
>>>> PcdPciExpressBaseAddress. If PCI is present, set the PCD to
>>>> the (nonzero) base address. If PCI is absent (or there is some other
>>>> parse error), set the PCD to zero. Either way, set
>>>> mPciExpressBaseAddress to the same value, and then return with
>>>> success.
>>>>
>>>> For the next patch (i.e., PciHostBridgeDxe):
>>>>
>>>> - The PCI presence check should be preserved in PciHostBridgeDxe, in
>>>> the entry point function (PCD-based). This would (see above) consume
>>>> the product of our one library instance that is linked into
>>>> PciHostBridgeDxe itself, *or* into one of those future DXE_DRIVERs
>>>> that I mentioned above. Because PciHostBridgeDxe itself links against
>>>> the library, the PCD will never be MAX_UINT64, when the driver entry
>>>> point checks it.
>>>>
>>>> - If that check passes, then PciHostBridgeDxe should implement its own,
>>>> more complete FDT traversal (using the FDT client protocol), in order
>>>> to find all the other characteristics of the pci-host node that it
>>>> needs for operating.
>>>>
>>>> - Going forward, the PCDs (from ArmPkg) that are currently used for
>>>> communicating these "other characteristics" from VirtFdtDxe to
>>>> PciHostBridgeDxe should be eliminated from ArmVirtPkg.
>>>>
>>>> - Drop everything else too that becomes unused (to state the obvious).
>>>>
>>>> Do you agree?
>>>>
>>>
>>> Yes, that makes sense. I coded up the first part, which is
>>> straightforward enough, but for the second part, I think it is better
>>> to deal with the MAX_UINT64 case in the code as well. This way, the
>>> module and the library are independent, and we don't ignore the 'reg'
>>> property in the module, which is a bit strange as well considering
>>> that we do consume the rest of the DT node,
>>
>> I disagree.
>>
>> There are several points in your one paragraph above, so I'll have to
>> elaborate :) (I didn't want to fragment your paragraph into individual
>> sentences.)
>>
>> Long version
>> ------------
>>
>> What you are saying (if I understand it correctly) means that any
>> DXE_DRIVER that needs PCI config space access would have to prepare
>> itself for an as-yet uninitialized PCD, and perform the initialization
>> (= the parsing of "reg") itself, explicitly.
>>
>> I think this is wrong. I think my point may not have been clear enough:
>>
>>>> I would like to preserve the ability to write a DXE_DRIVER that
>>>> accesses PCI config space through this PciExpressLib instance,
>>>> regardless of its own dispatch order against PciHostBridgeDxe.
>>
>> (Or maybe it was clear, and it's just me not understanding your point.)
>>
>
> No, I did not get the PCI config space nuance here.
Okay, understood.
> I also think that
> is highly unlikely (and a worse layering violation than the one we are
> dealing with) to enumerate PCI host bridges dynamically via DT and
> subsequently wire up a driver to a fixed B/D/F.
The point is exactly that it's not "subsequent".
Really, it is dictated by the PI and UEFI abstractions (and to some
extent the edk2 code base):
- The PciIo protocol interfaces come from the generic PCI bus driver,
which is a UEFI_DRIVER, conforming to the UEFI driver model. Which
implies that drivers that consume PciIo protocol interfaces:
- have to be UEFI drivers themselves (conforming to the UEFI driver
model),
- or must register protocol notify callbacks for PciIo (and then
their behavior would depend on the platform BDS policy, which
decides about what UEFI drivers connect what devices)
- Whereas the PI spec dictates, for some protocols, to be produced by
DXE_(RUNTIME_)DRIVERs, way before BDS is entered (and PciIo instances
are produced). If these protocols are implementable by accessing the
PCI config space of a (few) fixed B/D/F(s) on a given platform, then
those accesses will occur before the PCI bus driver is dispatched (as
part of BDS).
> But I don't feel strongly about this at all, to be honest. The only
> thing I will do is add an assertion that there is only a single
> 'pci-ecam-generic' DT node,
Add it where?
And add it on top of what else, or instead of what?
> since having multiple will break lots of
> other stuff anyway, and it also validates the assumption that the
> contents of PcdPciExpressBaseAddress correspond with the DT node we
> are dealing with.
Hmm. I assume you mean you'll add the assert in the entry point of
PciHostBridgeDxe?
I agree that >1 should be asserted against, but ==1 is not guaranteed.
==0 should be handled dynamically.
Do you have a fresh v3-wip branch somewhere? I'd like to try expressing
my proposal for patches #13 and #14 in code. (If you are okay with that.)
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
