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, so: Acked-by: Jordan Justen <[email protected]> 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. -Jordan > > > 在 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
