On Tue, Feb 27, 2024 at 12:05:29AM +0000, Riku Viitanen wrote:
Thanks. We can certainly add support for "mxm" to seabios. It seems odd to me that this patch modifies the "generic" optionrom code. Is there a reason the code is checking for "mxm-30-sis" from optionsroms.c instead of from the existing vgahook_setup() code in vgahooks.c?
It seemed (to me) like the most logical place to put it, since it's right after a few other "config settings that impact VGA".
Similarly, is there a reason patch 1 modifies the generic src/romfile.c code to add a new romfile_loadfile_low() instead of having mxm_setup() call romfile_loadfile(), malloc_low(), and memcpy()?
Seems to me like adding extra steps for no reason as far as I know. In v1, I had a slightly different solution, and Gerd Hoffmann suggested adding a loadfile_romfile_low() instead.
I understand, but I think we should try to collect all the changes for this new code in vgahooks.c. I think we want to avoid adding device specific complexity to the generic code (optionroms.c and romfile.c).
I fear if we add device specific code to the generic code it increases the chance of a regression (eg, a binary size increase for users that have disabled CONFIG_VGAHOOKS), and increases the long-term maintenance burden (eg, if we had added via, intel, and sis hooks directly to optionroms.c then that file would become difficult to understand).
If it is reasonable to place all the "mxm" specific changes in a new section within vgahooks.c then it is easier to review, easier for other developers to compartmentalize, and easier to have confidence that it does not introduce errors for non "mxm" users.
Cheers, -Kevin