Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/33782
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
vbe.h: Convert hardcode vega mode into macro
This patch replaces vega framebuffer mode hard coded value into macro (MAX_VBE_FRAMEBUFFER_MODE).
Also make use of __packed to align data access.
Change-Id: I5e79b21acf44f6ca184a09d42acdf69c31dd4841 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/vbe.h 1 file changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33782/1
diff --git a/src/include/vbe.h b/src/include/vbe.h index 2c40d05..88888e8 100644 --- a/src/include/vbe.h +++ b/src/include/vbe.h @@ -14,6 +14,10 @@ #define VBE_H
#include <boot/coreboot_tables.h> + +/* Let's hope we never have more than 256 video modes. */ +#define MAX_VBE_FRAMEBUFFER_MODE 256 + // these structs are for input from and output to OF typedef struct { u8 display_type; // 0 = NONE, 1 = analog, 2 = digital @@ -41,10 +45,9 @@ u16 version; u8 *oem_string_ptr; u32 capabilities; - u16 video_mode_list[256]; // lets hope we never have more than - // 256 video modes... + u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE]; u16 total_memory; -} vbe_info_t; +} __packed vbe_info_t;
typedef struct { u16 mode_attributes; // 00
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33782/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33782/1//COMMIT_MSG@12 PS1, Line 12: Also make use of __packed to align data access. why is this needed and what are you trying to align?
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/33782/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33782/1//COMMIT_MSG@12 PS1, Line 12: Also make use of __packed to align data access.
why is this needed and what are you trying to align?
without __packed being specified below changes won't be able to get correct video_mode_ptr of starting video_mode_list[0] https://review.coreboot.org/c/coreboot/+/33737/6/src/device/oprom/realmode/x... line 253
info.video_mode_list[0]
Hello Aaron Durbin, Paul Menzel, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33782
to look at the new patch set (#2).
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
vbe.h: Convert hardcode vega mode into macro
This patch replaces vega framebuffer mode hard coded value into macro (MAX_VBE_FRAMEBUFFER_MODE).
Also make use of __packed in vbe_info_t structure.
Change-Id: I5e79b21acf44f6ca184a09d42acdf69c31dd4841 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/vbe.h 1 file changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33782/2
Hello Aaron Durbin, Paul Menzel, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33782
to look at the new patch set (#3).
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
vbe.h: Convert hardcode vega mode into macro
This patch replaces vega framebuffer mode hard coded value into macro (MAX_VBE_FRAMEBUFFER_MODE).
Also make use of __packed attribute in vbe_info_t structure.
Change-Id: I5e79b21acf44f6ca184a09d42acdf69c31dd4841 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/vbe.h 1 file changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33782/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33782/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33782/1//COMMIT_MSG@12 PS1, Line 12: Also make use of __packed to align data access.
without __packed being specified below changes won't be able to get correct video_mode_ptr of starti […]
i understood your comment now :) its mistake in commit msg, fixed now.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vega mode into macro ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/33782/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33782/3//COMMIT_MSG@7 PS3, Line 7: vbe.h: Convert hardcode vega mode into macro vega? Did you mean video or VGA or VESA perhaps?
Hello Aaron Durbin, Paul Menzel, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33782
to look at the new patch set (#4).
Change subject: vbe.h: Convert hardcode vesa mode into macro ......................................................................
vbe.h: Convert hardcode vesa mode into macro
This patch replaces vesa framebuffer mode hard coded value into macro (MAX_VBE_FRAMEBUFFER_MODE).
Also make use of __packed attribute in vbe_info_t structure.
Change-Id: I5e79b21acf44f6ca184a09d42acdf69c31dd4841 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/vbe.h 1 file changed, 6 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33782/4
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vesa mode into macro ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33782/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33782/3//COMMIT_MSG@7 PS3, Line 7: vbe.h: Convert hardcode vega mode into macro
vega? Did you mean video or VGA or VESA perhaps?
Thanks, its my mistake
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vesa mode into macro ......................................................................
Patch Set 4: Code-Review-1
(2 comments)
https://review.coreboot.org/#/c/33782/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/33782/4//COMMIT_MSG@12 PS4, Line 12: Also make use of __packed attribute in vbe_info_t structure. Please split this change off. This is the major change in this patch but does not look like that from the commit message.
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h@48 PS1, Line 48: u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE]; This struct does not map to the return value of the VBEINFO INT, which just returns a pointer. If you you memcpy the return value buffer into this struct, the total_memory value is not correct. It's also does seem not very portable either. I'd just decode the return buffer like done in device/oprom/yabel/vbe.c\090:vbe_info(vbe_info_t * info)
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vesa mode into macro ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h@48 PS1, Line 48: u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE];
This struct does not map to the return value of the VBEINFO INT, which just returns a pointer. […]
you are right, this structure should be like this
typedef struct { char signature[4]; u16 version; u8 *oem_string_ptr; u32 capabilities; u32 *video_mode_ptr; u16 total_memory; char reserved[236]; } __packed vbe_info_t;
I can make this code change, even i have done the same but then i have realized that device/oprom/yabel/vbe.c is kind of using info->video_mode_list[256] hence revert my changes.
I can push those changes back and modify my CL accordingly, will you be able to modify device/oprom/yabel/vbe.c accordingly as per vbe spec.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Convert hardcode vesa mode into macro ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h@48 PS1, Line 48: u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE];
you are right, this structure should be like this […]
i will make required code changes into CL https://review.coreboot.org/c/coreboot/+/33737 tomorrow.
Hello Aaron Durbin, Arthur Heymans, Paul Menzel, Stefan Reinauer, Duncan Laurie, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/33782
to look at the new patch set (#5).
Change subject: vbe.h: Make use of __packed attribute in vbe_info_t structure ......................................................................
vbe.h: Make use of __packed attribute in vbe_info_t structure
This patch adds __packed attribute so that vbe_info_t structure variables can be accessed without any additional padding.
Change-Id: I5e79b21acf44f6ca184a09d42acdf69c31dd4841 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/vbe.h 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/82/33782/5
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Make use of __packed attribute in vbe_info_t structure ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/#/c/33782/1/src/include/vbe.h@48 PS1, Line 48: u16 video_mode_list[MAX_VBE_FRAMEBUFFER_MODE];
i will make required code changes into CL https://review.coreboot.org/c/coreboot/+/33737 tomorrow.
Fixed it here https://review.coreboot.org/c/coreboot/+/33806
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Make use of __packed attribute in vbe_info_t structure ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h@42 PS7, Line 42: u8 *oem_string_ptr; unaligned
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h@43 PS7, Line 43: u32 capabilities; unaligned
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h@47 PS7, Line 47: } __packed vbe_info_t; This makes some fields unaligned which is a pain to review. And I can't find any code in the follow-up commits that requires this. Please ela- borate.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Make use of __packed attribute in vbe_info_t structure ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h@47 PS7, Line 47: } __packed vbe_info_t;
This makes some fields unaligned which is a pain to review. And I can't […]
vbe_info_t structure is read from HW through int 10 0x4f00. And why do compiler add any extra padding to make it align ? we should have parse oprom fill buffer as is. I could see compiler alignment cause issue while reading video_mode_list[0].
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Make use of __packed attribute in vbe_info_t structure ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h File src/include/vbe.h:
https://review.coreboot.org/c/coreboot/+/33782/7/src/include/vbe.h@47 PS7, Line 47: } __packed vbe_info_t;
vbe_info_t structure is read from HW through int 10 0x4f00.
No it is not. `int 10 0x4f00` fills a structure in the given buffer. And that structure is then carefully converted to `vbe_info_t`. See the comment above, it is not meant to be the same structure. `vbe_info_t` is an internal representation of the VBE info, not an interface.
And why do compiler add any extra padding to make it align ?
Because it is mandated by C. The `struct` in C is not fit for ABIs or hardware interfaces that don't follow the same alignment rules as C. You can only rely on compiler extensions like `packed`, to make it work. But then you break other assumptions of the C language. That's why I believe, `packed` should only be used by the most experienced developers. And only in extreme cases.
I could see compiler alignment cause issue while reading video_mode_list[0].
Yes. But only because you used `vbe_info_t` in a way that it was not written for. You used memcpy() to copy data in an incompatible format into it.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/33782 )
Change subject: vbe.h: Make use of __packed attribute in vbe_info_t structure ......................................................................
Abandoned
-
Arthur Heymans (Code Review) -
Kyösti Mälkki (Code Review)
-
Nico Huber (Code Review)
-
Subrata Banik (Code Review)