Re: [PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Roger Pau Monne <roger.pau@citrix.com>, xen-devel@lists.xenproject.org
Cc: Jan Beulich <jbeulich@suse.com>
Subject: Re: [PATCH 1/2] x86/video: add boot_video_info offset generation to asm-offsets
Date: Thu, 28 Mar 2024 19:55:41 +0000	[thread overview]
Message-ID: <ff5e2e70-a8d1-49cd-a15d-eb65a58eab34@citrix.com> (raw)
In-Reply-To: <20240328153523.4155-2-roger.pau@citrix.com>

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