Hi Kevin,
On 2/24/21 2:23 AM, Kevin O'Connor wrote:
On Thu, Feb 11, 2021 at 11:00:34PM +0100, Helge Deller wrote:
This patchset modifies SeaBIOS source code to be able to build a firmware for the PA-RISC CPU architecture. This firmware can then be used to boot a virtual PA-RISC machine with PA-Linux and HP/UX on QEMU.
Where possible existing SeaBIOS drivers and infrastructure is reused. Since PA-RISC is native 32/64-Bit, the various segment accessors are not needed and replaced by simple variable accesses.
Thanks.
My main high-level feedback is that this patch series has too many #ifdefs in it. That is, if we want to integrate the code, we'd really need to do the work to fully integrate it. That would mean going through each case where parisc differs from x86 and coming up with alternate code that works well for both architectures. That may, for example, involve compiling different files for different architectures, using different include directives so that different headers get pulled in, and code refactoring in general. Some of this would be possible using runtime checks (eg, if (CONFIG_X86) ), but even that would need to be kept to only those code paths that are architecture specific.
Yes, agreed.
Similarly, there are many cases where parisc has different implementations of similar functionality that isn't architecture
I assume you meant "...that IS architecture specific" ?
specific (eg, malloc implementation, high-level timer implementation, a different boot menu). If the goal is integration then I think we would need to integrate - including the "warts".
Ok.
I understand that is significantly more work, but I think it would be necessary. My feedback on the patches today is that it feels like there are two notably different SeaBIOS implementations welded together with ifdefs. Unfortunately, that would effectively double the ongoing maintenance costs. I suspect seemingly innocuous changes could break one of the architectures. Or, alternatively, require twice the work for developers to make similar changes in two places. I fear developers would be unlikely to test both architectures on every change and it would be difficult to know which changes impact each architecture.
Yes, my goal was not to put additional burden on the SeaBIOS develpers, so I did not expected everyone to do an additional build check if by mistake the parisc target breaks.
Separately, on the procedural side, every incremental patch would need to be compilable and runable. This doesn't appear to be the case for this series - as an example, patch 18 pulls in a header file that isn't actually added until patch 27. It's important to order the patches into functional chucks so that "git bisect" works properly, should we encounter a regression. This is particularly important for this patch series given the magnitude of the change.
Sure.
Overall, thanks a lot for your feedback!!!
But, I'm not clear yet on how to continue. Currently SeaBIOS-parisc is a fork, and I think it's easy to still keep the fork for the time beeing. That way there will not be additional work for the developers.
What I would prefer is if we maybe could work through at least some of the patches and see if we could integrate them (where it makes sense), so that my diff to upstream-seabios can get reduced. Would that be an acceptable way forward?
If yes, I think I need clear guidance, e.g. first of all, is adding a CONFIG_X86 and CONFIG_PARISC config option (patch #1) in the Kconfig OK ?
What about simplifying the bda accessors (patch #2 and #3, but drop the parisc part there before applying). Same for the patches regarding endianess (e.g. patch #4). Trivial ones like patch #7 which adds some parisc constants? And patch #10 which adds the portaddr_t typedef?
Would you be willing to work walk through the various patches and give specific feedback?
Thanks for your help! Helge