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
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-le...@seabios.org

Reply via email to