[PATCH v1 0/5] Fix XHCI console on legacy MB2 boot

[PATCH v1 0/5] Fix XHCI console on legacy MB2 boot

[Marek Marczykowski-Górecki]

XHCI console works fine with UEFI boot, as Xen doesn't relocate itself. But on
legacy boot, relocation is a problem, as the hardware is programmed with
physical addresses of structures that are moved. Fix this by adding new console
init hooks. This series includes also few minor improvements that were useful
when working on the fix.

Marek Marczykowski-Górecki (5):
  console: add relocation hook
  drivers/char: Handle Xen relocation in the XHCI console driver
  drivers/char: make dbc_uart_dump() a bit more useful
  drivers/char: remove outdated comment in xhci driver
  console: support multiple serial console simultaneously

 docs/misc/xen-command-line.pandoc |   4 +-
 xen/arch/x86/setup.c              |   8 ++-
 xen/drivers/char/Kconfig          |  11 +++-
 xen/drivers/char/console.c        | 108 ++++++++++++++++++++++++-------
 xen/drivers/char/serial.c         |  18 +++++-
 xen/drivers/char/xhci-dbc.c       |  98 +++++++++++++++++++---------
 xen/include/xen/console.h         |   2 +-
 xen/include/xen/serial.h          |   7 ++-
 8 files changed, 204 insertions(+), 52 deletions(-)

base-commit: 7ab4b392b78b5ac1c7a1fb1d085637526e67521a
-- 
git-series 0.9.1

[PATCH v1 1/5] console: add relocation hook

[Marek Marczykowski-Górecki]

The XHCI console uses DMA, so it's sensitive for relocating its
structures, even if their virtual addresses remain the same. Add a new
console initialization hooks called before+after Xen relocation.
Relocation happens conditionally, but call the hooks unconditionally, as
that simplifies logic in the driver (and if needed, the driver can
easily detect if relocation happened anyway). Thanks to that, a driver
may use it to finalize init steps that need physical address but can be
delayed - this way, it doesn't need un-doing on relocation.
The most important part is the post-relocation hook, but add also
pre-relocation one to simplify clean handling of in-flight data.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
I considered more limited scope - calling them just around move_xen()
(or even from within that function), but that complicates
iommu_add_extra_reserved_device_memory() call - see the next patch.
As for the post-relocation hook, I have considered a parameter with info
whether the relocation actually happened, but driver can figure it out
on its own anyway.
---
 xen/arch/x86/setup.c       |  8 ++++++++
 xen/drivers/char/console.c | 10 ++++++++++
 xen/drivers/char/serial.c  | 18 ++++++++++++++++++
 xen/include/xen/console.h  |  2 ++
 xen/include/xen/serial.h   |  6 ++++++
 5 files changed, 44 insertions(+)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 25189541244d..3ef819a252e4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
         highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
 #endif
 
+    console_init_pre_relocate();
+
     /*
      * Iterate backwards over all superpage-aligned RAM regions.
      *
@@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
     if ( !xen_phys_start )
         panic("Not enough memory to relocate Xen\n");
 
+    /*
+     * Notify console drivers about relocation, before reusing old Xen's
+     * memory.
+     */
+    console_init_post_relocate();
+
     /* FIXME: Putting a hole in .bss would shatter the large page mapping. */
     if ( using_2M_mapping() )
         efi_boot_mem_unused(NULL, NULL);
diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index c15987f5bbe2..12898b684b5e 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -1120,6 +1120,16 @@ void __init console_init_ring(void)
     printk("Allocated console ring of %u KiB.\n", opt_conring_size >> 10);
 }
 
+void __init console_init_pre_relocate(void)
+{
+    serial_init_pre_relocate();
+}
+
+void __init console_init_post_relocate(void)
+{
+    serial_init_post_relocate();
+}
+
 void __init console_init_irq(void)
 {
     serial_init_irq();
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 591a00900869..95f7410afa9c 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -447,6 +447,24 @@ void __init serial_init_preirq(void)
             com[i].driver->init_preirq(&com[i]);
 }
 
+void __init serial_init_pre_relocate(void)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(com); i++ )
+        if ( com[i].driver && com[i].driver->init_pre_relocate )
+            com[i].driver->init_pre_relocate(&com[i]);
+}
+
+void __init serial_init_post_relocate(void)
+{
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(com); i++ )
+        if ( com[i].driver && com[i].driver->init_post_relocate )
+            com[i].driver->init_post_relocate(&com[i]);
+}
+
 void __init serial_init_irq(void)
 {
     unsigned int i;
diff --git a/xen/include/xen/console.h b/xen/include/xen/console.h
index 83cbc9fbdafc..d563777ad9e2 100644
--- a/xen/include/xen/console.h
+++ b/xen/include/xen/console.h
@@ -18,6 +18,8 @@ void console_init_preirq(void);
 void console_init_ring(void);
 void console_init_irq(void);
 void console_init_postirq(void);
+void console_init_pre_relocate(void);
+void console_init_post_relocate(void);
 void console_endboot(void);
 int console_has(const char *device);
 
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 63a82b032dde..1ee3df2624fb 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -64,6 +64,9 @@ struct uart_driver {
     void (*init_preirq)(struct serial_port *port);
     void (*init_irq)(struct serial_port *port);
     void (*init_postirq)(struct serial_port *port);
+    /* Hooks around optional Xen relocation. */
+    void (*init_pre_relocate)(struct serial_port *port);
+    void (*init_post_relocate)(struct serial_port *port);
     /* Hook to clean up after Xen bootstrap (before domain 0 runs). */
     void (*endboot)(struct serial_port *port);
     /* Driver suspend/resume. */
@@ -103,6 +106,9 @@ struct uart_driver {
 void serial_init_preirq(void);
 void serial_init_irq(void);
 void serial_init_postirq(void);
+/* Notify drivers about Xen relocation (relevant for those using DMA). */
+void serial_init_pre_relocate(void);
+void serial_init_post_relocate(void);
 
 /* Clean-up hook before domain 0 runs. */
 void serial_endboot(void);
-- 
git-series 0.9.1

[PATCH v1 2/5] drivers/char: Handle Xen relocation in the XHCI console driver

[Marek Marczykowski-Górecki]

The XHCI uses DMA for a bunch of configuration structures and also for
transfer rings. Since those buffers live in .bss it's sensitive for Xen
relocation. Use the newly added hooks to handle this case:

In pre-relocation hook wait for all the data already sent to be handled
and pause sending any more.

In the post-relocation hook detect if relocation happened (check if
physical address of one of the structures matches what was programmed
into hardware) and if so - re-initialize all structures carying
physical addresses and program XHCI with new addresses. And then resume
console - this needs to happen in no-relocation case too, to undo
pausing done in pre-relocation.

Move the iommu_add_extra_reserved_device_memory() call post relocation,
as it needs physical addresses. It needs to happen before setting up
IOMMU (specifically before the acpi_iommu_init() call) but that's the
only ordering constraint - moving it is simpler than doing it initially
with pre-relocation addresses and then un-doing during relocation. This
is also the place where calling post-relocation hook unconditionally
(even if relocation didn't actually happened) is helpful - otherwise the
iommu_add_extra_reserved_device_memory() call would need to be done
conditionally in two places.

Finally, move dbc_dma_bufs declaration near top of the file, as it's
used earlier now.

Unfortunately, changes to several registers require flipping DCE (Debug
Capability Enable) to 0 and then back to 1 which results in the device
disconnect for a short time. Linux's xhci_dbc driver appears to do some
synchronization (or buffering?) so if one re-connects to the /dev/USB0
fast enough (for example by running minicom/picocom/etc in a loop), no
messages are lost. But technically there is no guarantee of that...

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
I tried to avoid flipping DCE, but it seems to be a limitation actually
enforced by the hardware. The XHCI spec says "just" this about a bunch
of registers:

    Software shall initialize this register before setting the Debug
    Capability Enable bit in the Debug Capability Control Register to ‘1’.

As for the implicit console flush in pre-relocation hook, technically it
could be avoided in the no-relocation case, but that would complicate
code structure (see note about reserved device memory). For the
relocation case, avoiding it might be possible, if the driver could
access old pages somehow (as without the flush the device might have
modified them in the meantime), but again - IMO it's not worth it.
Alternative would be flipping DCE also in the no-relocation case, which
is IMO worse than just the flush.
---
 xen/drivers/char/xhci-dbc.c | 89 +++++++++++++++++++++++++++-----------
 1 file changed, 64 insertions(+), 25 deletions(-)

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index c45e4b6825cc..c4bb371ff78f 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -264,6 +264,24 @@ struct dbc {
     uint16_t pci_cr;
 };
 
+/* Those are accessed via DMA. */
+struct dbc_dma_bufs {
+    struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
+    struct xhci_trb out_trb[DBC_TRB_RING_CAP];
+    struct xhci_trb in_trb[DBC_TRB_RING_CAP];
+    uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
+    uint8_t in_wrk_buf[DBC_WORK_RING_CAP];
+    struct xhci_erst_segment erst __aligned(16);
+    struct xhci_dbc_ctx ctx __aligned(16);
+    struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+    /*
+     * Don't place anything else on this page - it will be
+     * DMA-reachable by the USB controller.
+     */
+};
+static struct dbc_dma_bufs __section(".bss.page_aligned") __aligned(PAGE_SIZE)
+    dbc_dma_bufs;
+
 static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
 {
     size_t i;
@@ -1189,6 +1207,50 @@ static void __init cf_check dbc_uart_init_preirq(struct serial_port *port)
     uart->lock = &port->tx_lock;
 }
 
+static void __init cf_check dbc_uart_init_pre_relocate(struct serial_port *port)
+{
+    struct dbc_uart *uart = port->uart;
+    struct dbc *dbc = &uart->dbc;
+
+    /* Wait for all the data already sent to be handled. */
+    while ( xhci_trb_ring_size(&dbc->dbc_oring) )
+        dbc_pop_events(dbc);
+    /* Do not send any more data until after relocation. */
+    dbc->suspended = true;
+}
+
+static void __init cf_check dbc_uart_init_post_relocate(struct serial_port *port)
+{
+    struct dbc_uart *uart = port->uart;
+    struct dbc *dbc = &uart->dbc;
+
+    if ( readq(&dbc->dbc_reg->erstba) != virt_to_maddr(dbc->dbc_erst) )
+    {
+        /*
+         * Do not use dbc_init_work_ring() to not discard queued data, just
+         * update the DMA address.
+         */
+        dbc->dbc_owork.dma = virt_to_maddr(dbc->dbc_owork.buf);
+        dbc->dbc_iwork.dma = virt_to_maddr(dbc->dbc_iwork.buf);
+
+        if ( !dbc_init_dbc(dbc) )
+        {
+            dbc_error("relocate failed\n");
+            return;
+        }
+
+        dbc_enable_dbc(dbc);
+    }
+
+    dbc->suspended = false;
+
+    iommu_add_extra_reserved_device_memory(
+            PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
+            PFN_UP(sizeof(dbc_dma_bufs)),
+            uart->dbc.sbdf,
+            "XHCI console");
+}
+
 static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
 {
     struct dbc_uart *uart = port->uart;
@@ -1310,6 +1372,8 @@ static void cf_check dbc_uart_resume(struct serial_port *port)
 static struct uart_driver dbc_uart_driver = {
     .init_preirq = dbc_uart_init_preirq,
     .init_postirq = dbc_uart_init_postirq,
+    .init_pre_relocate = dbc_uart_init_pre_relocate,
+    .init_post_relocate = dbc_uart_init_post_relocate,
     .tx_ready = dbc_uart_tx_ready,
     .putc = dbc_uart_putc,
     .getc = dbc_uart_getc,
@@ -1318,24 +1382,6 @@ static struct uart_driver dbc_uart_driver = {
     .resume = dbc_uart_resume,
 };
 
-/* Those are accessed via DMA. */
-struct dbc_dma_bufs {
-    struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
-    struct xhci_trb out_trb[DBC_TRB_RING_CAP];
-    struct xhci_trb in_trb[DBC_TRB_RING_CAP];
-    uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
-    uint8_t in_wrk_buf[DBC_WORK_RING_CAP];
-    struct xhci_erst_segment erst __aligned(16);
-    struct xhci_dbc_ctx ctx __aligned(16);
-    struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
-    /*
-     * Don't place anything else on this page - it will be
-     * DMA-reachable by the USB controller.
-     */
-};
-static struct dbc_dma_bufs __section(".bss.page_aligned") __aligned(PAGE_SIZE)
-    dbc_dma_bufs;
-
 static int __init cf_check xhci_parse_dbgp(const char *opt_dbgp)
 {
     struct dbc_uart *uart = &dbc_uart;
@@ -1425,14 +1471,7 @@ void __init xhci_dbc_uart_init(void)
     dbc->dbc_str = dbc_dma_bufs.str_buf;
 
     if ( dbc_open(dbc) )
-    {
-        iommu_add_extra_reserved_device_memory(
-                PFN_DOWN(virt_to_maddr(&dbc_dma_bufs)),
-                PFN_UP(sizeof(dbc_dma_bufs)),
-                uart->dbc.sbdf,
-                "XHCI console");
         serial_register_uart(SERHND_XHCI, &dbc_uart_driver, &dbc_uart);
-    }
 }
 
 #ifdef DBC_DEBUG
-- 
git-series 0.9.1

[PATCH v1 3/5] drivers/char: make dbc_uart_dump() a bit more useful

[Marek Marczykowski-Górecki]

Make it safe to call also if xhci console is not enabled. And make it
non-static, to require one less modification when actually using it.
When using it, one still needs to add its declaration in some header
(or just next to the call site).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
IIUC Misra would not be happy about a declaration of an usused function.
And I'd rather avoid extending DBC_DEBUG scope beyond that single file.
---
 xen/drivers/char/xhci-dbc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index c4bb371ff78f..ced28cae0a29 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1498,11 +1498,14 @@ static void dbc_dump(struct dbc *dbc)
               readq(&r->cp) == virt_to_maddr(dbc->dbc_ctx));
 }
 
-static void dbc_uart_dump(void)
+void dbc_uart_dump(void)
 {
     struct dbc_uart *uart = &dbc_uart;
     struct dbc *dbc = &uart->dbc;
 
+    if ( !dbc->enable )
+        return;
+
     dbc_dump(dbc);
 }
 #endif
-- 
git-series 0.9.1

[PATCH v1 4/5] drivers/char: remove outdated comment in xhci driver

[Marek Marczykowski-Górecki]

The input handling is already implemented, and that limitation is not
there anymore.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/char/xhci-dbc.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index ced28cae0a29..3692776cec11 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -672,10 +672,6 @@ static void dbc_rx_trb(struct dbc *dbc, struct xhci_trb *trb,
         cache_flush(&ring->buf[start], end - start);
 }
 
-/*
- * Note that if IN transfer support is added, then this
- * will need to be changed; it assumes an OUT transfer ring only
- */
 static void dbc_pop_events(struct dbc *dbc)
 {
     struct dbc_reg *reg = dbc->dbc_reg;
-- 
git-series 0.9.1

[PATCH v1 5/5] console: support multiple serial console simultaneously

[Marek Marczykowski-Górecki]

Previously only one serial console was supported at the same time. Using
console=com1,dbgp,vga silently ignored all but last serial console (in
this case: only dbgp and vga were active).

Fix this by storing not a single sercon_handle, but an array of them, up
to MAX_SERCONS entries. The value of MAX_SERCONS can be chosen in
kconfig, the default (4) is arbitrary, inspired by the number of
SERHND_IDX values.

Make console_steal() aware of multiple consoles too. It can now either
steal output from specific console (for gdbstub), or from all of them at
once (for console suspend).

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
This was posted before as part of initial xhci console submission, it
reached v6 (but last changes were in v4), but wasn't considered useful
enough to review/ack:
https://lore.kernel.org/xen-devel/Yu0XHUhsebE+WG0g@mail-itl/

Since I needed this feature again, to debug xhci console issue, I'm
including this patch again in the series.

Changes in v4:
- use unsigned int for loop counters
- other minor changes
Changes in v3:
- adjust console_steal() for multiple consoles too
- add MAX_SERCONS to kconfig
- add warning about sync_console impact
- add warning if too many consoles are configured
- log issue with PCI spec parsing
---
 docs/misc/xen-command-line.pandoc |  4 +-
 xen/drivers/char/Kconfig          | 11 ++++-
 xen/drivers/char/console.c        | 98 ++++++++++++++++++++++++--------
 xen/include/xen/serial.h          |  1 +-
 4 files changed, 92 insertions(+), 22 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index b0eadd2c5d58..052c01f87bfc 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -456,6 +456,9 @@ only available when used together with `pv-in-pvh`.
 `none` indicates that Xen should not use a console.  This option only
 makes sense on its own.
 
+Specifying more than one serial console will increase console latency,
+especially when `sync_console` option is used.
+
 ### console_timestamps
 > `= none | date | datems | boot | raw`
 
@@ -2637,6 +2640,7 @@ Intel SA-00982.  Intel suggest that some workloads will benefit from this.
 
 Flag to force synchronous console output.  Useful for debugging, but
 not suitable for production environments due to incurred overhead.
+If multiple consoles are configured, the incurred overhead is even bigger.
 
 ### tboot (x86)
 > `= 0x<phys_addr>`
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e6e12bb41397..76305fcb4afa 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 MAX_SERCONS
+	int "Maximum number of serial consoles active at once"
+	default 4
+	help
+	  Controls how many serial consoles can be active at once. Configuring more
+	  using `console=` parameter will be ignored.
+	  When multiple consoles are configured, overhead of `sync_console` option
+	  is even bigger.
+
+	  Default value is 4.
+
 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 12898b684b5e..e306986bfcbc 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -134,7 +134,9 @@ static char *__read_mostly conring = _conring;
 static uint32_t __read_mostly conring_size = _CONRING_SIZE;
 static uint32_t conringc, conringp;
 
-static int __read_mostly sercon_handle = -1;
+#define MAX_SERCONS CONFIG_MAX_SERCONS
+static int __read_mostly sercon_handle[MAX_SERCONS];
+static unsigned int __read_mostly nr_sercon_handle = 0;
 
 #ifdef CONFIG_X86
 /* Tristate: 0 disabled, 1 user enabled, -1 default enabled */
@@ -424,32 +426,61 @@ long read_console_ring(struct xen_sysctl_readconsole *op)
 static char serial_rx_ring[SERIAL_RX_SIZE];
 static unsigned int serial_rx_cons, serial_rx_prod;
 
-static void (*serial_steal_fn)(const char *str, size_t nr) = early_puts;
+/* The last entry means "steal from all consoles" */
+static void (*serial_steal_fn[])(const char *str, size_t nr) = {
+    [MAX_SERCONS] = early_puts,
+};
 
+/*
+ * Redirect console *handle* output to *fn*. Use SERHND_STEAL_ALL as *handle* to
+ * redirect all the consoles. 
+ */
 int console_steal(int handle, void (*fn)(const char *str, size_t nr))
 {
-    if ( (handle == -1) || (handle != sercon_handle) )
-        return 0;
+    unsigned int i;
+
+    if ( handle == -1 )
+        return -ENOENT;
+    if ( serial_steal_fn[MAX_SERCONS] != NULL )
+        return -EBUSY;
+    if ( handle == SERHND_STEAL_ALL )
+    {
+        serial_steal_fn[MAX_SERCONS] = fn;
+        return MAX_SERCONS;
+    }
+    for ( i = 0; i < nr_sercon_handle; i++ )
+        if ( handle == sercon_handle[i] )
+            break;
+    if ( i == nr_sercon_handle )
+        return -ENOENT;
 
-    if ( serial_steal_fn != NULL )
+    if ( serial_steal_fn[i] != NULL )
         return -EBUSY;
 
-    serial_steal_fn = fn;
-    return 1;
+    serial_steal_fn[i] = fn;
+    return i;
 }
 
 void console_giveback(int id)
 {
-    if ( id == 1 )
-        serial_steal_fn = NULL;
+    if ( id >= 0 && id <= MAX_SERCONS )
+        serial_steal_fn[id] = NULL;
 }
 
 void console_serial_puts(const char *s, size_t nr)
 {
-    if ( serial_steal_fn != NULL )
-        serial_steal_fn(s, nr);
+    unsigned int i;
+
+    if ( serial_steal_fn[MAX_SERCONS] != NULL )
+        serial_steal_fn[MAX_SERCONS](s, nr);
     else
-        serial_puts(sercon_handle, s, nr);
+        for ( i = 0; i < nr_sercon_handle; i++ )
+        {
+            if ( serial_steal_fn[i] != NULL )
+                serial_steal_fn[i](s, nr);
+            else
+                serial_puts(sercon_handle[i], s, nr);
+        }
 }
 
 /*
@@ -1026,6 +1057,7 @@ void __init console_init_preirq(void)
 {
     char *p;
     int sh;
+    unsigned int i;
 
     serial_init_preirq();
 
@@ -1046,8 +1078,12 @@ void __init console_init_preirq(void)
             continue;
         else if ( (sh = serial_parse_handle(p)) >= 0 )
         {
-            sercon_handle = sh;
-            serial_steal_fn = NULL;
+            if ( nr_sercon_handle < MAX_SERCONS )
+                sercon_handle[nr_sercon_handle++] = sh;
+            else
+                printk("Too many consoles (max %d), ignoring '%s'\n",
+                       MAX_SERCONS, p);
+            serial_steal_fn[MAX_SERCONS] = NULL;
         }
         else
         {
@@ -1065,7 +1101,8 @@ void __init console_init_preirq(void)
         opt_console_xen = 0;
 #endif
 
-    serial_set_rx_handler(sercon_handle, serial_rx);
+    for ( i = 0; i < nr_sercon_handle; i++ )
+        serial_set_rx_handler(sercon_handle[i], serial_rx);
     pv_console_set_rx_handler(serial_rx);
 
     /* NB: send conring contents to all enabled physical consoles, if any */
@@ -1084,7 +1121,8 @@ void __init console_init_preirq(void)
 
     if ( opt_sync_console )
     {
-        serial_start_sync(sercon_handle);
+        for ( i = 0; i < nr_sercon_handle; i++ )
+            serial_start_sync(sercon_handle[i]);
         add_taint(TAINT_SYNC_CONSOLE);
         printk("Console output is synchronous.\n");
         warning_add(warning_sync_console);
@@ -1201,13 +1239,19 @@ int __init console_has(const char *device)
 
 void console_start_log_everything(void)
 {
-    serial_start_log_everything(sercon_handle);
+    unsigned int i;
+
+    for ( i = 0; i < nr_sercon_handle; i++ )
+        serial_start_log_everything(sercon_handle[i]);
     atomic_inc(&print_everything);
 }
 
 void console_end_log_everything(void)
 {
-    serial_end_log_everything(sercon_handle);
+    unsigned int i;
+
+    for ( i = 0; i < nr_sercon_handle; i++ )
+        serial_end_log_everything(sercon_handle[i]);
     atomic_dec(&print_everything);
 }
 
@@ -1223,23 +1267,32 @@ void console_unlock_recursive_irqrestore(unsigned long flags)
 
 void console_force_unlock(void)
 {
+    unsigned int i;
+
     watchdog_disable();
     spin_debug_disable();
     rspin_lock_init(&console_lock);
-    serial_force_unlock(sercon_handle);
+    for ( i = 0 ; i < nr_sercon_handle ; i++ )
+        serial_force_unlock(sercon_handle[i]);
     conring_no_notify = true;
     console_start_sync();
 }
 
 void console_start_sync(void)
 {
+    unsigned int i;
+
     atomic_inc(&print_everything);
-    serial_start_sync(sercon_handle);
+    for ( i = 0 ; i < nr_sercon_handle ; i++ )
+        serial_start_sync(sercon_handle[i]);
 }
 
 void console_end_sync(void)
 {
-    serial_end_sync(sercon_handle);
+    unsigned int i;
+
+    for ( i = 0; i < nr_sercon_handle; i++ )
+        serial_end_sync(sercon_handle[i]);
     atomic_dec(&print_everything);
 }
 
@@ -1362,7 +1415,8 @@ static int suspend_steal_id;
 
 int console_suspend(void)
 {
-    suspend_steal_id = console_steal(sercon_handle, suspend_steal_fn);
+    if ( nr_sercon_handle )
+        suspend_steal_id = console_steal(SERHND_STEAL_ALL, suspend_steal_fn);
     serial_suspend();
     return 0;
 }
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 1ee3df2624fb..31159814d883 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -101,6 +101,7 @@ struct uart_driver {
 #define SERHND_HI       (1<<2) /* Mux/demux each transferred char by MSB. */
 #define SERHND_LO       (1<<3) /* Ditto, except that the MSB is cleared.  */
 #define SERHND_COOKED   (1<<4) /* Newline/carriage-return translation?    */
+#define SERHND_STEAL_ALL 0xff  /* Synthetic handle used in console_steal() */
 
 /* Three-stage initialisation (before/during/after IRQ-subsystem setup). */
 void serial_init_preirq(void);
-- 
git-series 0.9.1

Re: [PATCH v1 1/5] console: add relocation hook

[Andrew Cooper]

On 25/05/2025 3:15 pm, Marek Marczykowski-Górecki wrote:
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 25189541244d..3ef819a252e4 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
>          highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
>  #endif
>  
> +    console_init_pre_relocate();
> +
>      /*
>       * Iterate backwards over all superpage-aligned RAM regions.
>       *
> @@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
>      if ( !xen_phys_start )
>          panic("Not enough memory to relocate Xen\n");
>  
> +    /*
> +     * Notify console drivers about relocation, before reusing old Xen's
> +     * memory.
> +     */
> +    console_init_post_relocate();
> +

With reference to the next patch, there are printk()'s in this region
which want to work (in case something goes very wrong), so I don't think
setting dbc->suspended is the best approach.

In dbc_uart_init_pre_relocate(), you just wait for all transfers to
complete.  Can't this be merged with post_relocate(), at which point the
intermediate printk()'s will work too?  It will also remove a hook.

Meanwhile, we have things like:

    /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
    this_cpu(gdt_l1e) =
        l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
    if ( IS_ENABLED(CONFIG_PV32) )
        this_cpu(compat_gdt_l1e) =
            l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);


in traps_init() which really doesn't belong here, but does belong in
some kind of general "relocation done" mechanism.

I wonder if we want a new type of initcall for this, allowing individual
areas of code to opt in with less boilerpate?

~Andrew

Re: [PATCH v1 1/5] console: add relocation hook

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2735 bytes --]

On Mon, May 26, 2025 at 04:08:17PM +0100, Andrew Cooper wrote:
> On 25/05/2025 3:15 pm, Marek Marczykowski-Górecki wrote:
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 25189541244d..3ef819a252e4 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
> >          highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
> >  #endif
> >  
> > +    console_init_pre_relocate();
> > +
> >      /*
> >       * Iterate backwards over all superpage-aligned RAM regions.
> >       *
> > @@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
> >      if ( !xen_phys_start )
> >          panic("Not enough memory to relocate Xen\n");
> >  
> > +    /*
> > +     * Notify console drivers about relocation, before reusing old Xen's
> > +     * memory.
> > +     */
> > +    console_init_post_relocate();
> > +
> 
> With reference to the next patch, there are printk()'s in this region
> which want to work (in case something goes very wrong), so I don't think
> setting dbc->suspended is the best approach.

I guess the post_relocate hook might be moved a bit earlier, but still,
once relocation happens, the xhci console is not functional until it
gets relocated too (for example - it would post new TRBs into a ring
that isn't actually monitored by the controller).

> In dbc_uart_init_pre_relocate(), you just wait for all transfers to
> complete.  Can't this be merged with post_relocate(), at which point the
> intermediate printk()'s will work too?

Not really, because the structure that driver watches is not the one
that the controller DMA into anymore... The driver would need to watch
the old physical pages (specifically, the event ring buffer there).

>   It will also remove a hook.
> 
> Meanwhile, we have things like:
> 
>     /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
>     this_cpu(gdt_l1e) =
>         l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
>     if ( IS_ENABLED(CONFIG_PV32) )
>         this_cpu(compat_gdt_l1e) =
>             l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);
> 
> 
> in traps_init() which really doesn't belong here, but does belong in
> some kind of general "relocation done" mechanism.
> 
> I wonder if we want a new type of initcall for this, allowing individual
> areas of code to opt in with less boilerpate?

Maybe? But for the console specifically, we also need pre-relocation
hook (more context also in the next patch description).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/5] console: add relocation hook

[Jan Beulich]

On 26.05.2025 17:39, Marek Marczykowski-Górecki wrote:
> On Mon, May 26, 2025 at 04:08:17PM +0100, Andrew Cooper wrote:
>> On 25/05/2025 3:15 pm, Marek Marczykowski-Górecki wrote:
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 25189541244d..3ef819a252e4 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>>          highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
>>>  #endif
>>>  
>>> +    console_init_pre_relocate();
>>> +
>>>      /*
>>>       * Iterate backwards over all superpage-aligned RAM regions.
>>>       *
>>> @@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
>>>      if ( !xen_phys_start )
>>>          panic("Not enough memory to relocate Xen\n");
>>>  
>>> +    /*
>>> +     * Notify console drivers about relocation, before reusing old Xen's
>>> +     * memory.
>>> +     */
>>> +    console_init_post_relocate();
>>> +
>>
>> With reference to the next patch, there are printk()'s in this region
>> which want to work (in case something goes very wrong), so I don't think
>> setting dbc->suspended is the best approach.
> 
> I guess the post_relocate hook might be moved a bit earlier, but still,
> once relocation happens, the xhci console is not functional until it
> gets relocated too (for example - it would post new TRBs into a ring
> that isn't actually monitored by the controller).

Why is it that this ring is dependent upon Xen's position? If the ring was
dynamically allocated, it wouldn't change position when Xen is moved.

Jan

Re: [PATCH v1 4/5] drivers/char: remove outdated comment in xhci driver

[Jan Beulich]

On 25.05.2025 16:15, Marek Marczykowski-Górecki wrote:
> The input handling is already implemented, and that limitation is not
> there anymore.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v1 1/5] console: add relocation hook

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
> On 26.05.2025 17:39, Marek Marczykowski-Górecki wrote:
> > On Mon, May 26, 2025 at 04:08:17PM +0100, Andrew Cooper wrote:
> >> On 25/05/2025 3:15 pm, Marek Marczykowski-Górecki wrote:
> >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> >>> index 25189541244d..3ef819a252e4 100644
> >>> --- a/xen/arch/x86/setup.c
> >>> +++ b/xen/arch/x86/setup.c
> >>> @@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
> >>>          highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
> >>>  #endif
> >>>  
> >>> +    console_init_pre_relocate();
> >>> +
> >>>      /*
> >>>       * Iterate backwards over all superpage-aligned RAM regions.
> >>>       *
> >>> @@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
> >>>      if ( !xen_phys_start )
> >>>          panic("Not enough memory to relocate Xen\n");
> >>>  
> >>> +    /*
> >>> +     * Notify console drivers about relocation, before reusing old Xen's
> >>> +     * memory.
> >>> +     */
> >>> +    console_init_post_relocate();
> >>> +
> >>
> >> With reference to the next patch, there are printk()'s in this region
> >> which want to work (in case something goes very wrong), so I don't think
> >> setting dbc->suspended is the best approach.
> > 
> > I guess the post_relocate hook might be moved a bit earlier, but still,
> > once relocation happens, the xhci console is not functional until it
> > gets relocated too (for example - it would post new TRBs into a ring
> > that isn't actually monitored by the controller).
> 
> Why is it that this ring is dependent upon Xen's position? If the ring was
> dynamically allocated, it wouldn't change position when Xen is moved.

The console is setup quite early, I don't think I can allocate memory at
this stage, no?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/5] console: add relocation hook

[Jan Beulich]

On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
>> On 26.05.2025 17:39, Marek Marczykowski-Górecki wrote:
>>> On Mon, May 26, 2025 at 04:08:17PM +0100, Andrew Cooper wrote:
>>>> On 25/05/2025 3:15 pm, Marek Marczykowski-Górecki wrote:
>>>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>>>> index 25189541244d..3ef819a252e4 100644
>>>>> --- a/xen/arch/x86/setup.c
>>>>> +++ b/xen/arch/x86/setup.c
>>>>> @@ -1481,6 +1481,8 @@ void asmlinkage __init noreturn __start_xen(void)
>>>>>          highmem_start &= ~((1UL << L3_PAGETABLE_SHIFT) - 1);
>>>>>  #endif
>>>>>  
>>>>> +    console_init_pre_relocate();
>>>>> +
>>>>>      /*
>>>>>       * Iterate backwards over all superpage-aligned RAM regions.
>>>>>       *
>>>>> @@ -1606,6 +1608,12 @@ void asmlinkage __init noreturn __start_xen(void)
>>>>>      if ( !xen_phys_start )
>>>>>          panic("Not enough memory to relocate Xen\n");
>>>>>  
>>>>> +    /*
>>>>> +     * Notify console drivers about relocation, before reusing old Xen's
>>>>> +     * memory.
>>>>> +     */
>>>>> +    console_init_post_relocate();
>>>>> +
>>>>
>>>> With reference to the next patch, there are printk()'s in this region
>>>> which want to work (in case something goes very wrong), so I don't think
>>>> setting dbc->suspended is the best approach.
>>>
>>> I guess the post_relocate hook might be moved a bit earlier, but still,
>>> once relocation happens, the xhci console is not functional until it
>>> gets relocated too (for example - it would post new TRBs into a ring
>>> that isn't actually monitored by the controller).
>>
>> Why is it that this ring is dependent upon Xen's position? If the ring was
>> dynamically allocated, it wouldn't change position when Xen is moved.
> 
> The console is setup quite early, I don't think I can allocate memory at
> this stage, no?

But you allocate before Xen is moved, I suppose.

Jan

Re: [PATCH v1 1/5] console: add relocation hook

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

On Thu, Jun 05, 2025 at 06:05:02PM +0200, Jan Beulich wrote:
> On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
> > On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
> >> Why is it that this ring is dependent upon Xen's position? If the ring was
> >> dynamically allocated, it wouldn't change position when Xen is moved.
> > 
> > The console is setup quite early, I don't think I can allocate memory at
> > this stage, no?
> 
> But you allocate before Xen is moved, I suppose.

Well, I have those buffers in BSS exactly to avoid the need to allocate
them (or rather: have bootloader allocate them for me).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/5] console: add relocation hook

[Jan Beulich]

On 05.06.2025 18:08, Marek Marczykowski-Górecki wrote:
> On Thu, Jun 05, 2025 at 06:05:02PM +0200, Jan Beulich wrote:
>> On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
>>> On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
>>>> Why is it that this ring is dependent upon Xen's position? If the ring was
>>>> dynamically allocated, it wouldn't change position when Xen is moved.
>>>
>>> The console is setup quite early, I don't think I can allocate memory at
>>> this stage, no?
>>
>> But you allocate before Xen is moved, I suppose.
> 
> Well, I have those buffers in BSS exactly to avoid the need to allocate
> them (or rather: have bootloader allocate them for me).

Yet them remaining in .bss is now getting in the way. Imo moving them to
.init.* and adding proper allocation is going to be easier than the hook-
ary you are proposing.

Jan

Re: [PATCH v1 1/5] console: add relocation hook

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

On Fri, Jun 06, 2025 at 08:26:33AM +0200, Jan Beulich wrote:
> On 05.06.2025 18:08, Marek Marczykowski-Górecki wrote:
> > On Thu, Jun 05, 2025 at 06:05:02PM +0200, Jan Beulich wrote:
> >> On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
> >>> On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
> >>>> Why is it that this ring is dependent upon Xen's position? If the ring was
> >>>> dynamically allocated, it wouldn't change position when Xen is moved.
> >>>
> >>> The console is setup quite early, I don't think I can allocate memory at
> >>> this stage, no?
> >>
> >> But you allocate before Xen is moved, I suppose.
> > 
> > Well, I have those buffers in BSS exactly to avoid the need to allocate
> > them (or rather: have bootloader allocate them for me).
> 
> Yet them remaining in .bss is now getting in the way. Imo moving them to
> .init.* and adding proper allocation is going to be easier than the hook-
> ary you are proposing.

So, when would you propose to allocate them? Wouldn't that be yet
another hook?

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/5] console: add relocation hook

[Jan Beulich]

On 06.06.2025 17:54, Marek Marczykowski-Górecki wrote:
> On Fri, Jun 06, 2025 at 08:26:33AM +0200, Jan Beulich wrote:
>> On 05.06.2025 18:08, Marek Marczykowski-Górecki wrote:
>>> On Thu, Jun 05, 2025 at 06:05:02PM +0200, Jan Beulich wrote:
>>>> On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
>>>>> On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
>>>>>> Why is it that this ring is dependent upon Xen's position? If the ring was
>>>>>> dynamically allocated, it wouldn't change position when Xen is moved.
>>>>>
>>>>> The console is setup quite early, I don't think I can allocate memory at
>>>>> this stage, no?
>>>>
>>>> But you allocate before Xen is moved, I suppose.
>>>
>>> Well, I have those buffers in BSS exactly to avoid the need to allocate
>>> them (or rather: have bootloader allocate them for me).
>>
>> Yet them remaining in .bss is now getting in the way. Imo moving them to
>> .init.* and adding proper allocation is going to be easier than the hook-
>> ary you are proposing.
> 
> So, when would you propose to allocate them? Wouldn't that be yet
> another hook?

Exactly one, yes. Given Andrew's earlier reply my understanding was that to
get things correct in your proposed model would end up requiring more than
one. However, the point in time where move_xen() is called is still too
early to allocate memory a (somewhat) normal way - even the boot allocator
gets seeded only later. So I guess console_init_preirq() may need to inform
its caller how much memory is going to be needed later on (and what address
constraints there are, if any). Then a suitable chunk would need setting
aside kind of like it's done for kexec. That address would then need to be
passed into the new hook.

Jan

Re: [PATCH v1 3/5] drivers/char: make dbc_uart_dump() a bit more useful

[Jan Beulich]

On 25.05.2025 16:15, Marek Marczykowski-Górecki wrote:
> Make it safe to call also if xhci console is not enabled. And make it
> non-static, to require one less modification when actually using it.
> When using it, one still needs to add its declaration in some header
> (or just next to the call site).
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> IIUC Misra would not be happy about a declaration of an usused function.
> And I'd rather avoid extending DBC_DEBUG scope beyond that single file.

It's not going to be happy about a non-static one without declaration
either. Misra-wise this is pretty much a no-go.

Jan

> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1498,11 +1498,14 @@ static void dbc_dump(struct dbc *dbc)
>                readq(&r->cp) == virt_to_maddr(dbc->dbc_ctx));
>  }
>  
> -static void dbc_uart_dump(void)
> +void dbc_uart_dump(void)
>  {
>      struct dbc_uart *uart = &dbc_uart;
>      struct dbc *dbc = &uart->dbc;
>  
> +    if ( !dbc->enable )
> +        return;
> +
>      dbc_dump(dbc);
>  }
>  #endif

Re: [PATCH v1 5/5] console: support multiple serial console simultaneously

[Jan Beulich]

On 25.05.2025 16:15, Marek Marczykowski-Górecki wrote:
> Previously only one serial console was supported at the same time. Using
> console=com1,dbgp,vga silently ignored all but last serial console (in
> this case: only dbgp and vga were active).
> 
> Fix this by storing not a single sercon_handle, but an array of them, up
> to MAX_SERCONS entries. The value of MAX_SERCONS can be chosen in
> kconfig, the default (4) is arbitrary, inspired by the number of
> SERHND_IDX values.
> 
> Make console_steal() aware of multiple consoles too. It can now either
> steal output from specific console (for gdbstub), or from all of them at
> once (for console suspend).
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
> This was posted before as part of initial xhci console submission, it
> reached v6 (but last changes were in v4), but wasn't considered useful
> enough to review/ack:
> https://lore.kernel.org/xen-devel/Yu0XHUhsebE+WG0g@mail-itl/
> 
> Since I needed this feature again, to debug xhci console issue, I'm
> including this patch again in the series.

Beyond this narrow aspect, has anything changed in the picture, compared
to what was said / discussed earlier on?

Jan

Re: [PATCH v1 1/5] console: add relocation hook

[Andrew Cooper]

On 10/06/2025 8:52 am, Jan Beulich wrote:
> On 06.06.2025 17:54, Marek Marczykowski-Górecki wrote:
>> On Fri, Jun 06, 2025 at 08:26:33AM +0200, Jan Beulich wrote:
>>> On 05.06.2025 18:08, Marek Marczykowski-Górecki wrote:
>>>> On Thu, Jun 05, 2025 at 06:05:02PM +0200, Jan Beulich wrote:
>>>>> On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
>>>>>> On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
>>>>>>> Why is it that this ring is dependent upon Xen's position? If the ring was
>>>>>>> dynamically allocated, it wouldn't change position when Xen is moved.
>>>>>> The console is setup quite early, I don't think I can allocate memory at
>>>>>> this stage, no?
>>>>> But you allocate before Xen is moved, I suppose.
>>>> Well, I have those buffers in BSS exactly to avoid the need to allocate
>>>> them (or rather: have bootloader allocate them for me).
>>> Yet them remaining in .bss is now getting in the way. Imo moving them to
>>> .init.* and adding proper allocation is going to be easier than the hook-
>>> ary you are proposing.
>> So, when would you propose to allocate them? Wouldn't that be yet
>> another hook?
> Exactly one, yes. Given Andrew's earlier reply my understanding was that to
> get things correct in your proposed model would end up requiring more than
> one. However, the point in time where move_xen() is called is still too
> early to allocate memory a (somewhat) normal way - even the boot allocator
> gets seeded only later. So I guess console_init_preirq() may need to inform
> its caller how much memory is going to be needed later on (and what address
> constraints there are, if any). Then a suitable chunk would need setting
> aside kind of like it's done for kexec. That address would then need to be
> passed into the new hook.

How about initialising the console using _va(_pa(xxx)) of the Xen
datastructures?

Xen is mapped into the directmap (pagetable handling depends on this),
so these pointers will work right from the very outset, and will
(intentionally) refer to the old position.

After relocation (specifically, before we free the old Xen image), we
can drain in-flight requests and update the pointers.

~Andrew

Re: [PATCH v1 1/5] console: add relocation hook

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

On Tue, Jun 10, 2025 at 01:54:04PM +0100, Andrew Cooper wrote:
> On 10/06/2025 8:52 am, Jan Beulich wrote:
> > On 06.06.2025 17:54, Marek Marczykowski-Górecki wrote:
> >> On Fri, Jun 06, 2025 at 08:26:33AM +0200, Jan Beulich wrote:
> >>> On 05.06.2025 18:08, Marek Marczykowski-Górecki wrote:
> >>>> On Thu, Jun 05, 2025 at 06:05:02PM +0200, Jan Beulich wrote:
> >>>>> On 05.06.2025 16:51, Marek Marczykowski-Górecki wrote:
> >>>>>> On Thu, Jun 05, 2025 at 04:42:53PM +0200, Jan Beulich wrote:
> >>>>>>> Why is it that this ring is dependent upon Xen's position? If the ring was
> >>>>>>> dynamically allocated, it wouldn't change position when Xen is moved.
> >>>>>> The console is setup quite early, I don't think I can allocate memory at
> >>>>>> this stage, no?
> >>>>> But you allocate before Xen is moved, I suppose.
> >>>> Well, I have those buffers in BSS exactly to avoid the need to allocate
> >>>> them (or rather: have bootloader allocate them for me).
> >>> Yet them remaining in .bss is now getting in the way. Imo moving them to
> >>> .init.* and adding proper allocation is going to be easier than the hook-
> >>> ary you are proposing.
> >> So, when would you propose to allocate them? Wouldn't that be yet
> >> another hook?
> > Exactly one, yes. Given Andrew's earlier reply my understanding was that to
> > get things correct in your proposed model would end up requiring more than
> > one. However, the point in time where move_xen() is called is still too
> > early to allocate memory a (somewhat) normal way - even the boot allocator
> > gets seeded only later. So I guess console_init_preirq() may need to inform
> > its caller how much memory is going to be needed later on (and what address
> > constraints there are, if any). Then a suitable chunk would need setting
> > aside kind of like it's done for kexec. That address would then need to be
> > passed into the new hook.
> 
> How about initialising the console using _va(_pa(xxx)) of the Xen
> datastructures?
> 
> Xen is mapped into the directmap (pagetable handling depends on this),
> so these pointers will work right from the very outset, and will
> (intentionally) refer to the old position.
> 
> After relocation (specifically, before we free the old Xen image), we
> can drain in-flight requests and update the pointers.

Ah, you mean to use directmap to access the ring pages. Yes, that should
work, and should be enough to not need the pre-relocate hook. The post
relocate would still be needed though (none of existing console init
hooks fits this purpose). I'll try that approach.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 5/5] console: support multiple serial console simultaneously

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]

On Tue, Jun 10, 2025 at 02:48:54PM +0200, Jan Beulich wrote:
> On 25.05.2025 16:15, Marek Marczykowski-Górecki wrote:
> > Previously only one serial console was supported at the same time. Using
> > console=com1,dbgp,vga silently ignored all but last serial console (in
> > this case: only dbgp and vga were active).
> > 
> > Fix this by storing not a single sercon_handle, but an array of them, up
> > to MAX_SERCONS entries. The value of MAX_SERCONS can be chosen in
> > kconfig, the default (4) is arbitrary, inspired by the number of
> > SERHND_IDX values.
> > 
> > Make console_steal() aware of multiple consoles too. It can now either
> > steal output from specific console (for gdbstub), or from all of them at
> > once (for console suspend).
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> > This was posted before as part of initial xhci console submission, it
> > reached v6 (but last changes were in v4), but wasn't considered useful
> > enough to review/ack:
> > https://lore.kernel.org/xen-devel/Yu0XHUhsebE+WG0g@mail-itl/
> > 
> > Since I needed this feature again, to debug xhci console issue, I'm
> > including this patch again in the series.
> 
> Beyond this narrow aspect, has anything changed in the picture, compared
> to what was said / discussed earlier on?

Not really. It's still quite useful for people doing console drivers
debugging and have the luxury of having many options at the same time.
For me it's xhci, but it could be useful also for debugging other
drivers (outside of x86 too). And it's still not very useful for anybody
else.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]