| Message ID | 20240328153523.4155-2-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/video: improve early video detection | expand |
On 28/03/2024 3:35 pm, Roger Pau Monne wrote: > Currently the offsets into the boot_video_info struct are manually encoded in > video.S, which is fragile. Generate them in asm-offsets.c and switch the > current code to use those instead. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> R-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks for looking at this. As you're touching these lines, there are a few style fixes. > diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S > index 0ae04f270f8c..a4b25a3b34d1 100644 > --- a/xen/arch/x86/boot/video.S > +++ b/xen/arch/x86/boot/video.S > @@ -26,32 +26,13 @@ > /* Force 400 scan lines for standard modes (hack to fix bad BIOS behaviour */ > #undef CONFIG_VIDEO_400_HACK > > -/* Positions of various video parameters passed to the kernel */ > -/* (see also include/linux/tty.h) */ > -#define PARAM_CURSOR_POS 0x00 > -#define PARAM_VIDEO_MODE 0x02 > -#define PARAM_VIDEO_COLS 0x03 > -#define PARAM_VIDEO_LINES 0x04 > -#define PARAM_HAVE_VGA 0x05 > -#define PARAM_FONT_POINTS 0x06 > -#define PARAM_CAPABILITIES 0x08 > -#define PARAM_LFB_LINELENGTH 0x0c > -#define PARAM_LFB_WIDTH 0x0e > -#define PARAM_LFB_HEIGHT 0x10 > -#define PARAM_LFB_DEPTH 0x12 > -#define PARAM_LFB_BASE 0x14 > -#define PARAM_LFB_SIZE 0x18 > -#define PARAM_LFB_COLORS 0x1c > -#define PARAM_VESAPM_SEG 0x24 > -#define PARAM_VESAPM_OFF 0x26 > -#define PARAM_VESA_ATTRIB 0x28 > #define _param(param) bootsym(boot_vid_info)+(param) > > video: xorw %ax, %ax > movw %ax, %gs # GS is zero > cld > call basic_detect # Basic adapter type testing (EGA/VGA/MDA/CGA) > - cmpb $0,_param(PARAM_HAVE_VGA) > + cmpb $0,_param(BVI_have_vga) Space > @@ -160,16 +141,16 @@ mopar_gr: > dac_set: > # set color size to DAC size > movzbw bootsym(dac_size), %ax > - movb %al, _param(PARAM_LFB_COLORS+0) > - movb %al, _param(PARAM_LFB_COLORS+2) > - movb %al, _param(PARAM_LFB_COLORS+4) > - movb %al, _param(PARAM_LFB_COLORS+6) > + movb %al, _param(BVI_lfb_colors+0) > + movb %al, _param(BVI_lfb_colors+2) > + movb %al, _param(BVI_lfb_colors+4) > + movb %al, _param(BVI_lfb_colors+6) Spaces > diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c > index d8903a3ce9c7..91da6b9d3885 100644 > --- a/xen/arch/x86/x86_64/asm-offsets.c > +++ b/xen/arch/x86/x86_64/asm-offsets.c > @@ -16,6 +16,10 @@ > #include <xen/multiboot.h> > #include <xen/multiboot2.h> > > +#ifdef CONFIG_VIDEO > +# include "../boot/video.h" > +#endif > + > #define DEFINE(_sym, _val) \ > asm volatile ( "\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\""\ > :: "i" (_val) ) > @@ -208,4 +212,26 @@ void __dummy__(void) > > OFFSET(DOMAIN_vm_assist, struct domain, vm_assist); > BLANK(); > + > +#ifdef CONFIG_VIDEO > + DEFINE(BVI_size, sizeof(struct boot_video_info)); > + OFFSET(BVI_cursor_pos, struct boot_video_info, orig_x); > + OFFSET(BVI_video_mode, struct boot_video_info, orig_video_mode); > + OFFSET(BVI_video_cols, struct boot_video_info, orig_video_cols); > + OFFSET(BVI_video_lines, struct boot_video_info, orig_video_lines); > + OFFSET(BVI_have_vga, struct boot_video_info, orig_video_isVGA); > + OFFSET(BVI_font_points, struct boot_video_info, orig_video_points); > + OFFSET(BVI_capabilities, struct boot_video_info, capabilities); > + OFFSET(BVI_lfb_linelength, struct boot_video_info, lfb_linelength); > + OFFSET(BVI_lfb_width, struct boot_video_info, lfb_width); > + OFFSET(BVI_lfb_height, struct boot_video_info, lfb_height); > + OFFSET(BVI_lfb_depth, struct boot_video_info, lfb_depth); > + OFFSET(BVI_lfb_base, struct boot_video_info, lfb_base); > + OFFSET(BVI_lfb_size, struct boot_video_info, lfb_size); > + OFFSET(BVI_lfb_colors, struct boot_video_info, colors); > + OFFSET(BVI_vesapm_seg, struct boot_video_info, vesapm.seg); > + OFFSET(BVI_vesapm_off, struct boot_video_info, vesapm.off); > + OFFSET(BVI_vesa_attrib, struct boot_video_info, vesa_attrib); > + BLANK(); > +#endif /* CONFIG_VIDEO */ BVI_size traditionally goes last. MB2 (which I guess you copied?) is a little odd. I'd like to start vertically aligning this for readability, like I started with EFRAME_*. Happy to fold of all of these tweaks on commit. ~Andrew
On 28.03.2024 16:35, Roger Pau Monne wrote: > Currently the offsets into the boot_video_info struct are manually encoded in > video.S, which is fragile. Generate them in asm-offsets.c and switch the > current code to use those instead. Just to mention it (without asking for immediate action): Defining boot_vid_info in assembly code then is as fragile. Moving to C would likely be problematic because it needs to be in the trampoline range. But at least its size should (at some point) perhaps better be tied to the C struct's sizeof(). The fields, with some effort, could also be converted using the new BVI_* constants. That would still leave the field sizes; maybe those could at least be cross-checked by some BUILD_BUG_ONs. Jan
On 02.04.2024 11:38, Jan Beulich wrote: > On 28.03.2024 16:35, Roger Pau Monne wrote: >> Currently the offsets into the boot_video_info struct are manually encoded in >> video.S, which is fragile. Generate them in asm-offsets.c and switch the >> current code to use those instead. > > Just to mention it (without asking for immediate action): Defining boot_vid_info > in assembly code then is as fragile. Moving to C would likely be problematic > because it needs to be in the trampoline range. But at least its size should (at > some point) perhaps better be tied to the C struct's sizeof(). Actually I overlooked that you partly do this. The use of BVI_capabilities there looks odd to me, though. Why not .space BVI_size - (. - boot_vid_info) ? I realize it becomes just BVI_size in patch 2, but I have some question there, too. Jan > The fields, with > some effort, could also be converted using the new BVI_* constants. That would > still leave the field sizes; maybe those could at least be cross-checked by some > BUILD_BUG_ONs. > > Jan
On Tue, Apr 02, 2024 at 11:43:49AM +0200, Jan Beulich wrote: > On 02.04.2024 11:38, Jan Beulich wrote: > > On 28.03.2024 16:35, Roger Pau Monne wrote: > >> Currently the offsets into the boot_video_info struct are manually encoded in > >> video.S, which is fragile. Generate them in asm-offsets.c and switch the > >> current code to use those instead. > > > > Just to mention it (without asking for immediate action): Defining boot_vid_info > > in assembly code then is as fragile. Moving to C would likely be problematic > > because it needs to be in the trampoline range. But at least its size should (at > > some point) perhaps better be tied to the C struct's sizeof(). > > Actually I overlooked that you partly do this. The use of BVI_capabilities there > looks odd to me, though. Why not > > .space BVI_size - (. - boot_vid_info) > > ? I realize it becomes just BVI_size in patch 2, but I have some question there, > too. I didn't think much about this because it was going away in the next patch, but let me adjust. In fact I was tempted to just use BVI_size and allocate more than required. Thanks, Roger.
diff --git a/xen/arch/x86/boot/video.S b/xen/arch/x86/boot/video.S index 0ae04f270f8c..a4b25a3b34d1 100644 --- a/xen/arch/x86/boot/video.S +++ b/xen/arch/x86/boot/video.S @@ -26,32 +26,13 @@ /* Force 400 scan lines for standard modes (hack to fix bad BIOS behaviour */ #undef CONFIG_VIDEO_400_HACK -/* Positions of various video parameters passed to the kernel */ -/* (see also include/linux/tty.h) */ -#define PARAM_CURSOR_POS 0x00 -#define PARAM_VIDEO_MODE 0x02 -#define PARAM_VIDEO_COLS 0x03 -#define PARAM_VIDEO_LINES 0x04 -#define PARAM_HAVE_VGA 0x05 -#define PARAM_FONT_POINTS 0x06 -#define PARAM_CAPABILITIES 0x08 -#define PARAM_LFB_LINELENGTH 0x0c -#define PARAM_LFB_WIDTH 0x0e -#define PARAM_LFB_HEIGHT 0x10 -#define PARAM_LFB_DEPTH 0x12 -#define PARAM_LFB_BASE 0x14 -#define PARAM_LFB_SIZE 0x18 -#define PARAM_LFB_COLORS 0x1c -#define PARAM_VESAPM_SEG 0x24 -#define PARAM_VESAPM_OFF 0x26 -#define PARAM_VESA_ATTRIB 0x28 #define _param(param) bootsym(boot_vid_info)+(param) video: xorw %ax, %ax movw %ax, %gs # GS is zero cld call basic_detect # Basic adapter type testing (EGA/VGA/MDA/CGA) - cmpb $0,_param(PARAM_HAVE_VGA) + cmpb $0,_param(BVI_have_vga) je 1f # Bail if there's no VGA movw bootsym(boot_vid_mode), %ax # User selected video mode cmpw $ASK_VGA, %ax # Bring up the menu @@ -69,7 +50,7 @@ vid1: call store_edid # Detect if we have CGA, MDA, EGA or VGA and pass it to the kernel. basic_detect: - movb $0, _param(PARAM_HAVE_VGA) + movb $0, _param(BVI_have_vga) movb $0x12, %ah # Check EGA/VGA movb $0x10, %bl int $0x10 @@ -79,7 +60,7 @@ basic_detect: int $0x10 cmpb $0x1a, %al # 1a means VGA... jne basret # anything else is EGA. - incb _param(PARAM_HAVE_VGA) # We've detected a VGA + incb _param(BVI_have_vga) # We've detected a VGA basret: ret # Store the video mode parameters for later usage by the kernel. @@ -92,57 +73,57 @@ mode_params: movb $0x03, %ah # Read cursor position xorb %bh, %bh int $0x10 - movw %dx, _param(PARAM_CURSOR_POS) + movw %dx, _param(BVI_cursor_pos) movb $0x0f, %ah # Read page/mode/width int $0x10 - movw %ax, _param(PARAM_VIDEO_MODE) # Video mode and screen width + movw %ax, _param(BVI_video_mode) # Video mode and screen width movw %gs:(0x485), %ax # Font size - movw %ax, _param(PARAM_FONT_POINTS) # (valid only on EGA/VGA) + movw %ax, _param(BVI_font_points) # (valid only on EGA/VGA) movw bootsym(force_size), %ax # Forced size? orw %ax, %ax jz mopar1 - movb %ah, _param(PARAM_VIDEO_COLS) - movb %al, _param(PARAM_VIDEO_LINES) + movb %ah, _param(BVI_video_cols) + movb %al, _param(BVI_video_lines) ret mopar1: movb %gs:(0x484), %al # On EGA/VGA, use the EGA+ BIOS incb %al # location of max lines. -mopar2: movb %al, _param(PARAM_VIDEO_LINES) +mopar2: movb %al, _param(BVI_video_lines) ret # Fetching of VESA frame buffer parameters mopar_gr: movw $vesa_mode_info, %di - movb $0x23, _param(PARAM_HAVE_VGA) + movb $0x23, _param(BVI_have_vga) movw 16(%di), %ax - movw %ax, _param(PARAM_LFB_LINELENGTH) + movw %ax, _param(BVI_lfb_linelength) movw 18(%di), %ax - movw %ax, _param(PARAM_LFB_WIDTH) + movw %ax, _param(BVI_lfb_width) movw 20(%di), %ax - movw %ax, _param(PARAM_LFB_HEIGHT) + movw %ax, _param(BVI_lfb_height) movzbw 25(%di), %ax - movw %ax, _param(PARAM_LFB_DEPTH) + movw %ax, _param(BVI_lfb_depth) movl 40(%di), %eax - movl %eax, _param(PARAM_LFB_BASE) + movl %eax, _param(BVI_lfb_base) movl 31(%di), %eax - movl %eax, _param(PARAM_LFB_COLORS) + movl %eax, _param(BVI_lfb_colors) movl 35(%di), %eax - movl %eax, _param(PARAM_LFB_COLORS+4) + movl %eax, _param(BVI_lfb_colors+4) movw 0(%di), %ax - movw %ax, _param(PARAM_VESA_ATTRIB) + movw %ax, _param(BVI_vesa_attrib) # get video mem size movw $vesa_glob_info, %di movzwl 18(%di), %eax - movl %eax, _param(PARAM_LFB_SIZE) + movl %eax, _param(BVI_lfb_size) # store mode capabilities movl 10(%di), %eax - movl %eax, _param(PARAM_CAPABILITIES) + movl %eax, _param(BVI_capabilities) # switching the DAC to 8-bit is for <= 8 bpp only - cmpw $8, _param(PARAM_LFB_DEPTH) + cmpw $8, _param(BVI_lfb_depth) jg dac_done # get DAC switching capability @@ -160,16 +141,16 @@ mopar_gr: dac_set: # set color size to DAC size movzbw bootsym(dac_size), %ax - movb %al, _param(PARAM_LFB_COLORS+0) - movb %al, _param(PARAM_LFB_COLORS+2) - movb %al, _param(PARAM_LFB_COLORS+4) - movb %al, _param(PARAM_LFB_COLORS+6) + movb %al, _param(BVI_lfb_colors+0) + movb %al, _param(BVI_lfb_colors+2) + movb %al, _param(BVI_lfb_colors+4) + movb %al, _param(BVI_lfb_colors+6) # set color offsets to 0 - movb %ah, _param(PARAM_LFB_COLORS+1) - movb %ah, _param(PARAM_LFB_COLORS+3) - movb %ah, _param(PARAM_LFB_COLORS+5) - movb %ah, _param(PARAM_LFB_COLORS+7) + movb %ah, _param(BVI_lfb_colors+1) + movb %ah, _param(BVI_lfb_colors+3) + movb %ah, _param(BVI_lfb_colors+5) + movb %ah, _param(BVI_lfb_colors+7) dac_done: # get protected mode interface information @@ -180,8 +161,8 @@ dac_done: cmp $0x004f, %ax jnz no_pm - movw %es, _param(PARAM_VESAPM_SEG) - movw %di, _param(PARAM_VESAPM_OFF) + movw %es, _param(BVI_vesapm_seg) + movw %di, _param(BVI_vesapm_off) no_pm: pushw %ds popw %es @@ -1018,7 +999,7 @@ GLOBAL(boot_vid_info) .byte 80, 25 /* 80x25 */ .byte 1 /* isVGA */ .word 16 /* 8x16 font */ - .fill 0x28,1,0 + .space BVI_size - BVI_capabilities GLOBAL(boot_edid_info) .fill 128,1,0x13 GLOBAL(boot_edid_caps) diff --git a/xen/arch/x86/x86_64/asm-offsets.c b/xen/arch/x86/x86_64/asm-offsets.c index d8903a3ce9c7..91da6b9d3885 100644 --- a/xen/arch/x86/x86_64/asm-offsets.c +++ b/xen/arch/x86/x86_64/asm-offsets.c @@ -16,6 +16,10 @@ #include <xen/multiboot.h> #include <xen/multiboot2.h> +#ifdef CONFIG_VIDEO +# include "../boot/video.h" +#endif + #define DEFINE(_sym, _val) \ asm volatile ( "\n.ascii\"==>#define " #_sym " %0 /* " #_val " */<==\""\ :: "i" (_val) ) @@ -208,4 +212,26 @@ void __dummy__(void) OFFSET(DOMAIN_vm_assist, struct domain, vm_assist); BLANK(); + +#ifdef CONFIG_VIDEO + DEFINE(BVI_size, sizeof(struct boot_video_info)); + OFFSET(BVI_cursor_pos, struct boot_video_info, orig_x); + OFFSET(BVI_video_mode, struct boot_video_info, orig_video_mode); + OFFSET(BVI_video_cols, struct boot_video_info, orig_video_cols); + OFFSET(BVI_video_lines, struct boot_video_info, orig_video_lines); + OFFSET(BVI_have_vga, struct boot_video_info, orig_video_isVGA); + OFFSET(BVI_font_points, struct boot_video_info, orig_video_points); + OFFSET(BVI_capabilities, struct boot_video_info, capabilities); + OFFSET(BVI_lfb_linelength, struct boot_video_info, lfb_linelength); + OFFSET(BVI_lfb_width, struct boot_video_info, lfb_width); + OFFSET(BVI_lfb_height, struct boot_video_info, lfb_height); + OFFSET(BVI_lfb_depth, struct boot_video_info, lfb_depth); + OFFSET(BVI_lfb_base, struct boot_video_info, lfb_base); + OFFSET(BVI_lfb_size, struct boot_video_info, lfb_size); + OFFSET(BVI_lfb_colors, struct boot_video_info, colors); + OFFSET(BVI_vesapm_seg, struct boot_video_info, vesapm.seg); + OFFSET(BVI_vesapm_off, struct boot_video_info, vesapm.off); + OFFSET(BVI_vesa_attrib, struct boot_video_info, vesa_attrib); + BLANK(); +#endif /* CONFIG_VIDEO */ }
Currently the offsets into the boot_video_info struct are manually encoded in video.S, which is fragile. Generate them in asm-offsets.c and switch the current code to use those instead. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/boot/video.S | 83 ++++++++++++------------------- xen/arch/x86/x86_64/asm-offsets.c | 26 ++++++++++ 2 files changed, 58 insertions(+), 51 deletions(-)