| Message ID | 1460108711-12122-14-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 04/08/16 11:45, Ard Biesheuvel wrote: > Instead of using a constructor, which may reference a dynamic PCD which is > set by the DXE entry point of its user, defer the assignment of the global > mPciExpressBaseAddress until the first the library is actually invoked. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf | 1 - > ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | 16 ++++------------ > 2 files changed, 4 insertions(+), 13 deletions(-) > > diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > index f6a346d49f22..5374f1b9a369 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf > @@ -23,7 +23,6 @@ [Defines] > MODULE_TYPE = BASE > VERSION_STRING = 1.0 > LIBRARY_CLASS = PciExpressLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION > - CONSTRUCTOR = PciExpressLibInitialize > > # > # VALID_ARCHITECTURES = ARM AARCH64 > diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > index 6479f53b3714..0ce6d118483b 100644 > --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c > @@ -68,18 +68,7 @@ PciExpressRegisterForRuntimeAccess ( > return RETURN_UNSUPPORTED; > } > > -STATIC UINT64 mPciExpressBaseAddress; > - > -RETURN_STATUS > -EFIAPI > -PciExpressLibInitialize ( > - VOID > - ) > -{ > - mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); > - return RETURN_SUCCESS; > -} > - > +STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64; > > /** > Gets the base address of PCI Express. > @@ -92,6 +81,9 @@ GetPciExpressBaseAddress ( > VOID > ) > { > + if (mPciExpressBaseAddress == MAX_UINT64) { > + mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); > + } > return (VOID*)(UINTN) mPciExpressBaseAddress; > } > > 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? Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/08/16 11:45, Ard Biesheuvel wrote: >> Instead of using a constructor, which may reference a dynamic PCD which is >> set by the DXE entry point of its user, defer the assignment of the global >> mPciExpressBaseAddress until the first the library is actually invoked. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf | 1 - >> ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | 16 ++++------------ >> 2 files changed, 4 insertions(+), 13 deletions(-) >> >> diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf >> index f6a346d49f22..5374f1b9a369 100644 >> --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf >> +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf >> @@ -23,7 +23,6 @@ [Defines] >> MODULE_TYPE = BASE >> VERSION_STRING = 1.0 >> LIBRARY_CLASS = PciExpressLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION >> - CONSTRUCTOR = PciExpressLibInitialize >> >> # >> # VALID_ARCHITECTURES = ARM AARCH64 >> diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c >> index 6479f53b3714..0ce6d118483b 100644 >> --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c >> +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c >> @@ -68,18 +68,7 @@ PciExpressRegisterForRuntimeAccess ( >> return RETURN_UNSUPPORTED; >> } >> >> -STATIC UINT64 mPciExpressBaseAddress; >> - >> -RETURN_STATUS >> -EFIAPI >> -PciExpressLibInitialize ( >> - VOID >> - ) >> -{ >> - mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); >> - return RETURN_SUCCESS; >> -} >> - >> +STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64; >> >> /** >> Gets the base address of PCI Express. >> @@ -92,6 +81,9 @@ GetPciExpressBaseAddress ( >> VOID >> ) >> { >> + if (mPciExpressBaseAddress == MAX_UINT64) { >> + mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); >> + } >> return (VOID*)(UINTN) mPciExpressBaseAddress; >> } >> >> > > 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, Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/11/16 14:17, Ard Biesheuvel wrote: > On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> 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.) The idea is this: assume for the sake of argument that the "virt" machine type grows a special (board-specific) PCI B/D/F that is necessary for implementing a given PI or UEFI protocol, in a DXE_DRIVER module, without relying on the PCI bus driver and/or PciHostBridgeDxe. A good example for this, on x86 / OVMF, is the SmmControl2Dxe driver: - It implements EFI_SMM_CONTROL2_PROTOCOL, in a DXE_RUNTIME_DRIVER. - It accesses the config space registers of some fixed location PCI devices directly with PciLib. (Those B/D/F constants come from Intel's ICH9 / Q35 specs.) - Its entry point function can run before or after the entry point function of PciHostBridgeDxe. This kind of activity is valid for a DXE_(RUNTIME_)DRIVER, and I would like to keep it available to ArmVirtPkg. Now, the problem we have here is that PCI(e) is entirely optional in the "virt" machtype of today, whereas the PciLib / PciExpressLib / PciSegmentLib classes all assume that PCI(e) is available unconditionally (and the only variable is "where"). So, these library classes do not expose a function or variable (or a boolean PCD for that matter) that allows client modules to interrogate the presense of PCI. This is the reason why we have to commit a layering violation in all our PCI-consuming DXE_DRIVER modules, and abuse PcdPciExpressBaseAddress to see if PCI(e) is available at all. (Normally a DXE_DRIVER that consumes PciLib / PciExpressLib / PciSegmentLib has zero business looking at this PCD at all!) But, I would like to keep this layering violation as minimal as possible, and keep the detection of the MMCONFIG base address outside of the client modules. Which is why I disagree with "deal[ing] with the MAX_UINT64 case in the [client] code as well". Namely, if a DXE_DRIVER like I described above does come along sometime later, we will have to duplicate the handling of MAX_UINT64 in it (the parsing of "reg"). IOW, both drivers would have to be explicitly aware of the question, "am I the first guy here that wants PCIe config space access?" In the current (un-refactored) code, the "reg" property controls the ConfigBase setting. This property is different from the other properties because: - All DXE_DRIVER modules that I describe above (that is, clients of PciLib / PciExpressLib / PciSegmentLib) need to rely on it. (Indirectly of course, i.e., through the library instance). - Simultaneously, "reg" is the only such property. Namely, "bus-range" is irrelevant for a DXE_DRIVER that wants config space access to a predefined B/D/F location. And "ranges" is entirely irrelevant for config space. Its entries describe IO and MMIO ranges, hence "ranges" falls outside of the domain of PciLib / PciExpressLib / PciSegmentLib. Thus, "bus-range" and "ranges" are relevant only to PciHostBridgeDxe. This is why I suggest to: - handle the "reg" property in the library instance, - handle everything except the "reg" property in PciHostBridgeDxe. I don't think that an argument like "we consume the rest of the DT node in PciHostBridgeDxe anyway" is very relevant here. After all, we handle the entire device tree in VirtFdtDxe at the moment, and your primary goal is to split that up along functional lines. Well, speaking in UEFI and edk2 terms, such a functional line happens to be drawn through the "pci-host-ecam-generic" node of the DT, between the "reg" property, and other properties. Short version ------------- - The PciLib & co. classes allow a client module to access PCI config space. Enumeration, discovery, valid ranges etc. are not their goal. Instead, they should enable a client module that knows exact B/D/F locations (from hardware specs) to perform such config space accesses. - Our PciExpressLib instance should support this goal as well, regardless of which of its client modules is dispatched first. Which implies that the "reg" property should be transparently parsed in the library constructor, so the PciLib & co interfaces just work. - This parsing is expensive (even if it means just the one "reg" property). Therefore we should cache the parsed value. The common solution for caching the MMCONFIG base address is to save it in PcdPciExpressBaseAddress. - Everything else (the "ranges" and the "bus-range" properties) are unrelated to the PciLib & co. classes, hence they should be handled by PciHostBridgeDxe separately, explicitly. - The PciLib & co. classes don't allow a client module to ask "is PCI(e) available at all?" In order to cope with this, we need to violate the PciLib & co. abstraction a bit, but only as little as necessary. Our current way is to look at PcdPciExpressBaseAddress in the entry point of each such client module. This makes the "availability question" overlap the "caching question". If you want, we can introduce a new dynamic boolean PCD for the availability question separately. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 April 2016 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/11/16 14:17, Ard Biesheuvel wrote: >> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> 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. 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. 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, 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. Thanks, Ard. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/11/16 15:43, Ard Biesheuvel wrote: > On 11 April 2016 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote: >> On 04/11/16 14:17, Ard Biesheuvel wrote: >>> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/11/16 15:43, Ard Biesheuvel wrote: >> On 11 April 2016 at 15:34, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 04/11/16 14:17, Ard Biesheuvel wrote: >>>> On 8 April 2016 at 21:15, Laszlo Ersek <lersek@redhat.com> 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". > I meant subsequent in Git commit log time, not in boot time. > 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. > I simply want to check that the 'pci-ecam-generic' DT node we end up consuming is the only one that exists in the device tree. In fact, I think it makes sense to assert that PcdPciExpressBaseAddress equals the DT node reg property when we encounter it. > 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.) > Sure, give me 30 mins. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/11/16 16:34, Ard Biesheuvel wrote: > On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote: >> On 04/11/16 15:43, Ard Biesheuvel wrote: [snip] > I simply want to check that the 'pci-ecam-generic' DT node we end up > consuming is the only one that exists in the device tree. In fact, I > think it makes sense to assert that PcdPciExpressBaseAddress equals > the DT node reg property when we encounter it. Yes, that definitely makes sense (at least if I understand it correctly). That is, the library constructor would do the "reg" parsing / PCD setting / caching etc (for any DXE_DRIVER that uses this instance, really), and *specifically* PciHostBridgeDxe, when it parses the same node (for those other two properties), could also look at "reg", and assert that PcdPciExpressBaseAddress is *already set* the way it expects it to be. I think that's a good idea. >> 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.) >> > > Sure, give me 30 mins. Let's do the following instead (as we discussed off-list): Please commit patches v2 1-10 (with the fixes I requested) to the master branch, and then we can collaborate, on the list, with patches for fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master branch. Cheers! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/11/16 16:34, Ard Biesheuvel wrote: >> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 04/11/16 15:43, Ard Biesheuvel wrote: > > [snip] > >> I simply want to check that the 'pci-ecam-generic' DT node we end up >> consuming is the only one that exists in the device tree. In fact, I >> think it makes sense to assert that PcdPciExpressBaseAddress equals >> the DT node reg property when we encounter it. > > Yes, that definitely makes sense (at least if I understand it > correctly). That is, the library constructor would do the "reg" parsing > / PCD setting / caching etc (for any DXE_DRIVER that uses this instance, > really), and *specifically* PciHostBridgeDxe, when it parses the same > node (for those other two properties), could also look at "reg", and > assert that PcdPciExpressBaseAddress is *already set* the way it expects > it to be. I think that's a good idea. > >>> 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.) >>> >> >> Sure, give me 30 mins. > > Let's do the following instead (as we discussed off-list): > > Please commit patches v2 1-10 (with the fixes I requested) to the master > branch, and then we can collaborate, on the list, with patches for > fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master > branch. > OK, wip branch is here git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3 Thanks _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/11/16 18:22, Ard Biesheuvel wrote: > On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote: >> On 04/11/16 16:34, Ard Biesheuvel wrote: >>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote: >>>> On 04/11/16 15:43, Ard Biesheuvel wrote: >> >> [snip] >> >>> I simply want to check that the 'pci-ecam-generic' DT node we end up >>> consuming is the only one that exists in the device tree. In fact, I >>> think it makes sense to assert that PcdPciExpressBaseAddress equals >>> the DT node reg property when we encounter it. >> >> Yes, that definitely makes sense (at least if I understand it >> correctly). That is, the library constructor would do the "reg" parsing >> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance, >> really), and *specifically* PciHostBridgeDxe, when it parses the same >> node (for those other two properties), could also look at "reg", and >> assert that PcdPciExpressBaseAddress is *already set* the way it expects >> it to be. I think that's a good idea. >> >>>> 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.) >>>> >>> >>> Sure, give me 30 mins. >> >> Let's do the following instead (as we discussed off-list): >> >> Please commit patches v2 1-10 (with the fixes I requested) to the master >> branch, and then we can collaborate, on the list, with patches for >> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master >> branch. >> > > OK, wip branch is here > git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3 Great, thank you. * Patch f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol is exactly what I had in mind. Please modify the commit message (as I pointed out previously, in the v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list of client drivers, plus update the count "three" to "four". With that, you can add my Reviewed-by: Laszlo Ersek <lersek@redhat.com> to the patch. I think it's also fine to push the first two patches to master afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol" and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node" (currently a889387fee53)). After pushing these two patches above (with my requested updates in the first one), can you please submit a smaller, separate series that focuses on the conversion of the PCI host node exclusively? Please see my comments below: * Patch 60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT client protocol is also precisely what I had in mind. - There is one typo in the commit message (with two instances): s/PciExpressBaseAddress/PcdPciExpressBaseAddress/ - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF default values in the two DSC files, saying that the MAX_UINT64 value means "undiscovered". - Furthermore, please modify the comment on PcdPciExpressBaseAddress in the INF file like this: ## CONSUMES ## SOMETIMES_PRODUCES With those tweaked: Reviewed-by: Laszlo Ersek <lersek@redhat.com> (However, this shouldn't be pushed without the rest below.) * Can you please submit a separate patch that removes PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"? * Patch 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol also looks mostly good. Some comments: - Please fix a typo in the commit message: s/host provided/host-provided/ - Also, the paragraph "This means some PCI related PCDs...", in the commit message, belongs to the next patch ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please move it over. - The ProcessPciHost() function's leading comment has become stale. Please drop it. - Please do not introduce those global variables just for returning data from ProcessPciHost() to InitializePciHostBridge(). Use locals and an appropriate signature for ProcessPciHost(). (You can collect those values in a new structure type too.) This might require you to bring back those explicit zero-assignments, before the "ranges" loop. - Since the MmioBase and MmioSize values are no longer kept in UINT32 PCDs (but UINT64 variables), the explicit (UINT64) cast is unnecessary, in InitializePciHostBridge(). - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are redundant, I think, after all of the GetNodeProperty() calls, in ProcessPciHost(). - Can you factor out the setting of the /chosen node to a separate function? (Together with the Tmp variable.) - In this (v3-wip) iteration, you also removed the PcdSetBool (PcdPciDisableBusEnumeration, FALSE); call. I almost suggested to reinstate it, but doing so wouldn't be safe actually. Namely, according to the build report, two drivers consume PcdPciDisableBusEnumeration: PciBusDxe and QemuFwCfgAcpiPlatformDxe. The PciBusDxe case is safe, because it is a UEFI driver model driver that depends on protocols produced by PciHostBridgeDxe. However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it consumes PcdPciDisableBusEnumeration first thing in its entry point. This means that you will have to introduce another plugin library (in a separate patch) just for setting PcdPciDisableBusEnumeration, and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe. Also, in this plugin library, since the PCD is just a boolean, we cannot use the caching trick (through a special value of the PCD), hence minimally the presence of "pci-host-ecam-generic" will have to be verified every time. Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/11/16 18:22, Ard Biesheuvel wrote: >> On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 04/11/16 16:34, Ard Biesheuvel wrote: >>>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> On 04/11/16 15:43, Ard Biesheuvel wrote: >>> >>> [snip] >>> >>>> I simply want to check that the 'pci-ecam-generic' DT node we end up >>>> consuming is the only one that exists in the device tree. In fact, I >>>> think it makes sense to assert that PcdPciExpressBaseAddress equals >>>> the DT node reg property when we encounter it. >>> >>> Yes, that definitely makes sense (at least if I understand it >>> correctly). That is, the library constructor would do the "reg" parsing >>> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance, >>> really), and *specifically* PciHostBridgeDxe, when it parses the same >>> node (for those other two properties), could also look at "reg", and >>> assert that PcdPciExpressBaseAddress is *already set* the way it expects >>> it to be. I think that's a good idea. >>> >>>>> 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.) >>>>> >>>> >>>> Sure, give me 30 mins. >>> >>> Let's do the following instead (as we discussed off-list): >>> >>> Please commit patches v2 1-10 (with the fixes I requested) to the master >>> branch, and then we can collaborate, on the list, with patches for >>> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master >>> branch. >>> >> >> OK, wip branch is here >> git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3 > > Great, thank you. > > * Patch > > f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol > > is exactly what I had in mind. > > Please modify the commit message (as I pointed out previously, in the > v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list > of client drivers, plus update the count "three" to "four". With that, > you can add my > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > to the patch. > > I think it's also fine to push the first two patches to master > afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol" > and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node" > (currently a889387fee53)). > > After pushing these two patches above (with my requested updates in the > first one), Done > can you please submit a smaller, separate series that > focuses on the conversion of the PCI host node exclusively? Please see > my comments below: > > * Patch > > 60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT > client protocol > > is also precisely what I had in mind. > > - There is one typo in the commit message (with two instances): > > s/PciExpressBaseAddress/PcdPciExpressBaseAddress/ > > - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF > default values in the two DSC files, saying that the MAX_UINT64 > value means "undiscovered". > > - Furthermore, please modify the comment on PcdPciExpressBaseAddress > in the INF file like this: > > ## CONSUMES ## SOMETIMES_PRODUCES > > With those tweaked: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > (However, this shouldn't be pushed without the rest below.) > OK > * Can you please submit a separate patch that removes > PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"? > Well, this patch needs to wait after the PCI references are removed from VirtFdtDxe, and it is already removed by that patch. In fact, I should probably drop PcdPciDisableBusEnumeration from ArmVirtXen.dsc in that patch as well. > * Patch > > 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol > > also looks mostly good. Some comments: > > - Please fix a typo in the commit message: > > s/host provided/host-provided/ > > - Also, the paragraph "This means some PCI related PCDs...", in the > commit message, belongs to the next patch > ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please > move it over. > > - The ProcessPciHost() function's leading comment has become stale. > Please drop it. > > - Please do not introduce those global variables just for > returning data from ProcessPciHost() to InitializePciHostBridge(). > Use locals and an appropriate signature for ProcessPciHost(). (You > can collect those values in a new structure type too.) > > This might require you to bring back those explicit > zero-assignments, before the "ranges" loop. > > - Since the MmioBase and MmioSize values are no longer kept in UINT32 > PCDs (but UINT64 variables), the explicit (UINT64) cast is > unnecessary, in InitializePciHostBridge(). > > - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are > redundant, I think, after all of the GetNodeProperty() calls, in > ProcessPciHost(). > > - Can you factor out the setting of the /chosen node to a separate > function? (Together with the Tmp variable.) > > - In this (v3-wip) iteration, you also removed the > > PcdSetBool (PcdPciDisableBusEnumeration, FALSE); > > call. I almost suggested to reinstate it, but doing so wouldn't be > safe actually. > > Namely, according to the build report, two drivers consume > PcdPciDisableBusEnumeration: PciBusDxe and > QemuFwCfgAcpiPlatformDxe. > > The PciBusDxe case is safe, because it is a UEFI driver model > driver that depends on protocols produced by PciHostBridgeDxe. > However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it > consumes PcdPciDisableBusEnumeration first thing in its entry point. > > This means that you will have to introduce another plugin library > (in a separate patch) just for setting PcdPciDisableBusEnumeration, > and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe. > > Also, in this plugin library, since the PCD is just a boolean, we > cannot use the caching trick (through a special value of the PCD), > hence minimally the presence of "pci-host-ecam-generic" will have to > be verified every time. > OK, that all looks reasonable. I will send it out as a separate series, and once we are happy with those, I will repost the remaining patches. _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote: > On 04/11/16 18:22, Ard Biesheuvel wrote: >> On 11 April 2016 at 16:50, Laszlo Ersek <lersek@redhat.com> wrote: >>> On 04/11/16 16:34, Ard Biesheuvel wrote: >>>> On 11 April 2016 at 16:12, Laszlo Ersek <lersek@redhat.com> wrote: >>>>> On 04/11/16 15:43, Ard Biesheuvel wrote: >>> >>> [snip] >>> >>>> I simply want to check that the 'pci-ecam-generic' DT node we end up >>>> consuming is the only one that exists in the device tree. In fact, I >>>> think it makes sense to assert that PcdPciExpressBaseAddress equals >>>> the DT node reg property when we encounter it. >>> >>> Yes, that definitely makes sense (at least if I understand it >>> correctly). That is, the library constructor would do the "reg" parsing >>> / PCD setting / caching etc (for any DXE_DRIVER that uses this instance, >>> really), and *specifically* PciHostBridgeDxe, when it parses the same >>> node (for those other two properties), could also look at "reg", and >>> assert that PcdPciExpressBaseAddress is *already set* the way it expects >>> it to be. I think that's a good idea. >>> >>>>> 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.) >>>>> >>>> >>>> Sure, give me 30 mins. >>> >>> Let's do the following instead (as we discussed off-list): >>> >>> Please commit patches v2 1-10 (with the fixes I requested) to the master >>> branch, and then we can collaborate, on the list, with patches for >>> fw_cfg and PciExpressLib / PciHostBridgeDxe, directly against the master >>> branch. >>> >> >> OK, wip branch is here >> git://git.linaro.org/people/ard.biesheuvel/uefi-next.git virt-fdt-refactor-v3 > > Great, thank you. > > * Patch > > f3089d080cd0 ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol > > is exactly what I had in mind. > > Please modify the commit message (as I pointed out previously, in the > v2 11/24 sub-thread here) to include "SmbiosPlatformDxe" in the list > of client drivers, plus update the count "three" to "four". With that, > you can add my > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > to the patch. > > I think it's also fine to push the first two patches to master > afterwards ("ArmVirtPkg/QemuFwCfgLib: move to FDT client protocol" > and "ArmVirtPkg/VirtFdtDxe: remove handling of fw_cfg DT node" > (currently a889387fee53)). > > After pushing these two patches above (with my requested updates in the > first one), can you please submit a smaller, separate series that > focuses on the conversion of the PCI host node exclusively? Please see > my comments below: > > * Patch > > 60471256eb10 ArmVirtPkg/BaseCachingPciExpressLib: convert to FDT > client protocol > > is also precisely what I had in mind. > > - There is one typo in the commit message (with two instances): > > s/PciExpressBaseAddress/PcdPciExpressBaseAddress/ > > - Also, please add a minimal comment above the 0xFFFFFFFFFFFFFFFF > default values in the two DSC files, saying that the MAX_UINT64 > value means "undiscovered". > > - Furthermore, please modify the comment on PcdPciExpressBaseAddress > in the INF file like this: > > ## CONSUMES ## SOMETIMES_PRODUCES > > With those tweaked: > > Reviewed-by: Laszlo Ersek <lersek@redhat.com> > > (However, this shouldn't be pushed without the rest below.) > > * Can you please submit a separate patch that removes > PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"? > > * Patch > > 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol > > also looks mostly good. Some comments: > > - Please fix a typo in the commit message: > > s/host provided/host-provided/ > > - Also, the paragraph "This means some PCI related PCDs...", in the > commit message, belongs to the next patch > ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please > move it over. > > - The ProcessPciHost() function's leading comment has become stale. > Please drop it. > > - Please do not introduce those global variables just for > returning data from ProcessPciHost() to InitializePciHostBridge(). > Use locals and an appropriate signature for ProcessPciHost(). (You > can collect those values in a new structure type too.) > > This might require you to bring back those explicit > zero-assignments, before the "ranges" loop. > > - Since the MmioBase and MmioSize values are no longer kept in UINT32 > PCDs (but UINT64 variables), the explicit (UINT64) cast is > unnecessary, in InitializePciHostBridge(). > > - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are > redundant, I think, after all of the GetNodeProperty() calls, in > ProcessPciHost(). > > - Can you factor out the setting of the /chosen node to a separate > function? (Together with the Tmp variable.) > > - In this (v3-wip) iteration, you also removed the > > PcdSetBool (PcdPciDisableBusEnumeration, FALSE); > > call. I almost suggested to reinstate it, but doing so wouldn't be > safe actually. > > Namely, according to the build report, two drivers consume > PcdPciDisableBusEnumeration: PciBusDxe and > QemuFwCfgAcpiPlatformDxe. > > The PciBusDxe case is safe, because it is a UEFI driver model > driver that depends on protocols produced by PciHostBridgeDxe. > However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it > consumes PcdPciDisableBusEnumeration first thing in its entry point. > > This means that you will have to introduce another plugin library > (in a separate patch) just for setting PcdPciDisableBusEnumeration, > and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe. > > Also, in this plugin library, since the PCD is just a boolean, we > cannot use the caching trick (through a special value of the PCD), > hence minimally the presence of "pci-host-ecam-generic" will have to > be verified every time. > Actually, looking at this again, wouldn't it make sense to have a single plugin library that sets both PcdPciDisableBusEnumeration and PcdPciExpressBaseAddress, and use it for these two modules and for the BaseCachingPciExpressLib? _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/12/16 12:10, Ard Biesheuvel wrote: > On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote: >> * Can you please submit a separate patch that removes >> PcdPciExpressBaseAddress from "ArmVirtPkg/ArmVirtXen.dsc"? >> > > Well, this patch needs to wait after the PCI references are removed > from VirtFdtDxe, and it is already removed by that patch. In fact, I > should probably drop PcdPciDisableBusEnumeration from ArmVirtXen.dsc > in that patch as well. Okay. >> * Patch >> >> 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol >> >> also looks mostly good. Some comments: > > OK, that all looks reasonable. I will send it out as a separate > series, and once we are happy with those, I will repost the remaining > patches. That's what I thought as well. Thanks! Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/12/16 12:15, Ard Biesheuvel wrote: > On 11 April 2016 at 21:14, Laszlo Ersek <lersek@redhat.com> wrote: >> * Patch >> >> 8528ac9292e4 ArmVirtPkg/PciHostBridgeDxe: move to FDT client protocol >> >> also looks mostly good. Some comments: >> >> - Please fix a typo in the commit message: >> >> s/host provided/host-provided/ >> >> - Also, the paragraph "This means some PCI related PCDs...", in the >> commit message, belongs to the next patch >> ("ArmVirtPkg/VirtFdtDxe: drop PCI host bridge handling"). Please >> move it over. >> >> - The ProcessPciHost() function's leading comment has become stale. >> Please drop it. >> >> - Please do not introduce those global variables just for >> returning data from ProcessPciHost() to InitializePciHostBridge(). >> Use locals and an appropriate signature for ProcessPciHost(). (You >> can collect those values in a new structure type too.) >> >> This might require you to bring back those explicit >> zero-assignments, before the "ranges" loop. >> >> - Since the MmioBase and MmioSize values are no longer kept in UINT32 >> PCDs (but UINT64 variables), the explicit (UINT64) cast is >> unnecessary, in InitializePciHostBridge(). >> >> - The nullity checks in (EFI_ERROR (Status) || Prop == NULL) are >> redundant, I think, after all of the GetNodeProperty() calls, in >> ProcessPciHost(). >> >> - Can you factor out the setting of the /chosen node to a separate >> function? (Together with the Tmp variable.) >> >> - In this (v3-wip) iteration, you also removed the >> >> PcdSetBool (PcdPciDisableBusEnumeration, FALSE); >> >> call. I almost suggested to reinstate it, but doing so wouldn't be >> safe actually. >> >> Namely, according to the build report, two drivers consume >> PcdPciDisableBusEnumeration: PciBusDxe and >> QemuFwCfgAcpiPlatformDxe. >> >> The PciBusDxe case is safe, because it is a UEFI driver model >> driver that depends on protocols produced by PciHostBridgeDxe. >> However, QemuFwCfgAcpiPlatformDxe is just a DXE_DRIVER, and it >> consumes PcdPciDisableBusEnumeration first thing in its entry point. >> >> This means that you will have to introduce another plugin library >> (in a separate patch) just for setting PcdPciDisableBusEnumeration, >> and link it into both PciBusDxe and QemuFwCfgAcpiPlatformDxe. >> >> Also, in this plugin library, since the PCD is just a boolean, we >> cannot use the caching trick (through a special value of the PCD), >> hence minimally the presence of "pci-host-ecam-generic" will have to >> be verified every time. >> > > Actually, looking at this again, wouldn't it make sense to have a > single plugin library that sets both PcdPciDisableBusEnumeration and > PcdPciExpressBaseAddress, and use it for these two modules and for the > BaseCachingPciExpressLib? > This could be a valid idea, but it requires some care. - BaseCachingPciExpressLib will have to spell out the dependency on this additional library class explicitly, otherwise the order of constructors would not be ensured. In other words, for clients of BaseCachingPciExpressLib, the new library instance would not be "plugged", but regularly inherited. It should also mean that the source code of the BaseCachingPciExpressLib instance doesn't change (relative to master), only the [LibraryClasses] section does, in its INF file. - For modules that need PcdPciDisableBusEnumeration, the plugin lib instance would have to remain a plugin. I'm unsure about the case when a module gets the plugin both implicitly and explicitly. - Should the condition (PcdPciExpressBaseAddress != MAX_UINT64) imply that PcdPciDisableBusEnumeration has been finalized as well? Probably so, but this should be documented in the DSC files, near the MAX_UINT64 default value. - The above co-caching saves some cycles, and we avoid yet another library class+instance. That's good. However, we should check how much potential is there for wasted work. For a client of PciExpressLib, the waste is only a single PcdSetBool(), which is negligible. For a client that needs only PcdPciDisableBusEnumeration, the waste is: a PcdSet64(), which is negligible, and the fact that we don't just look up the "pci-host-ecam-generic" node, but also grab its "reg" property (plus a SwapBytes64() call). I don't think it's bad. Clearly, the "waste" in the above cases can only be called "waste" if nothing in the firmware needs that "other" thing later. And even then, the "waste" is spent only once, due to the caching through (PcdPciExpressBaseAddress != MAX_UINT64). So, I think this is worth a shot! I wonder what a good name could be, for this new plugin lib. PciPcdProducerLib? Thanks Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 04/12/16 12:59, Laszlo Ersek wrote: > I wonder what a good name could be, for this new plugin lib. > PciPcdProducerLib? Maybe stick in a reference to "Fdt" as well... Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf index f6a346d49f22..5374f1b9a369 100644 --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf @@ -23,7 +23,6 @@ [Defines] MODULE_TYPE = BASE VERSION_STRING = 1.0 LIBRARY_CLASS = PciExpressLib|DXE_DRIVER UEFI_DRIVER UEFI_APPLICATION - CONSTRUCTOR = PciExpressLibInitialize # # VALID_ARCHITECTURES = ARM AARCH64 diff --git a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c index 6479f53b3714..0ce6d118483b 100644 --- a/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c +++ b/ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c @@ -68,18 +68,7 @@ PciExpressRegisterForRuntimeAccess ( return RETURN_UNSUPPORTED; } -STATIC UINT64 mPciExpressBaseAddress; - -RETURN_STATUS -EFIAPI -PciExpressLibInitialize ( - VOID - ) -{ - mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); - return RETURN_SUCCESS; -} - +STATIC UINT64 mPciExpressBaseAddress = MAX_UINT64; /** Gets the base address of PCI Express. @@ -92,6 +81,9 @@ GetPciExpressBaseAddress ( VOID ) { + if (mPciExpressBaseAddress == MAX_UINT64) { + mPciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress); + } return (VOID*)(UINTN) mPciExpressBaseAddress; }
Instead of using a constructor, which may reference a dynamic PCD which is set by the DXE entry point of its user, defer the assignment of the global mPciExpressBaseAddress until the first the library is actually invoked. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- ArmVirtPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf | 1 - ArmVirtPkg/Library/BaseCachingPciExpressLib/PciExpressLib.c | 16 ++++------------ 2 files changed, 4 insertions(+), 13 deletions(-) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel