On 06/26/15 06:26, Ni, Ruiyu wrote: > Jordan, > Simply using PCD to split the code is very straightforward. Another > approach is to introduce a new library class with carefully defined > library APIs so that different platforms can use one common driver > plus different instances of libraries. > After all, abstracting is never a easy work.
I certainly agree that extracting a library could make this work. Also, your point about being "careful" in determining the interfaces matches my thinking. In my opinion, in such cases the library interface is almost always "incidental". That is, most of the time one cannot design the library interface *in advance*, and expect all (or future) client drivers to adopt that interface internally. Instead, I like to have two or three clients, and then try to identify the commonalities between them, and extract those. (IIRC, I said the same thing when Jordan first suggested "VirtioLib" for OvmfPkg -- I believe I agreed to the idea, but said that I would delay the librarization of common code until the code in question was *actually* common, between at least two virtio drivers. That is, I wanted to delay the extraction of VirtioLib until there were two virtio drivers.) On the other hand, my experience tells me that Jordan usually dislikes new library classes, and prefers PCDs and -- when applicable -- arch-dependent sections (and source files) in INF files. > So let's firstly make OVMF work by duplicating a PciHostBridgeDxe. > What do you think? > > Laszlo, > I don't want to block you. And I do not see a big conflict between > you and Jordan. Sure, the conflict is not between my ideas and either yours or Jordan's. The conflict is that - you would like me to copy PciHostBridgeDxe first, and extract the common stuff second, perhaps with a library class (and, I admitted up-front that I probably wouldn't have time for that, for an indefinite period) - whereas Jordan would like me to modify PciHostBridgeDxe in-place at once, clean it up in-place, and introduce all the new code I need in-place (including the probing for extra root buses, dependent on a new PcAtChipsetPkg PCD), and keep just the *setting* of the PCD under OvmfPkg. I'm willing to implement *either*, but the two of you need to reach an agreement before I start re-working the series. The current v2 series matches your idea (well at least the first step of your idea). If Jordan decides to accept your idea, then we're mostly done, I'll submit a v3 with very small updates, and tack the librarization step to the end of my queue. (And who knows when that will happen.) Otherwise, if you decide to accept Jordan's idea, then I'll rework the series for v3, but first I'd like to see you state publicly that you are fine with Jordan's idea. Otherwise, the two of you can come up together with some completely new idea as well. I can say in advance that I will accept that too. In one sentence: this series is now blocked not by either of you *individually*, but by the disagreement between you. (And I'm caught in the middle.) I don't think I'd like to wait for a resolution longer than next Friday. > Thank you very much for your continuously code > contribution. Thank you. Laszlo > > Thanks, > Ray > >> -----Original Message----- >> From: Laszlo Ersek [mailto:ler...@redhat.com] >> Sent: Friday, June 26, 2015 3:07 AM >> To: Justen, Jordan L; Ni, Ruiyu >> Cc: edk2-devel@lists.sourceforge.net; Kinney, Michael D >> Subject: Re: [edk2] [PATCH v2 02/24] PcAtChipsetPkg: remove >> PciHostBridgeDxe >> >> Jordan, Ray, >> >> On 06/25/15 19:14, Jordan Justen wrote: >>> On 2015-06-24 19:10:01, Ni, Ruiyu wrote: >>>> Jordan, >>>> I prefer to share the code across multiple platforms as well, if >>>> that's possible. >>>> >>>> In real world, DUET's PciHostBridgeDxe driver does something >>>> additionally: it gathers all option roms from PCI devices and >>>> transfers them to its own special PciBus driver to dispatch. I am >>>> not familiar to OVMF and CorebootPayloadPkg's PciHostBridgeDxe >>>> drivers. What specific behaviors do they do? >>>> >>>> Can we generalize all the special behaviors to a common driver? I do >>>> NOT like to introduce a bunch of PCDs like PcdDuet, PcdOvmf and >>>> PcdCoreboot. >>> >>> I think the names would not need to include the platform names. >>> >>> For this case, it seems like 1 or 2 PCDs would be sufficient: >>> * PcdScanForAdditionalPciRootBuses (boolean / feature PCD) >>> * PcdAdditionalRootBusesMaxBusNumber >>> >>> We could also only have 1 PCD (PcdAdditionalRootBusesMaxBusNumber) >> and >>> set it to 0 to disable the feature. >> >> here's my respectful request for the two of you: >> >> Please work out an agreement between the two of you, by the end of next >> week, Friday, July 3rd, 2015, End-of-Business, in Jordan's timezone. >> >> (Reminder: the first version of the series (with practically identical >> PciHostBridgeDxe impact) has been posted on June 6th, 19 days ago.) >> >> I have already agreed to both of your designs, but I can't satisfy both >> at once, because your requirements conflict with each other. >> >> I'm (obviously) willing to implement what Ray suggests. >> >> I'm also willing to implement Jordan's suggestion, but then I will need >> some form of *commitment* from Ray in advance. Namely, I said earlier, >> and I'm saying again, that the *overwhelming majority* of the series >> applies immediately to the driver in PcAtChipsetPkg, therefore Ray can >> review those patches right now, on a higher level, and express if he >> agrees with those patches ending up under PcAtChipsetPkg. >> >> I will be on PTO next week. If you can reach an agreement until next >> Friday, I will do my best after, to implement that shared design of yours. >> >> If the two of you can't reach an agreement until next Friday, I will >> abandon this series publicly, and we will carry it downstream only. >> >> Thanks >> Laszlo ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
