Re: [PATCH 7/7] video/vesa: adjust (not just) command line option handling

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/04/2021 13:56, Jan Beulich wrote:

The grammar in the subject is very awkward.  The (not just) like that is
weird.

If it were me, I'd phrase this as "minor adjustments to command line
handling".

> Document both options. Add section annotations to both variables holding
> the parsed values as well as a few adjacent ones. Adjust the types of
> font_height and vga_compat.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

In principle, Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, with
one note below.

However, is there really any value in these options?  I can't see a case
where their use will result in a less broken system.

>
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2369,9 +2369,21 @@ cache-warming. 1ms (1000) has been measu
>  ### vesa-map
>  > `= <integer>`
>  
> +> Default: `0`
> +
> +Specify, in MiB, the portion of video RAM to actually remap.  This will be
> +honored only when large enough to cover the space needed for the chosen video
> +mode, and only when less than a non-zero value possibly specified through
> +'vesa-ram'.

"and only when less than a non-zero value possibly specified" is
confusing to follow.

What I think you mean is that vesa-map will be honoured when it is >=
chosen video mode, and <= vesa-ram?

~Andrew

> +
>  ### vesa-ram
>  > `= <integer>`
>  
> +> Default: `0`
> +
> +This allows to override the amount of video RAM, in MiB, determined to be
> +present.
> +
>  ### vga
>  > `= ( ask | current | text-80x<rows> | gfx-<width>x<height>x<depth> | 
> mode-<mode> )[,keep]`
>  
> --- a/xen/drivers/video/vesa.c
> +++ b/xen/drivers/video/vesa.c
> @@ -19,17 +19,17 @@
>  
>  static void lfb_flush(void);
>  
> -static unsigned char *lfb;
> -static const struct font_desc *font;
> -static bool_t vga_compat;
> +static unsigned char *__read_mostly lfb;
> +static const struct font_desc *__initdata font;
> +static bool __initdata vga_compat;
>  
> -static unsigned int vram_total;
> +static unsigned int __initdata vram_total;
>  integer_param("vesa-ram", vram_total);
>  
> -static unsigned int vram_remap;
> +static unsigned int __initdata vram_remap;
>  integer_param("vesa-map", vram_remap);
>  
> -static int font_height;
> +static unsigned int __initdata font_height;
>  static int __init parse_font_height(const char *s)
>  {
>      if ( simple_strtoul(s, &s, 10) == 8 && (*s++ == 'x') )
>