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

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Tuesday, January 28th, 2025 at 8:32 AM, Jan Beulich <jbeulich@xxxxxxxx> 
wrote:

> 
> 
> 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?

I mentioned in the cover letter that there are two group of patches - console
driver cleanups/fixes and the follow on UART emulator (and up until v3 it was OK
to have this patch bundled into the series).

Yes, I acknowledge that the first group of patches for console driver grew big
and probably should have been posted in its own thread.

I can move "console" part to its own series if it makes sense now.

What do you think?

> 
> > --- 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