Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33737 )
Change subject: device/oprom: Add vbe return status support as per VBE spec 3.0 ......................................................................
Patch Set 10:
(1 comment)
@nico, would you like to pay one more look, i know you don't like to review OpRom code but still, this CL is hanging for very long time and i need to get rid of this work
I already looked at the current patch set yesterday. You didn't fix the address problem I tried to explain. If you need more help to understand the problem, I fear you have to ask another reviewer.
Did you mean do like below for video_mode_ptr as well ?
(segment << 4) + offset so in this case
(0x0000 << 4) + 0x0668 == 0x00668
Yes. Preferably before the conversion to `u32 *`.
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86_asm.S:
https://review.coreboot.org/c/coreboot/+/33737/9/src/device/oprom/realmode/x... PS9, Line 299: mov %eax, REALMODE_BASE
I'm asking when is the value stored there read? […]
I see. But why don't you make it a proper return value of the realmode_interrupt() function? Store it away, then restore it before `ret`?
I still see a conflict with __realmode_idt:
$ objdump -t build/cbfs/fallback/ramstage.debug | grep __realmode_idt 00000600 l *ABS* 00000000 __realmode_idt
It would be better to declare a proper symbol for it, how about
__realmode_ret = RELOCATED(.) .long 0