Re: [PATCH 3/3] drivers/char: Panic when the requested UART fails to initialise

["Halder, Ayan Kumar" <ayankuma@xxxxxxx>]

Hi,

On 02/09/2026 08:36, Michal Orzel wrote:
> uart_init() cannot tell its caller that the UART the user asked for did
> not come up: every failure path only printks. Arm and RISC-V carry on
> into console_init_preirq() and boot without a console, rather than
> refusing to boot as they do elsewhere when a user request cannot be met.
I just want to emphasize that from functional safety perspective, this 
is the preferred approach. The user's request is given the priority and 
whenever it cannot be satisfied, Xen should panic.
> Return an error from dt_uart_init() and panic in start_xen(). An
> explicit request Xen cannot satisfy should stop the boot rather than
> silently degrade it,
If there is a silent degradation, then we need to document this behavior 
somewhere. I am happy to keep this documented under docs/fusa.
In the safety manual, we should mention all the instances when there is 
a silent degradation observed, the underlying reason and how the end 
user can detect it.
Other FuSa experts can comment.

>   which is what start_xen() already does for the rest
> of the boot configuration.
>
> Only a path given on the command line counts as a request we have to
> satisfy. Falling back to /chosen/stdout-path or acpi_uart_init()
> therefore never fails. SPCR is firmware provided, the analogue of
> stdout-path, and there is no ACPI equivalent of dtuart= to make an
> explicit request with.
>
> While here, decide whether the SPCR table was found from the returned
> acpi_status rather than from the table pointer, which was only NULL
> because the caller initialised it - acpi_get_table() writes it solely
> on success.
>
> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
> ---
> With this change diagnosibility decreases only for a single scenario:
> when dom0 is reachable not via console (e.g. network) and you'd have used xl
> dmesg to read messages from the conring.
>
> On Arm (I suppose RISC-V is similar), given that safety becomes the major
> use-case and we need to satisfy all the user/guest-xen contracts, I think the
> patch moves us in a direction we already chose (i.e. we panic on every boot
> failure where we cannot meet the requests).
> ---
>   xen/arch/arm/setup.c         |  5 +++-
>   xen/arch/riscv/setup.c       |  6 ++++-
>   xen/drivers/char/uart-init.c | 52 +++++++++++++++++++++---------------
>   xen/include/xen/serial.h     |  6 ++++-
>   4 files changed, 44 insertions(+), 25 deletions(-)
>
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 6310a47d68b6..d0066db42e7c 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -379,7 +379,10 @@ void asmlinkage __init noreturn start_xen(unsigned long 
> fdt_paddr)
>   
>       gic_preinit();
>   
> -    uart_init();
> +    rc = uart_init();
> +    if ( rc )
> +        panic("Failed to initialize the requested UART (%d)\n", rc);
> +
>       console_init_preirq();
>       console_init_ring();
>   
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 56a0907a855f..07f46ac3ce27 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -77,6 +77,7 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>   {
>       const char *cmdline;
>       size_t fdt_size;
> +    int rc;
>   
>       remove_identity_mapping();
>   
> @@ -149,7 +150,10 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>   
>       intc_preinit();
>   
> -    uart_init();
> +    rc = uart_init();
> +    if ( rc )
> +        panic("Failed to initialize the requested UART (%d)\n", rc);
> +
>       console_init_preirq();
>   
>       intc_init();
> diff --git a/xen/drivers/char/uart-init.c b/xen/drivers/char/uart-init.c
> index eb7f85549593..b79135be9620 100644
> --- a/xen/drivers/char/uart-init.c
> +++ b/xen/drivers/char/uart-init.c
> @@ -30,15 +30,17 @@
>   static char __initdata opt_dtuart[256] = "";
>   string_param("dtuart", opt_dtuart);
>   
> -static void __init dt_uart_init(void)
> +static int __init dt_uart_init(void)
>   {
>       struct dt_device_node *dev;
>       int ret;
>       const char *devpath = opt_dtuart;
>       const char *options;
>       char *split;
> +    /* Set on the command line, as opposed to inherited from /chosen */
> +    bool explicit_request = strcmp(opt_dtuart, "") != 0;
>   
> -    if ( !strcmp(opt_dtuart, "") )
> +    if ( !explicit_request )
>       {
>           const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>   
> @@ -62,7 +64,12 @@ static void __init dt_uart_init(void)
>       if ( !strcmp(opt_dtuart, "") )
>       {
>           printk("No dtuart path configured\n");
> -        return;
> +
> +        /*
> +         * console=dtuart is the compiled-in default, so an absent dtuart= is
> +         * not a failed user request.
> +         */
> +        return 0;
>       }
>   
>       split = strchr(opt_dtuart, ':');
> @@ -83,48 +90,49 @@ static void __init dt_uart_init(void)
>       if ( !dev )
>       {
>           printk("Unable to find device \"%s\"\n", devpath);
> -        return;
> +        return explicit_request ? -ENODEV : 0;
>       }
>   
>       ret = device_init(dev, DEVICE_SERIAL, options);
> -
>       if ( ret )
>           printk("Unable to initialize dtuart: %d\n", ret);
> +
> +    return explicit_request ? ret : 0;
>   }
>   
>   #ifdef CONFIG_ACPI
> -static void __init acpi_uart_init(void)
> +static int __init acpi_uart_init(void)
>   {
> -    struct acpi_table_spcr *spcr = NULL;
> +    struct acpi_table_spcr *spcr;
> +    acpi_status status;
>       int ret;
>   
> -    acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header **)&spcr);
> +    /* SPCR is firmware provided, so nothing here is a failed user request */
> +    status = acpi_get_table(ACPI_SIG_SPCR, 0,
> +                            (struct acpi_table_header **)&spcr);
>   
> -    if ( spcr == NULL )
> +    if ( ACPI_FAILURE(status) )
>       {
>           printk("Unable to get spcr table\n");
> +        return 0;
>       }
> -    else
> -    {
> -        ret = acpi_device_init(DEVICE_SERIAL, NULL, spcr->interface_type);
>   
> -        if ( ret )
> -            printk("Unable to initialize acpi uart: %d\n", ret);
> -    }
> +    ret = acpi_device_init(DEVICE_SERIAL, NULL, spcr->interface_type);
> +    if ( ret )
> +        printk("Unable to initialize acpi uart: %d\n", ret);
> +
> +    return 0;
>   }
>   #else
> -static void __init acpi_uart_init(void) { }
> +static int __init acpi_uart_init(void) { return 0; }
>   #endif
>   
> -void __init uart_init(void)
> +int __init uart_init(void)
>   {
>       if ( !console_has("dtuart") )
> -        return; /* Not for us */
> +        return 0; /* Not for us */
>   
> -    if ( acpi_disabled )
> -        dt_uart_init();
> -    else
> -        acpi_uart_init();
> +    return acpi_disabled ? dt_uart_init() : acpi_uart_init();
>   }
>   
>   /*
> diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
> index 8e1844555208..3a71da767dd7 100644
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -170,7 +170,11 @@ void xhci_dbc_uart_init(void);
>   static void inline xhci_dbc_uart_init(void) {}
>   #endif
>   
> -void uart_init(void);
> +/*
> + * Returns 0 unless a UART explicitly requested via dtuart= failed to
> + * initialise.
> + */
> +int uart_init(void);
>   
>   struct physdev_dbgp_op;
>   int dbgp_op(const struct physdev_dbgp_op *op);
LGTM

- Ayan