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 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/33737/8/src/device/oprom/realmode/x... File src/device/oprom/realmode/x86.c:
https://review.coreboot.org/c/coreboot/+/33737/8/src/device/oprom/realmode/x... PS8, Line 246: mode = *video_mode_ptr++;
here is the dump of return buffer from int 10, 0x4F00 […]
Thanks for the dump, I think I understand it now. I'm not concerned about the location in memory, but more about the way the address (pointer) is represented. The VBIOS will write a 16-bit offset + 16-bit segment pointer. GCC, however, will expect a linear 32-bit pointer, afaict.
What I could have realized earlier is that we operate solely in segment 0 here. So the offset+segment pointer will match the linear pointer by coin- cidence. However, VBE also allows that the pointer points into ROM (the VBIOS code itself, see below), so we cannot make this assumption.
Maybe an example helps to understand the issue: `oem_string_ptr` in your dump is: 58 02 00 c0. These are two 16-bit numbers in little endian. The offset is 0x0258 and the segment 0xc000. The correct linear address is given by
(segment << 4) + offset
so in this case
(0xc000 << 4) + 0x0258 == 0xc0258
So this is pointing into the VBIOS ROM. You can try to dereference `oem_string_ptr` directly, I'm rather sure it won't work. Hence, the pointers inside the VbeInfoBlock have to be converted, before you can dereference them.
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 don't understand this one: 1. When is the value restored? 2. Isn't our relocated __realmode_idt (see above) at this address?
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@47 PS9, Line 47: } __packed vbe_info_t; You don't have to place it in a header file if it's going to be used only in a single C file. Also, the VBE spec calls this VbeInfoBlock, maybe `struct vbe_info_block` would be a better name and also avoid the name clash.
https://review.coreboot.org/c/coreboot/+/33737/9/src/include/vbe.h@59 PS9, Line 59: } vbe_info_t; This is also only used in `yabel/vbe.c`. I think it would be best to move both declarations into the respective C file to avoid confusion.