Re: [PATCH v6 4/4] xen/console: switch conring runtime allocation to xvmalloc

["Roger Pau Monné" <roger.pau@citrix.com>]

On Fri, May 08, 2026 at 05:57:14PM -0700, dmukhin@ford.com wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
> 
> The console ring only needs to be virtually contiguous; it does not need
> a naturally aligned or physically contiguous allocation. Replace the
> runtime xenheap allocation in console_init_ring() with an xvmalloc-backed
> buffer.
> 
> Also clamp the user-configured ring size to the supported range and emit
> warnings when the requested size is adjusted.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>
> ---
> Changes since v5:
> - switched to xvmalloc_array()
> - fixed conring size checks
> - corrected diagnostic messages
> ---
>  xen/drivers/char/console.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index 5cac87d052b9..29b9359468e7 100644
> --- a/xen/drivers/char/console.c
> +++ b/xen/drivers/char/console.c
> @@ -33,6 +33,7 @@
>  #include <asm/setup.h>
>  #include <xen/sections.h>
>  #include <xen/consoled.h>
> +#include <xen/xvmalloc.h>
>  
>  #ifdef CONFIG_X86
>  #include <asm/guest.h>
> @@ -343,6 +344,7 @@ static void cf_check do_dec_thresh(unsigned char key, bool unused)
>  static unsigned int __initdata opt_conring_size;
>  size_param("conring_size", opt_conring_size);
>  
> +#define CONRING_SIZE_MIN    (1U << 14)
>  #define _CONRING_SIZE       (1U << CONFIG_CONRING_SHIFT)
>  #define CONRING_IDX_MASK(i) ((i) & (conring_size - 1))
>  static char __initdata _conring[_CONRING_SIZE];
> @@ -464,20 +466,33 @@ void __init console_init_ring(void)
>  {
>      char *ring;
>      XENCONS_RING_IDX i, size;
> -    unsigned int order, memflags;
> +    unsigned int order;
>      unsigned long flags;
>  
>      if ( !opt_conring_size )
>          return;
>  
>      order = get_order_from_bytes(max(opt_conring_size, conring_size));
> -    memflags = MEMF_bits(crashinfo_maxaddr_bits);
> -    while ( (ring = alloc_xenheap_pages(order, memflags)) == NULL )
> +    size = PAGE_SIZE << order;
> +    if ( size != opt_conring_size )
>      {
> -        BUG_ON(order == 0);
> -        order--;
> +        opt_conring_size = size;
> +        printk(XENLOG_WARNING "Normalizing console ring size.\n");

I think you want to also specify to what it has been normalized, ie:

"Normalizing command line console ring size %u to %u.\n", opt_conring_size, size

However, note how console_init_postirq() sets opt_conring_size if not
specified, and AFAICT that calculated value might not be a power of 2,
and it might be lower than the build time setting.  If you want to
print warning messages about the user-provided value (if any), you
need to split the checking from console_init_ring(), and do it
exclusively for the command line user-provided value, not for the
value generated by Xen in console_init_postirq().

Also, a user attempting to set a console buffer size below the
built-time value will get this "Normalizing size" message, when it
should otherwise get a message about the command line value being
smaller than the built time setting.

>      }
> -    opt_conring_size = PAGE_SIZE << order;
> +    if ( opt_conring_size < CONRING_SIZE_MIN )
> +    {
> +        opt_conring_size = 0;
> +        printk(XENLOG_WARNING "Ignoring too-small console ring size override.\n");
> +        return;
> +    }

I don't think this is possible, as the built time Kconfig value is
forced to be >= 14, and hence the order calculated above can never be
smaller than 14.

> +    else if ( opt_conring_size > GB(2) )
> +    {
> +        opt_conring_size = GB(2);
> +        printk(XENLOG_WARNING "Limiting user-configured console ring size to 2 GiB.\n");
> +    }
> +
> +    ring = xvmalloc_array(char, opt_conring_size);
> +    BUG_ON(ring == NULL);

Since you are already touching this, might I suggest to use a panic
rather than a BUG?

if ( !ring )
    panic("Unable to allocate console ring buffer of size %u\n",...);

Thanks, Roger.