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.
Similarly, there are many cases where parisc has different implementations of similar functionality that isn't architecture 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".
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.
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.
Cheers, -Kevin