On 01/28/15 08:54, Paolo Bonzini wrote: > > > On 28/01/2015 00:29, Jordan Justen wrote: >> They may be different, but looking, I'm wondering why >> OvmfPkg/Library/QemuFwCfgLib doesn't have arm support, rather than >> putting it into a separate module over in >> ArmPlatformPkg/ArmVirtualizationPkg/Library/QemuFwCfgLib. >> >> It still may be valid to discuss whether it make sense to add to the >> QemuFwCfgLib interface, but if they are able to be merged into a >> single module, then maybe there would be less worry about code >> duplication. >> >> To throw out another idea ... If it still feels like there should be a >> separation between a library that accesses fw-cfg, and libraries than >> make use of fw-cfg data: How about something like QemuFwCfgIoLib and >> QemuFwCfgLib? > > I agree. > > I think it would make sense, long term, that something like QemuFwCfgLib > is in a new package QemuPkg, while different implementations of > QemuFwCfgIoLib are in OvmfPkg and ArmPlatformPkg/ArmVirtualizationPkg.
Just to repeat myself: part of this is actually being done already, and the other part would not be good, in my opinion. QemuFwCfgLib is already responsible for IO (modulo the small QemuFwCfgS3Enabled() API, which was indeed a mistake, but a minuscule one). QemuBootOrderLib (already committed) and QemuLoaderLib (subject of this patch) are precisely libraries that make use of fw-cfg data. They are separated from each other by functionality, and they are also linked into different drivers. The first goes into Intel's BdsDxe, via PlatformBdsLib, the second goes into AcpiTableDxe. In that sense Jordan's idea is already being implemented. On top of that, - introducing "QemuFwCfgIoLib" would amount to little else than renaming the current QemuFwCfgLib library class to QemuFwCfgIoLib, and (maybe) consolidating the three existent library instances under a common subdirectory. (The latter would IMHO come with a net negative effect -- for me it would *decrease* readability, because I already have trouble with the 2 instances that share a directory under OvmfPkg). - Fusing QemuBootOrderLib and QemuLoaderLib into a shared library class (to be called the new "QemuFwCfgLib") would lack any other organizing principle than "this stuff consumes fw-cfg". Those features are independent; they are linked into different drivers. (Note that I *did* add the recent GetFrontPageTimeoutFromQemu() function to QemuBootOrderLib (SVN r16610 to r16612), because it conceptually belongs together with the boot order stuff, and because it gets linked into BDS just the same as the boot order stuff.) - A third example would be fw_cfg -kernel booting. Currently those implementations are not shared between ArmVirtualizationPkg and OvmfPkg, hence they are not librarized -- single clients. (We've discussed that question extensively elsewhere, let's move on for now.) *If* they were shared though (ie. the synthetic filesystem-based approach), that code would either go into yet another, separate library, or (perhaps) QemuBootOrderLib. It definitely has nothing to do with ACPI. Why should BDS-related features be collected into the same library class as ACPI-related features? Just because they take the data from fw_cfg? I disagree. Thanks Laszlo ------------------------------------------------------------------------------ Dive into the World of Parallel Programming. The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
