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

Commit Message

Denis Mukhin via B4 Relay Jan. 4, 2025, 1:58 a.m. UTC

From: Denis Mukhin <[e-mail]>

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 <[e-mail]>
---
 docs/misc/xen-command-line.pandoc |  7 +++++--
 xen/drivers/char/Kconfig          | 11 +++++++++++
 xen/drivers/char/console.c        |  9 ++++++---
 3 files changed, 22 insertions(+), 5 deletions(-)

Comments

Jan Beulich Jan. 28, 2025, 4:32 p.m. UTC | #1

On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> From: Denis Mukhin <[e-mail]>
> 
> 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 <[e-mail]>

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

Denis Mukhin Jan. 29, 2025, 3:04 a.m. UTC | #2

On Tuesday, January 28th, 2025 at 8:32 AM, Jan Beulich <[e-mail]> wrote:

> 
> 
> On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> 
> > From: Denis Mukhin [e-mail]
> > 
> > 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 [e-mail]
> 
> 
> 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

Jan Beulich Jan. 29, 2025, 7:53 a.m. UTC | #3

On 29.01.2025 04:04, Denis Mukhin wrote:
> On Tuesday, January 28th, 2025 at 8:32 AM, Jan Beulich <[e-mail]> wrote:
>> On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
>>
>>> From: Denis Mukhin [e-mail]
>>>
>>> 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 [e-mail]
>>
>>
>> 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?

I for one would appreciate you doing so. Where patches are independent, you
may even want to consider posting them individually. That way it'll be clear
they're isolated, and hence any one of them that is fully reviewed/acked can
go in (once the tree is fully open again).

Jan

Denis Mukhin Feb. 12, 2025, 10:55 p.m. UTC | #4

On Tuesday, January 28th, 2025 at 11:53 PM, Jan Beulich <[e-mail]> wrote:

> 
> 
> On 29.01.2025 04:04, Denis Mukhin wrote:
> 
> > On Tuesday, January 28th, 2025 at 8:32 AM, Jan Beulich [e-mail] wrote:
> > 
> > > On 04.01.2025 02:58, Denis Mukhin via B4 Relay wrote:
> > > 
> > > > From: Denis Mukhin [e-mail]
> > > > 
> > > > 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 [e-mail]
> > > 
> > > 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?
> 
> 
> I for one would appreciate you doing so. Where patches are independent, you
> may even want to consider posting them individually. That way it'll be clear
> they're isolated, and hence any one of them that is fully reviewed/acked can
> go in (once the tree is fully open again).

Moved patch to a separate thread:
  https://lore.kernel.org/xen-devel/[e-mail]/

> 
> Jan

Patch

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 1a5ee3c91c5cc8bf653e5054211035b5d1bd560f..aeaee482f61aab41438a44eda470902e1e0806f8 100644
--- 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>`
 
-> Default: `conring_size=16k`
+> Default: `conring_size=32k`
 
 Specify the size of the console ring buffer.
 
+The console ring buffer size can be selected at build time via
+CONFIG_CONRING_SIZE.
+
 ### console
 > `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | ehci | xhci | none ]`
 
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e6e12bb4139717f9319031f51f5d20155d2caee2..f7e193e29e2d9ac7c1b878e13f3fb1ce444f31b5 100644
--- 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.
+
+	  Default value is 32768 (32KiB). The supported range is [16KiB..128MiB].
+
 config XHCI
 	bool "XHCI DbC UART driver"
 	depends on X86
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index 1308236403df8a0f87aeb7e2c00a036c2d8433a7..a33132b8fad95a1ad7e0aab4aef3fa3e46a5c03a 100644
--- 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). */
 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");
+
+#define CONRING_IDX_MASK(i) ( (i) & (conring_size - 1) )
 static char __initdata _conring[_CONRING_SIZE];
 static char *__read_mostly conring = _conring;
 static uint32_t __read_mostly conring_size = _CONRING_SIZE;