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