Re: [PATCH v2 26/35] xen/console: make console buffer size configurable

[Denis Mukhin <dmkhn@xxxxxxxxx>]

On Thursday, December 12th, 2024 at 4:47 AM, Roger Pau Monné 
<roger.pau@xxxxxxxxxx> wrote:

>
>
> On Thu, Dec 05, 2024 at 08:41:56PM -0800, Denis Mukhin via B4 Relay wrote:
>
> > From: Denis Mukhin dmukhin@xxxxxxxx
> >
> > Add new CONRING_LOG_SHIFT Kconfig parameter to specify the boot console 
> > buffer
> > size as a power of 2.
> >
> > Bump default size to 32 KiB.
> >
> > Link: https://gitlab.com/xen-project/xen/-/issues/185
> > Signed-off-by: Denis Mukhin dmukhin@xxxxxxxx
>
>
> Thanks for taking care of this.
>
> > ---
> > xen/drivers/char/Kconfig | 23 +++++++++++++++++++++++
> > xen/drivers/char/console.c | 4 ++--
> > 2 files changed, 25 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> > index 
> > e6e12bb4139717f9319031f51f5d20155d2caee2..3bc892fc38d8cdeb3c76ea44d747f712a8d0d372
> >  100644
> > --- a/xen/drivers/char/Kconfig
> > +++ b/xen/drivers/char/Kconfig
> > @@ -96,6 +96,29 @@ config SERIAL_TX_BUFSIZE
> >
> > Default value is 32768 (32KiB).
> >
> > +config CONRING_LOG_SHIFT
> > + int "Console buffer size"
> > + range 14 25
> > + default 15
> > + help
> > + Select the boot console buffer size as a power of 2.
> > + Run-time console buffer size is the same as the boot console size,
> > + unless enforced via 'conring_size=' boot parameter.
> > +
> > + Examples:
> > + 25 => 32 MiB
> > + 24 => 16 MiB
> > + 23 => 8 MiB
> > + 22 => 4 MiB
> > + 21 => 2 MiB
> > + 20 => 1 MiB
> > + 19 => 512 KiB
> > + 18 => 256 KiB
> > + 17 => 128 KiB
> > + 16 => 64 KiB
> > + 15 => 32 KiB
> > + 14 => 16 KiB
>
>
> It might be better to do something similar to what we do in
> SERIAL_TX_BUFSIZE, that the user provides a value in KiB which is
> rounded down to the nearest power of 2?

TBH, I do not think there should be a way for the user to allow reserving huge
amounts of RAM for the console ring. Plus, using logarithmic scale may avoid
confusion, w/o it user-defined CONFIG_CONRING_SIZE will not necessarily
match the real _CONRING_SIZE value.

But I see your point: Kconfig should match existing conring_size= spec which
defines number of bytes.

>
> > +
> > 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 
> > d22fb4a253af26f9b51d91bd408e1dbf4bb5a7c1..581ee22b85302a54db5b9d5d28e8b2d689d31403
> >  100644
> > --- a/xen/drivers/char/console.c
> > +++ b/xen/drivers/char/console.c
> > @@ -104,11 +104,11 @@ 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);
>
>
> You also need to update xen-command-line.pandoc to mention the default
> size is now set in Kconfig. And here I would mention
> CONFIG_CONRING_SIZE rather than explicit 32 KiB, because that's just
> the default in Kconfig, but might not be the default in the build
> itself.

Done.

>
> FWIW, you could define:
>
> #define _CONRING_SIZE (CONFIG_CONRING_SIZE & (CONFIG_CONRING_SIZE - 1))

Done.

>
> Thanks, Roger.