On 07/08/16 01:38, Jordan Justen wrote: > On 2016-07-07 15:55:34, Ni, Ruiyu wrote: >> I vote for separate driver:) it's more clean and easy to maintain. >> > > (sigh) I guess that is the nature of EDK II. Big, but modular! :) It's > not like this decision is going to make OVMF compete with seabios on > size or speed,
That's never been my goal, and the edk2 development model (even the PI / UEFI standards development model) are pretty unsuitable for that. PI and UEFI are about binary compatibility between proprietary, closed source modules, from independent vendors. Therefore interfaces that would normally be "internal" and always subject to change in a self-contained (hopefully even open source) code base, are exposed and standardized in PI and UEFI for interoperability -- hence they ossify. Some of the best examples for this relate precisely to the PCI Bus driver. While we discuss, and struggle with, the separation between the PCI bus driver, the PCI host bridge driver (with the many phases and back-and-forth reporting), the EFI_PCI_HOT_PLUG_INIT_PROTOCOL, the EFI_INCOMPATIBLE_PCI_DEVICE_SUPPORT_PROTOCOL, the SeaBIOS developers dig down and modify five lines of code in the SeaBIOS tree, in the guts of their PCI enumeration algorithm. They can construct new communication channels, modify function prototypes, reorganize the algorithms, and so on. I can submit an ECR, at best. (I guess.) I'm not complaining, but modularity on this level has a price. See "Documentation/stable_api_nonsense.txt" in the kernel tree :) > so: > > Acked-by: Jordan Justen <[email protected]> Thanks. I'll drop the PcdPciBusHotplugDeviceSupport check as discussed before. > Laszlo, I disagree with you about PlatformDxe. I don't think it is an > HII driver, but rather a dumping ground for any DXE phase platform > code that we choose. Noted. For the next similar driver, if I can't find a separate example driver in MdeModulePkg, I'll attempt to squeeze the new code into PlatformDxe. (I'm already getting cold feet from the thought. :)) Thanks, Laszlo >> >>> 在 2016年7月8日，上午1:59，Laszlo Ersek <[email protected]> 写道： >>> >>>> On 07/07/16 19:36, Jordan Justen wrote: >>>>> On 2016-06-30 18:10:53, Laszlo Ersek wrote: >>>>> +EFI_STATUS >>>>> +EFIAPI >>>>> +DriverInitialize ( >>>>> + IN EFI_HANDLE ImageHandle, >>>>> + IN EFI_SYSTEM_TABLE *SystemTable >>>>> + ) >>>>> +{ >>>>> + EFI_STATUS Status; >>>>> + >>>>> + if (!FeaturePcdGet (PcdPciBusHotplugDeviceSupport)) { >>>>> + DEBUG ((EFI_D_WARN, >>>>> + "%a: this driver should not have been built into the firmware\n", >>>>> + gEfiCallerBaseName)); >>>>> + return EFI_UNSUPPORTED; >>>>> + } >>>> >>>> It looks like we'll always have this PCD as true, and always include >>>> the driver. I don't have a concern about that. (Do you know of a >>>> reason that we might need to make it conditional?) >>> >>> No, I don't expect any changes to PcdPciBusHotplugDeviceSupport. It's >>> just a warning for the case if we happen to change the PCD nonetheless. >>> >>> In that case though, the warning is not high priority enough to prevent >>> the firmware from proceeding. Hence no ASSERT() / CpuDeadLoop(). It's >>> just a message for the maintainers to clean up their DSC. >>> >>>> What if we instead add this to PlatformDxe, and either drop, or turn >>>> the PcdPciBusHotplugDeviceSupport check into an ASSERT? >>> >>> I'm perfectly fine with dropping the PcdPciBusHotplugDeviceSupport check >>> -- I was a bit torn about adding it in the first place, but I wanted to >>> solicit your feedback about it, and including the check seemed the best >>> way to do that :) >>> >>> About including the functionality in PlatformDxe: I disagree with that >>> idea. I realize this is a (standardized) PCI Bus platform tweak, but we >>> already have IncompatiblePciDeviceSupportDxe separately, which is the >>> exact same kind of (standardized) PCI Bus platform tweak. (And that >>> driver in turn exists separately because the example driver >>> "MdeModulePkg/Bus/Pci/IncompatiblePciDeviceSupportDxe" exists separately >>> as well.) >>> >>> More importantly, to me PlatformDxe is primarily a HII-centered driver. >>> I would not like to dilute that "HII focus" in it. I have had to look at >>> PlatformDxe several times in the past few months -- you surely recall >>> those iPXE HII debugging sessions on the list -- for refreshing my >>> (sadly not extensive enough) HII knowlege. Turning PlatformDxe into a >>> grab-bag of platform tweaks would harm the driver, in my opinion. >>> >>> (I think about PlatformPei a bit differently -- it *is* a grab-bag of >>> platform tweaks, but its scope is much more focused. It does low-level >>> chipset, CPU, and memory setup.) >>> >>> If you would like to avoid yet another (standardized) PCI Bus driver >>> tweak, I guess I could fuse this driver with >>> IncompatiblePciDeviceSupportDxe. If you agree with that: do you have a >>> good name for the unified driver? >>> >>> Thanks >>> Laszlo >>> _______________________________________________ >>> edk2-devel mailing list >>> [email protected] >>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
