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

[Michal Orzel <michal.orzel@xxxxxxx>]

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.

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, 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);
-- 
2.43.0