Re: [PATCH v3 15/24] xen/console: make console buffer size configurable

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx>
> 
> Add new CONRING_SIZE Kconfig parameter to specify the boot console buffer size
> in bytes. The value is rounded to the nearest power of 2 to match existing
> conring_size= behavior.
> 
> The supported range is [16KiB..128MiB].
> 
> Bump default size to 32 KiB.
> 
> Link: https://gitlab.com/xen-project/xen/-/issues/185
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

As asked elsewhere already: How's this related to the goal of the series?

> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -423,12 +423,15 @@ The following are examples of correct specifications:
>      com1=baud=115200,parity=n,stop-bits=1,io-base=0x3f8,reg-width=4
>  
>  ### conring_size
> -> `= <size>`
> +> `= <size-in-bytes>`

May I direct you to the explanations near the top of the file? <size>
is a uniform term throughout this document, and wants to stay like this.

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -96,6 +96,17 @@ config SERIAL_TX_BUFSIZE
>  
>         Default value is 32768 (32KiB).
>  
> +config CONRING_SIZE
> +     int "Console buffer size"
> +     default 32768
> +     help
> +       Select the boot console buffer size (in bytes).
> +       Note, the value provided will be rounded down to the nearest power of 
> 2.
> +       Run-time console buffer size is the same as the boot console size,
> +       unless enforced via 'conring_size=' boot parameter.

Maybe s/enforced/overridden/ ?

> +       Default value is 32768 (32KiB). The supported range is 
> [16KiB..128MiB].

Yet then there's no "range" directive.

> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -100,12 +100,15 @@ static int cf_check parse_console_timestamps(const char 
> *s);
>  custom_runtime_param("console_timestamps", parse_console_timestamps,
>                       con_timestamp_mode_upd);
>  
> -/* conring_size: allows a large console ring than default (16kB). */
> +/* conring_size: allows a large console ring than default (32 KiB). */

As you touch this, also s/large/larger/ ?

>  static uint32_t __initdata opt_conring_size;
>  size_param("conring_size", opt_conring_size);
>  
> -#define _CONRING_SIZE 16384
> -#define CONRING_IDX_MASK(i) ((i)&(conring_size-1))
> +#define _CONRING_SIZE       (1UL << (31 - 
> __builtin_clz(CONFIG_CONRING_SIZE)))
> +_Static_assert(_CONRING_SIZE >= 4096 && _CONRING_SIZE <= MB(128),
> +    "CONFIG_CONRING_SIZE must be in [4K..128M] range");

Hmm, 4k here as the lower bound, when in description and Kconfig it's
said to be 16k?

Also I fear _Static_assert() can't be used here, for not being supported
by all gcc versions we continue to permit being used on x86. That'll be
unnecessary anyway once you put in place the missing range directive in
Kconfig. (If something like this needed keeping, it would be
BUILD_BUG_ON() that you want to use instead. Which, yes, can only be
used inside a function. Hence why we have a number of build_assertions()
functions throughout the codebase.)

> +#define CONRING_IDX_MASK(i) ( (i) & (conring_size - 1) )

Once again - no blanks immediately inside parentheses, _except_ as
written in ./CODING_STYLE (i.e. in control flow statements).

Jan