Mailing List Archive

[PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets
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(-)

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 */
}
--
2.44.0
Re: [PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets [ In reply to ]
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
Re: [PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets [ In reply to ]
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
Re: [PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets [ In reply to ]
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
Re: [PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets [ In reply to ]
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.