[PATCH v5 0/9] Add Xue - console over USB 3 Debug Capability

[PATCH v5 0/9] Add Xue - console over USB 3 Debug Capability

[Marek Marczykowski-Górecki]

This is integration of https://github.com/connojd/xue into mainline Xen.
This patch series includes several patches that I made in the process, some are
very loosely related.

The driver developed by Connor supports console via USB3 debug capability. The
capability is designed to operate mostly independently of normal XHCI driver,
so this patch series allows dom0 to drive standard USB3 controller part, while
Xen uses DbC for console output.

Changes since RFC:
 - move the driver to xue.c, remove non-Xen parts, remove now unneeded abstraction
 - adjust for Xen code style
 - build for x86 only
 - drop patch hidding the device from dom0
Changes since v1:
 - drop ehci patch - already applied
 - adjust for review comments from Jan (see changelogs in individual patches)
Changes since v2:
 - add runtime option to share (or not) the controller with dom0 or other domains
 - add RX support
 - several smaller changes according to review comments
Changes since v3:
 - put controller sharing behind experimental kconfig option
 - several other changes according to review comments
Changes since v4:
 - drop first 4 patches - already applied to staging
 - split dbgp=xhci into dbc=xhci

Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <george.dunlap@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Julien Grall <julien@xen.org>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Wei Liu <wl@xen.org>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Paul Durrant <paul@xen.org>
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Connor Davis <connojdavis@gmail.com>

Marek Marczykowski-Górecki (9):
  drivers/char: separate dbgp=xhci to dbc=xhci option
  console: support multiple serial console simultaneously
  IOMMU: add common API for device reserved memory
  IOMMU/VT-d: wire common device reserved memory API
  IOMMU/AMD: wire common device reserved memory API
  drivers/char: mark DMA buffers as reserved for the XHCI
  drivers/char: add RX support to the XHCI driver
  drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
  drivers/char: fix handling cable re-plug in XHCI console driver

 docs/misc/xen-command-line.pandoc        |  36 ++-
 xen/drivers/char/Kconfig                 |  11 +-
 xen/drivers/char/console.c               |  98 +++++--
 xen/drivers/char/serial.c                |   6 +-
 xen/drivers/char/xhci-dbc.c              | 335 +++++++++++++++++++++---
 xen/drivers/passthrough/amd/iommu_acpi.c |  21 ++-
 xen/drivers/passthrough/iommu.c          |  46 +++-
 xen/drivers/passthrough/vtd/dmar.c       | 201 ++++++++------
 xen/include/xen/iommu.h                  |  14 +-
 xen/include/xen/serial.h                 |   2 +-
 10 files changed, 623 insertions(+), 147 deletions(-)

base-commit: f6cd15188e097de1eb04855eb790a5f51c3ad71a
-- 
git-series 0.9.1

[PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Marek Marczykowski-Górecki]

This allows configuring EHCI and XHCI consoles separately,
simultaneously.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
new in v5
---
 docs/misc/xen-command-line.pandoc | 18 ++++++++++++------
 xen/drivers/char/serial.c         |  6 ++++++
 xen/drivers/char/xhci-dbc.c       | 20 ++++++++++----------
 xen/include/xen/serial.h          |  1 +
 4 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 9a79385a3712..0d07f0c75990 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -409,7 +409,7 @@ The following are examples of correct specifications:
 Specify the size of the console ring buffer.
 
 ### console
-> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | none ]`
+> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | dbc | none ]`
 
 > Default: `console=com1,vga`
 
@@ -428,7 +428,9 @@ cleared.  This allows a single port to be shared by two subsystems
 `pv` indicates that Xen should use Xen's PV console. This option is
 only available when used together with `pv-in-pvh`.
 
-`dbgp` indicates that Xen should use a USB debug port.
+`dbgp` indicates that Xen should use a USB2 debug port.
+
+`dbc` indicates that Xen should use a USB3 debug port.
 
 `none` indicates that Xen should not use a console.  This option only
 makes sense on its own.
@@ -721,14 +723,18 @@ Available alternatives, with their meaning, are:
 
 ### dbgp
 > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
+
+Specify the USB controller to use, either by instance number (when going
+over the PCI busses sequentially) or by PCI device (must be on segment 0).
+
+### dbc
 > `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
 
 Specify the USB controller to use, either by instance number (when going
 over the PCI busses sequentially) or by PCI device (must be on segment 0).
 
-Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability (output
-only). XHCI driver will wait indefinitely for the debug host to connect - make
-sure the cable is connected.
+Output only console. XHCI driver will wait indefinitely for the debug host to
+connect - make sure the cable is connected.
 
 ### debug_stack_lines
 > `= <integer>`
@@ -1174,7 +1180,7 @@ virtualization, to allow the L1 hypervisor to use EPT even if the L0 hypervisor
 does not provide `VM_ENTRY_LOAD_GUEST_PAT`.
 
 ### gdb
-> `= com1[H,L] | com2[H,L] | dbgp`
+> `= com1[H,L] | com2[H,L] | dbgp | dbc`
 
 > Default: ``
 
diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c
index 47899222cef8..7daaa61361bb 100644
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -311,6 +311,12 @@ int __init serial_parse_handle(const char *conf)
         goto common;
     }
 
+    if ( !strncmp(conf, "dbc", 3) && (!conf[3] || conf[3] == ',') )
+    {
+        handle = SERHND_DBC;
+        goto common;
+    }
+
     if ( !strncmp(conf, "dtuart", 6) )
     {
         handle = SERHND_DTUART;
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index ca7d4a62139e..eb35e3a2ee4f 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
 static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
 static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
 
-static char __initdata opt_dbgp[30];
+static char __initdata opt_dbc[30];
 
-string_param("dbgp", opt_dbgp);
+string_param("dbc", opt_dbc);
 
 void __init xhci_dbc_uart_init(void)
 {
@@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
     struct dbc *dbc = &uart->dbc;
     const char *e;
 
-    if ( strncmp(opt_dbgp, "xhci", 4) )
+    if ( strncmp(opt_dbc, "xhci", 4) )
         return;
 
     memset(dbc, 0, sizeof(*dbc));
 
-    if ( isdigit(opt_dbgp[4]) )
+    if ( isdigit(opt_dbc[4]) )
     {
-        dbc->xhc_num = simple_strtoul(opt_dbgp + 4, &e, 10);
+        dbc->xhc_num = simple_strtoul(opt_dbc + 4, &e, 10);
     }
-    else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
+    else if ( strncmp(opt_dbc + 4, "@pci", 4) == 0 )
     {
         unsigned int bus, slot, func;
 
-        e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
+        e = parse_pci(opt_dbc + 8, NULL, &bus, &slot, &func);
         if ( !e || *e )
         {
             printk(XENLOG_ERR
-                   "Invalid dbgp= PCI device spec: '%s'\n",
-                   opt_dbgp + 8);
+                   "Invalid dbc= PCI device spec: '%s'\n",
+                   opt_dbc + 8);
             return;
         }
 
@@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
     dbc->dbc_str = str_buf;
 
     if ( dbc_open(dbc) )
-        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
+        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
 }
 
 #ifdef DBC_DEBUG
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 4cd4ae5e6f1c..186afbed9c92 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -95,6 +95,7 @@ struct uart_driver {
 # define SERHND_COM1    (0<<0)
 # define SERHND_COM2    (1<<0)
 # define SERHND_DBGP    (2<<0)
+# define SERHND_DBC     (3<<0)
 # define SERHND_DTUART  (0<<0) /* Steal SERHND_COM1 value */
 #define SERHND_HI       (1<<2) /* Mux/demux each transferred char by MSB. */
 #define SERHND_LO       (1<<3) /* Ditto, except that the MSB is cleared.  */
-- 
git-series 0.9.1

[PATCH v5 2/9] 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>
---
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 0d07f0c75990..e31300ea3408 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -435,6 +435,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`
 
@@ -2409,6 +2412,7 @@ vulnerabilities.
 
 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 06350c387371..7b5ff0c414ec 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -85,6 +85,17 @@ config SERIAL_TX_BUFSIZE
 
 	  Default value is 16384 (16kiB).
 
+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 e8468c121ad0..60d42284f606 100644
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -113,7 +113,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 */
@@ -393,32 +395,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 *, size_t nr) = early_puts;
+/* The last entry means "steal from all consoles" */
+static void (*serial_steal_fn[])(const char *, 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 *, 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);
+        }
 
     /* Copy all serial output into PV console */
     pv_console_puts(s, nr);
@@ -957,6 +988,7 @@ void __init console_init_preirq(void)
 {
     char *p;
     int sh;
+    unsigned int i;
 
     serial_init_preirq();
 
@@ -977,8 +1009,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
         {
@@ -996,7 +1032,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);
 
     /* HELLO WORLD --- start-of-day banner text. */
@@ -1014,7 +1051,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);
@@ -1121,13 +1159,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);
 }
 
@@ -1149,23 +1193,32 @@ void console_unlock_recursive_irqrestore(unsigned long flags)
 
 void console_force_unlock(void)
 {
+    unsigned int i;
+
     watchdog_disable();
     spin_debug_disable();
     spin_lock_init(&console_lock);
-    serial_force_unlock(sercon_handle);
+    for ( i = 0 ; i < nr_sercon_handle ; i++ )
+        serial_force_unlock(sercon_handle[i]);
     console_locks_busted = 1;
     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);
 }
 
@@ -1291,7 +1344,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 186afbed9c92..31c825edb052 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -100,6 +100,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

[PATCH v5 3/9] IOMMU: add common API for device reserved memory

[Marek Marczykowski-Górecki]

Add API similar to rmrr= and ivmd= arguments, but in a common code. This
will allow drivers to register reserved memory regardless of the IOMMU
vendor.
The direct reason for this API is xhci-dbc console driver (aka xue),
that needs to use DMA. But future change may unify command line
arguments for user-supplied reserved memory, and it may be useful for
other drivers in the future too.

This commit just introduces an API, subsequent patches will plug it in
appropriate places. The reserved memory ranges needs to be saved
locally, because at the point when they are collected, Xen doesn't know
yet which IOMMU driver will be used.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v5:
- fix indentation, keep full "reserved_device_memory" for consistency
  with iommu_get_reserved_device_memory
Changes in v4:
- mark functions as __init
- use pci_sbdf_t type
Changes in v3:
 - adjust code style
---
 xen/drivers/passthrough/iommu.c | 46 ++++++++++++++++++++++++++++++++++-
 xen/include/xen/iommu.h         | 14 ++++++++++-
 2 files changed, 60 insertions(+)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 134cdb47e0dc..5e2a720d29b9 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -669,6 +669,52 @@ bool_t iommu_has_feature(struct domain *d, enum iommu_feature feature)
     return is_iommu_enabled(d) && test_bit(feature, dom_iommu(d)->features);
 }
 
+#define MAX_EXTRA_RESERVED_RANGES 20
+struct extra_reserved_range {
+    unsigned long start;
+    unsigned long nr;
+    pci_sbdf_t sbdf;
+};
+static unsigned int __initdata nr_extra_reserved_ranges;
+static struct extra_reserved_range __initdata
+    extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
+
+int __init iommu_add_extra_reserved_device_memory(unsigned long start,
+                                                  unsigned long nr,
+                                                  pci_sbdf_t sbdf)
+{
+    unsigned int idx;
+
+    if ( nr_extra_reserved_ranges >= MAX_EXTRA_RESERVED_RANGES )
+        return -ENOMEM;
+
+    idx = nr_extra_reserved_ranges++;
+    extra_reserved_ranges[idx].start = start;
+    extra_reserved_ranges[idx].nr = nr;
+    extra_reserved_ranges[idx].sbdf = sbdf;
+
+    return 0;
+}
+
+int __init iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
+                                                  void *ctxt)
+{
+    unsigned int idx;
+    int ret;
+
+    for ( idx = 0; idx < nr_extra_reserved_ranges; idx++ )
+    {
+        ret = func(extra_reserved_ranges[idx].start,
+                   extra_reserved_ranges[idx].nr,
+                   extra_reserved_ranges[idx].sbdf.sbdf,
+                   ctxt);
+        if ( ret < 0 )
+            return ret;
+    }
+
+    return 0;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
index 1240d7762d99..4f22fc1bed55 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -304,6 +304,20 @@ struct iommu_ops {
 #endif
 };
 
+/*
+ * To be called by Xen internally, to register extra RMRR/IVMD ranges.
+ * Needs to be called before IOMMU initialization.
+ */
+extern int iommu_add_extra_reserved_device_memory(unsigned long start,
+                                                  unsigned long nr,
+                                                  pci_sbdf_t sbdf);
+/*
+ * To be called by specific IOMMU driver during initialization,
+ * to fetch ranges registered with iommu_add_extra_reserved_device_memory().
+ */
+extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func,
+                                                  void *ctxt);
+
 #include <asm/iommu.h>
 
 #ifndef iommu_call
-- 
git-series 0.9.1

[PATCH v5 4/9] IOMMU/VT-d: wire common device reserved memory API

[Marek Marczykowski-Górecki]

Re-use rmrr= parameter handling code to handle common device reserved
memory.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- make MAX_USER_RMRR_PAGES applicable only to user-configured RMRR
---
 xen/drivers/passthrough/vtd/dmar.c | 201 +++++++++++++++++-------------
 1 file changed, 119 insertions(+), 82 deletions(-)

diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 367304c8739c..3df5f6b69719 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -861,111 +861,139 @@ static struct user_rmrr __initdata user_rmrrs[MAX_USER_RMRR];
 
 /* Macro for RMRR inclusive range formatting. */
 #define ERMRRU_FMT "[%lx-%lx]"
-#define ERMRRU_ARG(eru) eru.base_pfn, eru.end_pfn
+#define ERMRRU_ARG base_pfn, end_pfn
+
+static int __init add_one_user_rmrr(unsigned long base_pfn,
+                                    unsigned long end_pfn,
+                                    unsigned int dev_count,
+                                    uint32_t *sbdf);
 
 static int __init add_user_rmrr(void)
 {
+    unsigned int i;
+    int ret;
+
+    for ( i = 0; i < nr_rmrr; i++ )
+    {
+        ret = add_one_user_rmrr(user_rmrrs[i].base_pfn,
+                                user_rmrrs[i].end_pfn,
+                                user_rmrrs[i].dev_count,
+                                user_rmrrs[i].sbdf);
+        if ( ret < 0 )
+            return ret;
+    }
+    return 0;
+}
+
+/* Returns 1 on success, 0 when ignoring and < 0 on error. */
+static int __init add_one_user_rmrr(unsigned long base_pfn,
+                                    unsigned long end_pfn,
+                                    unsigned int dev_count,
+                                    uint32_t *sbdf)
+{
     struct acpi_rmrr_unit *rmrr, *rmrru;
-    unsigned int idx, seg, i;
-    unsigned long base, end;
+    unsigned int idx, seg;
+    unsigned long base_iter;
     bool overlap;
 
-    for ( i = 0; i < nr_rmrr; i++ )
+    if ( iommu_verbose )
+        printk(XENLOG_DEBUG VTDPREFIX
+               "Adding RMRR for %d device ([0]: %#x) range "ERMRRU_FMT"\n",
+               dev_count, sbdf[0], ERMRRU_ARG);
+
+    if ( base_pfn > end_pfn )
     {
-        base = user_rmrrs[i].base_pfn;
-        end = user_rmrrs[i].end_pfn;
+        printk(XENLOG_ERR VTDPREFIX
+               "Invalid RMRR Range "ERMRRU_FMT"\n",
+               ERMRRU_ARG);
+        return 0;
+    }
 
-        if ( base > end )
+    overlap = false;
+    list_for_each_entry(rmrru, &acpi_rmrr_units, list)
+    {
+        if ( pfn_to_paddr(base_pfn) <= rmrru->end_address &&
+             rmrru->base_address <= pfn_to_paddr(end_pfn) )
         {
             printk(XENLOG_ERR VTDPREFIX
-                   "Invalid RMRR Range "ERMRRU_FMT"\n",
-                   ERMRRU_ARG(user_rmrrs[i]));
-            continue;
+                   "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
+                   ERMRRU_ARG,
+                   paddr_to_pfn(rmrru->base_address),
+                   paddr_to_pfn(rmrru->end_address));
+            overlap = true;
+            break;
         }
+    }
+    /* Don't add overlapping RMRR. */
+    if ( overlap )
+        return 0;
 
-        if ( (end - base) >= MAX_USER_RMRR_PAGES )
+    base_iter = base_pfn;
+    do
+    {
+        if ( !mfn_valid(_mfn(base_iter)) )
         {
             printk(XENLOG_ERR VTDPREFIX
-                   "RMRR range "ERMRRU_FMT" exceeds "\
-                   __stringify(MAX_USER_RMRR_PAGES)" pages\n",
-                   ERMRRU_ARG(user_rmrrs[i]));
-            continue;
+                   "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
+                   ERMRRU_ARG);
+            break;
         }
+    } while ( base_iter++ < end_pfn );
 
-        overlap = false;
-        list_for_each_entry(rmrru, &acpi_rmrr_units, list)
-        {
-            if ( pfn_to_paddr(base) <= rmrru->end_address &&
-                 rmrru->base_address <= pfn_to_paddr(end) )
-            {
-                printk(XENLOG_ERR VTDPREFIX
-                       "Overlapping RMRRs: "ERMRRU_FMT" and [%lx-%lx]\n",
-                       ERMRRU_ARG(user_rmrrs[i]),
-                       paddr_to_pfn(rmrru->base_address),
-                       paddr_to_pfn(rmrru->end_address));
-                overlap = true;
-                break;
-            }
-        }
-        /* Don't add overlapping RMRR. */
-        if ( overlap )
-            continue;
+    /* Invalid pfn in range as the loop ended before end_pfn was reached. */
+    if ( base_iter <= end_pfn )
+        return 0;
 
-        do
-        {
-            if ( !mfn_valid(_mfn(base)) )
-            {
-                printk(XENLOG_ERR VTDPREFIX
-                       "Invalid pfn in RMRR range "ERMRRU_FMT"\n",
-                       ERMRRU_ARG(user_rmrrs[i]));
-                break;
-            }
-        } while ( base++ < end );
+    rmrr = xzalloc(struct acpi_rmrr_unit);
+    if ( !rmrr )
+        return -ENOMEM;
 
-        /* Invalid pfn in range as the loop ended before end_pfn was reached. */
-        if ( base <= end )
-            continue;
+    rmrr->scope.devices = xmalloc_array(u16, dev_count);
+    if ( !rmrr->scope.devices )
+    {
+        xfree(rmrr);
+        return -ENOMEM;
+    }
 
-        rmrr = xzalloc(struct acpi_rmrr_unit);
-        if ( !rmrr )
-            return -ENOMEM;
+    seg = 0;
+    for ( idx = 0; idx < dev_count; idx++ )
+    {
+        rmrr->scope.devices[idx] = sbdf[idx];
+        seg |= PCI_SEG(sbdf[idx]);
+    }
+    if ( seg != PCI_SEG(sbdf[0]) )
+    {
+        printk(XENLOG_ERR VTDPREFIX
+               "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
+               ERMRRU_ARG);
+        scope_devices_free(&rmrr->scope);
+        xfree(rmrr);
+        return 0;
+    }
 
-        rmrr->scope.devices = xmalloc_array(u16, user_rmrrs[i].dev_count);
-        if ( !rmrr->scope.devices )
-        {
-            xfree(rmrr);
-            return -ENOMEM;
-        }
+    rmrr->segment = seg;
+    rmrr->base_address = pfn_to_paddr(base_pfn);
+    /* Align the end_address to the end of the page */
+    rmrr->end_address = pfn_to_paddr(end_pfn) | ~PAGE_MASK;
+    rmrr->scope.devices_cnt = dev_count;
 
-        seg = 0;
-        for ( idx = 0; idx < user_rmrrs[i].dev_count; idx++ )
-        {
-            rmrr->scope.devices[idx] = user_rmrrs[i].sbdf[idx];
-            seg |= PCI_SEG(user_rmrrs[i].sbdf[idx]);
-        }
-        if ( seg != PCI_SEG(user_rmrrs[i].sbdf[0]) )
-        {
-            printk(XENLOG_ERR VTDPREFIX
-                   "Segments are not equal for RMRR range "ERMRRU_FMT"\n",
-                   ERMRRU_ARG(user_rmrrs[i]));
-            scope_devices_free(&rmrr->scope);
-            xfree(rmrr);
-            continue;
-        }
+    if ( register_one_rmrr(rmrr) )
+        printk(XENLOG_ERR VTDPREFIX
+               "Could not register RMMR range "ERMRRU_FMT"\n",
+               ERMRRU_ARG);
 
-        rmrr->segment = seg;
-        rmrr->base_address = pfn_to_paddr(user_rmrrs[i].base_pfn);
-        /* Align the end_address to the end of the page */
-        rmrr->end_address = pfn_to_paddr(user_rmrrs[i].end_pfn) | ~PAGE_MASK;
-        rmrr->scope.devices_cnt = user_rmrrs[i].dev_count;
+    return 1;
+}
 
-        if ( register_one_rmrr(rmrr) )
-            printk(XENLOG_ERR VTDPREFIX
-                   "Could not register RMMR range "ERMRRU_FMT"\n",
-                   ERMRRU_ARG(user_rmrrs[i]));
-    }
+static int __init cf_check add_one_extra_rmrr(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
+{
+    u32 sbdf_array[] = { id };
+    return add_one_user_rmrr(start, start+nr, 1, sbdf_array);
+}
 
-    return 0;
+static int __init add_extra_rmrr(void)
+{
+    return iommu_get_extra_reserved_device_memory(add_one_extra_rmrr, NULL);
 }
 
 #include <asm/tboot.h>
@@ -1010,7 +1038,7 @@ int __init acpi_dmar_init(void)
     {
         iommu_init_ops = &intel_iommu_init_ops;
 
-        return add_user_rmrr();
+        return add_user_rmrr() || add_extra_rmrr();
     }
 
     return ret;
@@ -1108,6 +1136,15 @@ static int __init cf_check parse_rmrr_param(const char *str)
         else
             end = start;
 
+        if ( (end - start) >= MAX_USER_RMRR_PAGES )
+        {
+            printk(XENLOG_ERR VTDPREFIX
+                    "RMRR range "ERMRRU_FMT" exceeds "\
+                    __stringify(MAX_USER_RMRR_PAGES)" pages\n",
+                    start, end);
+            return -E2BIG;
+        }
+
         user_rmrrs[nr_rmrr].base_pfn = start;
         user_rmrrs[nr_rmrr].end_pfn = end;
 
-- 
git-series 0.9.1

[PATCH v5 5/9] IOMMU/AMD: wire common device reserved memory API

[Marek Marczykowski-Górecki]

Register common device reserved memory similar to how ivmd= parameter is
handled.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v3:
 - use variable initializer
 - use pfn_to_paddr()
---
 xen/drivers/passthrough/amd/iommu_acpi.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index ac6835225bae..3b577c9b390c 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -1078,6 +1078,25 @@ static inline bool_t is_ivmd_block(u8 type)
             type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
 }
 
+static int __init cf_check add_one_extra_ivmd(unsigned long start,
+                                              unsigned long nr,
+                                              uint32_t id, void *ctxt)
+{
+    struct acpi_ivrs_memory ivmd = {
+        .header = {
+            .length = sizeof(ivmd),
+            .flags = ACPI_IVMD_UNITY | ACPI_IVMD_READ | ACPI_IVMD_WRITE,
+            .device_id = id,
+            .type = ACPI_IVRS_TYPE_MEMORY_ONE,
+        },
+    };
+
+    ivmd.start_address = pfn_to_paddr(start);
+    ivmd.memory_length = pfn_to_paddr(nr);
+
+    return parse_ivmd_block(&ivmd);
+}
+
 static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
 {
     const struct acpi_ivrs_header *ivrs_block;
@@ -1121,6 +1140,8 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
         AMD_IOMMU_DEBUG("IVMD: %u command line provided entries\n", nr_ivmd);
     for ( i = 0; !error && i < nr_ivmd; ++i )
         error = parse_ivmd_block(user_ivmds + i);
+    if ( !error )
+        error = iommu_get_extra_reserved_device_memory(add_one_extra_ivmd, NULL);
 
     /* Each IO-APIC must have been mentioned in the table. */
     for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
-- 
git-series 0.9.1

[PATCH v5 6/9] drivers/char: mark DMA buffers as reserved for the XHCI

[Marek Marczykowski-Górecki]

The important part is to include those buffers in IOMMU page table
relevant for the USB controller. Otherwise, DbC will stop working as
soon as IOMMU is enabled, regardless of to which domain device assigned
(be it xen or dom0).
If the device is passed through to dom0 or other domain (see later
patches), that domain will effectively have access to those buffers too.
It does give such domain yet another way to DoS the system (as is the
case when having PCI device assigned already), but also possibly steal
the console ring content. Thus, such domain should be a trusted one.
In any case, prevent anything else being placed on those pages by adding
artificial padding.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v5:
- add missing alignment
Changes in v3:
- adjust for xhci-dbc rename
- do not raise MAX_USER_RMRR_PAGES
- adjust alignment of DMA buffers
---
 xen/drivers/char/xhci-dbc.c | 43 +++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index eb35e3a2ee4f..32e9efeb0f77 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -20,6 +20,7 @@
  */
 
 #include <xen/delay.h>
+#include <xen/iommu.h>
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/serial.h>
@@ -1050,13 +1051,21 @@ static struct uart_driver dbc_uart_driver = {
 };
 
 /* Those are accessed via DMA. */
-static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
-static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
-static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
-static struct xhci_erst_segment erst __aligned(16);
-static struct xhci_dbc_ctx ctx __aligned(16);
-static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
-static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+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];
+    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 char __initdata opt_dbc[30];
 
@@ -1093,16 +1102,22 @@ void __init xhci_dbc_uart_init(void)
         dbc->sbdf = PCI_SBDF(0, bus, slot, func);
     }
 
-    dbc->dbc_ctx = &ctx;
-    dbc->dbc_erst = &erst;
-    dbc->dbc_ering.trb = evt_trb;
-    dbc->dbc_oring.trb = out_trb;
-    dbc->dbc_iring.trb = in_trb;
-    dbc->dbc_owork.buf = out_wrk_buf;
-    dbc->dbc_str = str_buf;
+    dbc->dbc_ctx = &dbc_dma_bufs.ctx;
+    dbc->dbc_erst = &dbc_dma_bufs.erst;
+    dbc->dbc_ering.trb = dbc_dma_bufs.evt_trb;
+    dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
+    dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
+    dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
+    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);
         serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
+    }
 }
 
 #ifdef DBC_DEBUG
-- 
git-series 0.9.1

[PATCH v5 7/9] drivers/char: add RX support to the XHCI driver

[Marek Marczykowski-Górecki]

Add another work ring buffer for received data, and point IN TRB at it.
Ensure there is always at least one pending IN TRB, so the controller
has a way to send incoming data to the driver.
Note that both "success" and "short packet" completion codes are okay -
in fact it will be "short packet" most of the time, as the TRB length is
about maximum size, not required size.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v4:
- adjust return types
- add some const
New patch in v3
---
 docs/misc/xen-command-line.pandoc |   4 +-
 xen/drivers/char/xhci-dbc.c       | 129 +++++++++++++++++++++++++++++++-
 2 files changed, 131 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index e31300ea3408..a5883ef49a88 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -736,8 +736,8 @@ over the PCI busses sequentially) or by PCI device (must be on segment 0).
 Specify the USB controller to use, either by instance number (when going
 over the PCI busses sequentially) or by PCI device (must be on segment 0).
 
-Output only console. XHCI driver will wait indefinitely for the debug host to
-connect - make sure the cable is connected.
+XHCI driver will wait indefinitely for the debug host to connect - make sure
+the cable is connected.
 
 ### debug_stack_lines
 > `= <integer>`
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 32e9efeb0f77..afcd3312e2aa 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -111,6 +111,7 @@ enum {
 enum {
     XHCI_TRB_CC_SUCCESS = 1,
     XHCI_TRB_CC_TRB_ERR = 5,
+    XHCI_TRB_CC_SHORT_PACKET = 13,
 };
 
 /* DbC endpoint types */
@@ -239,6 +240,7 @@ struct dbc {
     struct xhci_trb_ring dbc_oring;
     struct xhci_trb_ring dbc_iring;
     struct dbc_work_ring dbc_owork;
+    struct dbc_work_ring dbc_iwork;
     struct xhci_string_descriptor *dbc_str;
 
     pci_sbdf_t sbdf;
@@ -443,6 +445,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
     trb->ctrl |= 0x20;
 }
 
+static uint64_t xhci_trb_norm_buf(const struct xhci_trb *trb)
+{
+    return trb->params;
+}
+
+static uint32_t xhci_trb_norm_len(const struct xhci_trb *trb)
+{
+    return trb->status & 0x1FFFF;
+}
+
 /**
  * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
  * TRBs are read-only from software
@@ -452,6 +464,17 @@ static uint64_t xhci_trb_tfre_ptr(const struct xhci_trb *trb)
     return trb->params;
 }
 
+static uint32_t xhci_trb_tfre_cc(const struct xhci_trb *trb)
+{
+    return trb->status >> 24;
+}
+
+/* Amount of data _not_ transferred */
+static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb)
+{
+    return trb->status & 0x1FFFF;
+}
+
 /* Fields for link TRBs (section 6.4.4.1) */
 static void xhci_trb_link_set_rsp(struct xhci_trb *trb, uint64_t rsp)
 {
@@ -493,6 +516,14 @@ static bool xhci_trb_ring_full(const struct xhci_trb_ring *ring)
     return ((ring->enq + 1) & (DBC_TRB_RING_CAP - 1)) == ring->deq;
 }
 
+static unsigned int xhci_trb_ring_size(const struct xhci_trb_ring *ring)
+{
+    if ( ring->enq >= ring->deq )
+        return ring->enq - ring->deq;
+
+    return DBC_TRB_RING_CAP - ring->deq + ring->enq;
+}
+
 static bool dbc_work_ring_full(const struct dbc_work_ring *ring)
 {
     return ((ring->enq + 1) & (DBC_WORK_RING_CAP - 1)) == ring->deq;
@@ -506,6 +537,14 @@ static unsigned int dbc_work_ring_size(const struct dbc_work_ring *ring)
     return DBC_WORK_RING_CAP - ring->deq + ring->enq;
 }
 
+static unsigned int dbc_work_ring_space_to_end(const struct dbc_work_ring *ring)
+{
+    if ( ring->enq >= ring->deq )
+        return DBC_WORK_RING_CAP - ring->enq;
+
+    return ring->deq - ring->enq;
+}
+
 static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring,
                          uint64_t dma, uint64_t len)
 {
@@ -566,6 +605,31 @@ static unsigned int dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring,
     return i;
 }
 
+static void dbc_rx_trb(struct dbc *dbc, struct xhci_trb *trb,
+                       uint64_t not_transferred)
+{
+    struct dbc_work_ring *ring = &dbc->dbc_iwork;
+    unsigned int rx_len;
+    unsigned int end, start = ring->enq;
+
+    if ( xhci_trb_type(trb) != XHCI_TRB_NORM )
+        /* Can be Link TRB for example. */
+        return;
+
+    ASSERT(xhci_trb_norm_buf(trb) == ring->dma + ring->enq);
+    ASSERT(xhci_trb_norm_len(trb) >= not_transferred);
+    rx_len = xhci_trb_norm_len(trb) - not_transferred;
+
+    /* It can hit the ring end, but should not wrap around. */
+    ASSERT(ring->enq + rx_len <= DBC_WORK_RING_CAP);
+    ring->enq = (ring->enq + rx_len) & (DBC_WORK_RING_CAP - 1);
+
+    end = ring->enq;
+
+    if ( end > start )
+        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
@@ -575,6 +639,7 @@ static void dbc_pop_events(struct dbc *dbc)
     struct dbc_reg *reg = dbc->dbc_reg;
     struct xhci_trb_ring *er = &dbc->dbc_ering;
     struct xhci_trb_ring *tr = &dbc->dbc_oring;
+    struct xhci_trb_ring *ir = &dbc->dbc_iring;
     struct xhci_trb *event = &er->trb[er->deq];
     uint64_t erdp = readq(&reg->erdp);
     uint32_t portsc;
@@ -600,6 +665,14 @@ static void dbc_pop_events(struct dbc *dbc)
                 trb_idx = (event_ptr - tr->dma) >> XHCI_TRB_SHIFT;
                 tr->deq = (trb_idx + 1) & (DBC_TRB_RING_CAP - 1);
             }
+            else if ( event_ptr - ir->dma < DBC_TRB_RING_BYTES )
+            {
+                trb_idx = (event_ptr - ir->dma) >> XHCI_TRB_SHIFT;
+                if ( xhci_trb_tfre_cc(event) == XHCI_TRB_CC_SUCCESS ||
+                     xhci_trb_tfre_cc(event) == XHCI_TRB_CC_SHORT_PACKET )
+                    dbc_rx_trb(dbc, &ir->trb[trb_idx], xhci_trb_tfre_len(event));
+                ir->deq = (trb_idx + 1) & (DBC_TRB_RING_CAP - 1);
+            }
             else
                 dbc_alert("event: TRB 0x%lx not found in any ring\n",
                           event_ptr);
@@ -870,6 +943,7 @@ static bool __init dbc_open(struct dbc *dbc)
         return false;
 
     dbc_init_work_ring(dbc, &dbc->dbc_owork);
+    dbc_init_work_ring(dbc, &dbc->dbc_iwork);
     dbc_enable_dbc(dbc);
     dbc->open = true;
 
@@ -946,6 +1020,33 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb,
 }
 
 /**
+ * Ensure DbC has a pending transfer TRB to receive data into.
+ *
+ * @param dbc the dbc to flush
+ * @param trb the ring for the TRBs to transfer
+ * @param wrk the work ring to receive data into
+ */
+static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb,
+                           struct dbc_work_ring *wrk)
+{
+    struct dbc_reg *reg = dbc->dbc_reg;
+    uint32_t db = (readl(&reg->db) & 0xFFFF00FF) | (trb->db << 8);
+
+    /* Check if there is already queued TRB */
+    if ( xhci_trb_ring_size(trb) >= 1 )
+        return;
+
+    if ( dbc_work_ring_full(wrk) )
+        return;
+
+    dbc_push_trb(dbc, trb, wrk->dma + wrk->enq,
+                 dbc_work_ring_space_to_end(wrk));
+
+    wmb();
+    writel(db, &reg->db);
+}
+
+/**
  * Queue a single character to the DbC. A transfer TRB will be created
  * if the character is a newline and the DbC will be notified that data is
  * available for writing to the debug host.
@@ -968,6 +1069,19 @@ static int64_t dbc_putc(struct dbc *dbc, char c)
     return 1;
 }
 
+static int dbc_getc(struct dbc *dbc, char *c)
+{
+    struct dbc_work_ring *wrk = &dbc->dbc_iwork;
+
+    if ( dbc_work_ring_size(wrk) == 0 )
+        return 0;
+
+    *c = wrk->buf[wrk->deq];
+    wrk->deq = (wrk->deq + 1) & (DBC_WORK_RING_CAP - 1);
+
+    return 1;
+}
+
 struct dbc_uart {
     struct dbc dbc;
     struct timer timer;
@@ -986,10 +1100,16 @@ static void cf_check dbc_uart_poll(void *data)
     if ( spin_trylock_irqsave(&port->tx_lock, flags) )
     {
         if ( dbc_ensure_running(dbc) )
+        {
             dbc_flush(dbc, &dbc->dbc_oring, &dbc->dbc_owork);
+            dbc_enqueue_in(dbc, &dbc->dbc_iring, &dbc->dbc_iwork);
+        }
         spin_unlock_irqrestore(&port->tx_lock, flags);
     }
 
+    while ( dbc_work_ring_size(&dbc->dbc_iwork) )
+        serial_rx_interrupt(port, guest_cpu_user_regs());
+
     serial_tx_interrupt(port, guest_cpu_user_regs());
     set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL));
 }
@@ -1028,6 +1148,12 @@ static void cf_check dbc_uart_putc(struct serial_port *port, char c)
     dbc_putc(&uart->dbc, c);
 }
 
+static int cf_check dbc_uart_getc(struct serial_port *port, char *c)
+{
+    struct dbc_uart *uart = port->uart;
+    return dbc_getc(&uart->dbc, c);
+}
+
 static void cf_check dbc_uart_flush(struct serial_port *port)
 {
     s_time_t goal;
@@ -1047,6 +1173,7 @@ static struct uart_driver dbc_uart_driver = {
     .init_postirq = dbc_uart_init_postirq,
     .tx_ready = dbc_uart_tx_ready,
     .putc = dbc_uart_putc,
+    .getc = dbc_uart_getc,
     .flush = dbc_uart_flush,
 };
 
@@ -1056,6 +1183,7 @@ struct dbc_dma_bufs {
     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];
@@ -1108,6 +1236,7 @@ void __init xhci_dbc_uart_init(void)
     dbc->dbc_oring.trb = dbc_dma_bufs.out_trb;
     dbc->dbc_iring.trb = dbc_dma_bufs.in_trb;
     dbc->dbc_owork.buf = dbc_dma_bufs.out_wrk_buf;
+    dbc->dbc_iwork.buf = dbc_dma_bufs.in_wrk_buf;
     dbc->dbc_str = dbc_dma_bufs.str_buf;
 
     if ( dbc_open(dbc) )
-- 
git-series 0.9.1

[PATCH v5 8/9] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC

[Marek Marczykowski-Górecki]

That's possible, because the capability was designed specifically to
allow separate driver handle it, in parallel to unmodified xhci driver
(separate set of registers, pretending the port is "disconnected" for
the main xhci driver etc). It works with Linux dom0, although requires
an awful hack - re-enabling bus mastering behind dom0's backs.
Linux driver does similar thing - see
drivers/usb/early/xhci-dbc.c:xdbc_handle_events().

When controller sharing is enabled in kconfig (option marked as
experimental), dom0 is allowed to use the controller even if Xen uses it
for debug console. Additionally, option `dbgp=xhci,share=` is available
to either prevent even dom0 from using it (`no` value), or allow any
domain using it (`any` value).

In any case, to avoid Linux messing with the DbC, mark this MMIO area as
read-only. This might cause issues for Linux's driver (if it tries to
write something on the same page - like anoter xcap), but makes Xen's
use safe. In practice, as of Linux 5.18, it seems to work without
issues.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v5:
- drop CONFIG_XHCI_SHARE
- make XHCI_SHARE_HWDOM = 0
- use parse_boolean
- add comment about mmio_ro_ranges
- fix doc
Changes in v4:
- minor fix for cmdline parsing
- make sharing opt-in build time, with option marked as EXPERIMENTAL
- change cmdline syntax to share=<bool>|hwdom
- make share=hwdom default (if enabled build-time)
Changes in v3:
- adjust for xhci-dbc rename
- adjust for dbc_ensure_running() split
- wrap long lines
- add runtime option for sharing USB controller
---
 docs/misc/xen-command-line.pandoc |  14 ++-
 xen/drivers/char/xhci-dbc.c       | 129 +++++++++++++++++++++++++++++--
 2 files changed, 134 insertions(+), 9 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index a5883ef49a88..5aa184a4a06c 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -731,13 +731,25 @@ Specify the USB controller to use, either by instance number (when going
 over the PCI busses sequentially) or by PCI device (must be on segment 0).
 
 ### dbc
-> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
+> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=<bool>|hwdom]`
 
 Specify the USB controller to use, either by instance number (when going
 over the PCI busses sequentially) or by PCI device (must be on segment 0).
 
 XHCI driver will wait indefinitely for the debug host to connect - make sure
 the cable is connected.
+The `share` option for xhci controls who else can use the controller:
+* `no`: use the controller exclusively for console, even hardware domain
+  (dom0) cannot use it
+* `hwdom`: hardware domain may use the controller too, ports not used for debug
+  console will be available for normal devices; this is the default
+* `yes`: the controller can be assigned to any domain; it is not safe to assign
+  the controller to untrusted domain
+
+Choosing `share=hwdom` (the default) or `share=yes` allows a domain to reset the
+controller, which may cause small portion of the console output to be lost.
+
+The `share=yes` configuration is not security supported.
 
 ### debug_stack_lines
 > `= <integer>`
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index afcd3312e2aa..e838e285fb75 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -23,6 +23,7 @@
 #include <xen/iommu.h>
 #include <xen/mm.h>
 #include <xen/param.h>
+#include <xen/rangeset.h>
 #include <xen/serial.h>
 #include <xen/timer.h>
 #include <xen/types.h>
@@ -232,6 +233,12 @@ struct dbc_work_ring {
     uint64_t dma;
 };
 
+enum xhci_share {
+    XHCI_SHARE_HWDOM = 0,
+    XHCI_SHARE_NONE,
+    XHCI_SHARE_ANY
+};
+
 struct dbc {
     struct dbc_reg __iomem *dbc_reg;
     struct xhci_dbc_ctx *dbc_ctx;
@@ -249,6 +256,7 @@ struct dbc {
     void __iomem *xhc_mmio;
 
     bool open;
+    enum xhci_share share;
     unsigned int xhc_num; /* look for n-th xhc */
 };
 
@@ -951,13 +959,56 @@ static bool __init dbc_open(struct dbc *dbc)
 }
 
 /*
- * Ensure DbC is still running, handle events, and possibly re-enable if cable
- * was re-plugged. Returns true if DbC is operational.
+ * Ensure DbC is still running, handle events, and possibly
+ * re-enable/re-configure if cable was re-plugged or controller was reset.
+ * Returns true if DbC is operational.
  */
 static bool dbc_ensure_running(struct dbc *dbc)
 {
     struct dbc_reg *reg = dbc->dbc_reg;
     uint32_t ctrl;
+    uint16_t cmd;
+
+    if ( dbc->share != XHCI_SHARE_NONE )
+    {
+        /*
+         * Re-enable memory decoding and later bus mastering, if dom0 (or
+         * other) disabled it in the meantime.
+         */
+        cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+        if ( !(cmd & PCI_COMMAND_MEMORY) )
+        {
+            cmd |= PCI_COMMAND_MEMORY;
+            pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
+        }
+
+        /*
+         * FIXME: Make Linux coordinate XHCI reset, so the DbC driver can
+         * prepare for it properly, instead of only detecting it after the
+         * fact. See EHCI driver for similar handling.
+         */
+        if ( dbc->open && !(readl(&reg->ctrl) & (1U << DBC_CTRL_DCE)) )
+        {
+            if ( !dbc_init_dbc(dbc) )
+                return false;
+
+            dbc_init_work_ring(dbc, &dbc->dbc_owork);
+            dbc_enable_dbc(dbc);
+        }
+        else
+        {
+            /*
+             * dbc_init_dbc() takes care about it, so check only if it wasn't
+             * called.
+             */
+            cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+            if ( !(cmd & PCI_COMMAND_MASTER) )
+            {
+                cmd |= PCI_COMMAND_MASTER;
+                pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
+            }
+        }
+    }
 
     dbc_pop_events(dbc);
 
@@ -1128,10 +1179,38 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
     init_timer(&uart->timer, dbc_uart_poll, port, 0);
     set_timer(&uart->timer, NOW() + MILLISECS(1));
 
-    if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) )
-        printk(XENLOG_WARNING
-               "Failed to mark read-only %pp used for XHCI console\n",
-               &uart->dbc.sbdf);
+    switch ( uart->dbc.share )
+    {
+    case XHCI_SHARE_NONE:
+        if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) )
+            printk(XENLOG_WARNING
+                   "Failed to mark read-only %pp used for XHCI console\n",
+                   &uart->dbc.sbdf);
+        break;
+    case XHCI_SHARE_HWDOM:
+        if ( pci_hide_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) )
+            printk(XENLOG_WARNING
+                   "Failed to hide %pp used for XHCI console\n",
+                   &uart->dbc.sbdf);
+        break;
+    case XHCI_SHARE_ANY:
+        /* Do not hide. */
+        break;
+    }
+#ifdef CONFIG_X86
+    /*
+     * This marks the whole page as R/O, which may include other registers
+     * unrelated to DbC. Xen needs only DbC area protected, but it seems
+     * Linux's XHCI driver (as of 5.18) works without writting to the whole
+     * page, so keep it simple.
+     */
+    if ( rangeset_add_range(mmio_ro_ranges,
+                PFN_DOWN(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset),
+                PFN_UP(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset +
+                       sizeof(*uart->dbc.dbc_reg)) - 1) )
+        printk(XENLOG_INFO
+               "Error while adding MMIO range of device to mmio_ro_ranges\n");
+#endif
 }
 
 static int cf_check dbc_uart_tx_ready(struct serial_port *port)
@@ -1203,13 +1282,15 @@ void __init xhci_dbc_uart_init(void)
 {
     struct dbc_uart *uart = &dbc_uart;
     struct dbc *dbc = &uart->dbc;
-    const char *e;
+    const char *e, *opt;
+    int val;
 
     if ( strncmp(opt_dbc, "xhci", 4) )
         return;
 
     memset(dbc, 0, sizeof(*dbc));
 
+    e = &opt_dbc[4];
     if ( isdigit(opt_dbc[4]) )
     {
         dbc->xhc_num = simple_strtoul(opt_dbc + 4, &e, 10);
@@ -1219,7 +1300,7 @@ void __init xhci_dbc_uart_init(void)
         unsigned int bus, slot, func;
 
         e = parse_pci(opt_dbc + 8, NULL, &bus, &slot, &func);
-        if ( !e || *e )
+        if ( !e || (*e && *e != ',') )
         {
             printk(XENLOG_ERR
                    "Invalid dbc= PCI device spec: '%s'\n",
@@ -1229,6 +1310,38 @@ void __init xhci_dbc_uart_init(void)
 
         dbc->sbdf = PCI_SBDF(0, bus, slot, func);
     }
+    opt = e;
+
+    /* other options */
+    while ( *opt == ',' )
+    {
+        opt++;
+        e = strchr(opt, ',');
+        if ( !e )
+            e = strchr(opt, '\0');
+
+        if ( (val = parse_boolean("share", opt, e)) != -1 )
+        {
+            if ( val == -2 && !cmdline_strcmp(opt + 6, "hwdom") )
+                dbc->share = XHCI_SHARE_HWDOM;
+            else if ( val == 0 )
+                dbc->share = XHCI_SHARE_NONE;
+            else if ( val == 1 )
+                dbc->share = XHCI_SHARE_ANY;
+            else
+                break;
+        }
+        else
+            break;
+
+        opt = e;
+    }
+
+    if ( *opt )
+    {
+        printk(XENLOG_ERR "Invalid dbc= parameters: '%s'\n", opt);
+        return;
+    }
 
     dbc->dbc_ctx = &dbc_dma_bufs.ctx;
     dbc->dbc_erst = &dbc_dma_bufs.erst;
-- 
git-series 0.9.1

[PATCH v5 9/9] drivers/char: fix handling cable re-plug in XHCI console driver

[Marek Marczykowski-Górecki]

When cable is unplugged, dbc_ensure_running() correctly detects this
situation (DBC_CTRL_DCR flag is clear), and prevent sending data
immediately to the device. It gets only queued in work ring buffers.
When cable is plugged in again, subsequent dbc_flush() will send the
buffered data.
But there is a corner case, where no subsequent data was buffered in the
work buffer, but a TRB was still pending. Ring the doorbell to let the
controller re-send them. For console output it is rare corner case (TRB
is pending for a very short time), but for console input it is very
normal case (there is always one pending TRB for input).

Extract doorbell ringing into separate function to avoid duplication.

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

diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index e838e285fb75..0ff340dac103 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -553,6 +553,15 @@ static unsigned int dbc_work_ring_space_to_end(const struct dbc_work_ring *ring)
     return ring->deq - ring->enq;
 }
 
+static void dbc_ring_doorbell(struct dbc *dbc, int doorbell)
+{
+    uint32_t __iomem *db_reg = &dbc->dbc_reg->db;
+    uint32_t db = (readl(db_reg) & ~DBC_DOORBELL_TARGET_MASK) |
+                  (doorbell << DBC_DOORBELL_TARGET_SHIFT);
+
+    writel(db, db_reg);
+}
+
 static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring,
                          uint64_t dma, uint64_t len)
 {
@@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc)
         writel(ctrl | (1U << DBC_CTRL_DRC), &reg->ctrl);
         writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->portsc);
         wmb();
+        dbc_ring_doorbell(dbc, dbc->dbc_iring.db);
+        dbc_ring_doorbell(dbc, dbc->dbc_oring.db);
     }
 
     return true;
@@ -1040,10 +1051,6 @@ static bool dbc_ensure_running(struct dbc *dbc)
 static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb,
                       struct dbc_work_ring *wrk)
 {
-    struct dbc_reg *reg = dbc->dbc_reg;
-    uint32_t db = (readl(&reg->db) & ~DBC_DOORBELL_TARGET_MASK) |
-                  (trb->db << DBC_DOORBELL_TARGET_SHIFT);
-
     if ( xhci_trb_ring_full(trb) )
         return;
 
@@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb,
         }
     }
 
-    wmb();
-    writel(db, &reg->db);
+    dbc_ring_doorbell(dbc, trb->db);
 }
 
 /**
-- 
git-series 0.9.1

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Jan Beulich]

On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> This allows configuring EHCI and XHCI consoles separately,
> simultaneously.
> 
> Suggested-by: Jan Beulich <jbeulich@suse.com>

But was I maybe confused, and much less of a change would suffice? After
all ...

> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
>  
> -static char __initdata opt_dbgp[30];
> +static char __initdata opt_dbc[30];
>  
> -string_param("dbgp", opt_dbgp);
> +string_param("dbc", opt_dbc);
>  
>  void __init xhci_dbc_uart_init(void)
>  {
> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
>      struct dbc *dbc = &uart->dbc;
>      const char *e;
>  
> -    if ( strncmp(opt_dbgp, "xhci", 4) )
> +    if ( strncmp(opt_dbc, "xhci", 4) )
>          return;

... this already avoids mixing up who's going to parse what. So right
now I think that ...

> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
>      dbc->dbc_str = str_buf;
>  
>      if ( dbc_open(dbc) )
> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
>  }

... this and other SERHND_* related changes are enough, and there's no
need for a separate "dbc=" option.

> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -95,6 +95,7 @@ struct uart_driver {
>  # define SERHND_COM1    (0<<0)
>  # define SERHND_COM2    (1<<0)
>  # define SERHND_DBGP    (2<<0)
> +# define SERHND_DBC     (3<<0)

Please also update the comment just out of context.

Jan

Re: [PATCH v5 3/9] IOMMU: add common API for device reserved memory

[Jan Beulich]

On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> Add API similar to rmrr= and ivmd= arguments, but in a common code. This
> will allow drivers to register reserved memory regardless of the IOMMU
> vendor.
> The direct reason for this API is xhci-dbc console driver (aka xue),
> that needs to use DMA. But future change may unify command line
> arguments for user-supplied reserved memory, and it may be useful for
> other drivers in the future too.
> 
> This commit just introduces an API, subsequent patches will plug it in
> appropriate places. The reserved memory ranges needs to be saved
> locally, because at the point when they are collected, Xen doesn't know
> yet which IOMMU driver will be used.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

Re: [PATCH v5 6/9] drivers/char: mark DMA buffers as reserved for the XHCI

[Jan Beulich]

On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> The important part is to include those buffers in IOMMU page table
> relevant for the USB controller. Otherwise, DbC will stop working as
> soon as IOMMU is enabled, regardless of to which domain device assigned
> (be it xen or dom0).
> If the device is passed through to dom0 or other domain (see later
> patches), that domain will effectively have access to those buffers too.
> It does give such domain yet another way to DoS the system (as is the
> case when having PCI device assigned already), but also possibly steal
> the console ring content. Thus, such domain should be a trusted one.
> In any case, prevent anything else being placed on those pages by adding
> artificial padding.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Marek Marczykowski-Górecki]

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

On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> > This allows configuring EHCI and XHCI consoles separately,
> > simultaneously.
> > 
> > Suggested-by: Jan Beulich <jbeulich@suse.com>
> 
> But was I maybe confused, and much less of a change would suffice? After
> all ...
> 
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
> >  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
> >  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> >  
> > -static char __initdata opt_dbgp[30];
> > +static char __initdata opt_dbc[30];
> >  
> > -string_param("dbgp", opt_dbgp);
> > +string_param("dbc", opt_dbc);
> >  
> >  void __init xhci_dbc_uart_init(void)
> >  {
> > @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
> >      struct dbc *dbc = &uart->dbc;
> >      const char *e;
> >  
> > -    if ( strncmp(opt_dbgp, "xhci", 4) )
> > +    if ( strncmp(opt_dbc, "xhci", 4) )
> >          return;
> 
> ... this already avoids mixing up who's going to parse what. So right
> now I think that ...
> 
> > @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
> >      dbc->dbc_str = str_buf;
> >  
> >      if ( dbc_open(dbc) )
> > -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
> > +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
> >  }
> 
> ... this and other SERHND_* related changes are enough, and there's no
> need for a separate "dbc=" option.

But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
one would override the other, no?

> 
> > --- a/xen/include/xen/serial.h
> > +++ b/xen/include/xen/serial.h
> > @@ -95,6 +95,7 @@ struct uart_driver {
> >  # define SERHND_COM1    (0<<0)
> >  # define SERHND_COM2    (1<<0)
> >  # define SERHND_DBGP    (2<<0)
> > +# define SERHND_DBC     (3<<0)
> 
> Please also update the comment just out of context.
> 
> Jan

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

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

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Jan Beulich]

On 26.08.2022 13:46, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
>> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
>>> This allows configuring EHCI and XHCI consoles separately,
>>> simultaneously.
>>>
>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>
>> But was I maybe confused, and much less of a change would suffice? After
>> all ...
>>
>>> --- a/xen/drivers/char/xhci-dbc.c
>>> +++ b/xen/drivers/char/xhci-dbc.c
>>> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
>>>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
>>>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
>>>  
>>> -static char __initdata opt_dbgp[30];
>>> +static char __initdata opt_dbc[30];
>>>  
>>> -string_param("dbgp", opt_dbgp);
>>> +string_param("dbc", opt_dbc);
>>>  
>>>  void __init xhci_dbc_uart_init(void)
>>>  {
>>> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
>>>      struct dbc *dbc = &uart->dbc;
>>>      const char *e;
>>>  
>>> -    if ( strncmp(opt_dbgp, "xhci", 4) )
>>> +    if ( strncmp(opt_dbc, "xhci", 4) )
>>>          return;
>>
>> ... this already avoids mixing up who's going to parse what. So right
>> now I think that ...
>>
>>> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
>>>      dbc->dbc_str = str_buf;
>>>  
>>>      if ( dbc_open(dbc) )
>>> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
>>> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
>>>  }
>>
>> ... this and other SERHND_* related changes are enough, and there's no
>> need for a separate "dbc=" option.
> 
> But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
> one would override the other, no?

Not as long as both use string_param(), true. They'd need to both become
custom_param(), doing at least some basic parsing right away.

But using two such options at the same time isn't of interest anyway
without your multiple-serial-consoles change, so possibly not of
immediate need (unless someone comes forward expressing interest and
actually approving that change of yours).

Jan

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Andrew Cooper]

On 26/08/2022 15:20, Jan Beulich wrote:
> On 26.08.2022 13:46, Marek Marczykowski-Górecki wrote:
>> On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
>>> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
>>>> This allows configuring EHCI and XHCI consoles separately,
>>>> simultaneously.
>>>>
>>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>> But was I maybe confused, and much less of a change would suffice? After
>>> all ...
>>>
>>>> --- a/xen/drivers/char/xhci-dbc.c
>>>> +++ b/xen/drivers/char/xhci-dbc.c
>>>> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
>>>>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
>>>>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
>>>>  
>>>> -static char __initdata opt_dbgp[30];
>>>> +static char __initdata opt_dbc[30];
>>>>  
>>>> -string_param("dbgp", opt_dbgp);
>>>> +string_param("dbc", opt_dbc);
>>>>  
>>>>  void __init xhci_dbc_uart_init(void)
>>>>  {
>>>> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
>>>>      struct dbc *dbc = &uart->dbc;
>>>>      const char *e;
>>>>  
>>>> -    if ( strncmp(opt_dbgp, "xhci", 4) )
>>>> +    if ( strncmp(opt_dbc, "xhci", 4) )
>>>>          return;
>>> ... this already avoids mixing up who's going to parse what. So right
>>> now I think that ...
>>>
>>>> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
>>>>      dbc->dbc_str = str_buf;
>>>>  
>>>>      if ( dbc_open(dbc) )
>>>> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
>>>> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
>>>>  }
>>> ... this and other SERHND_* related changes are enough, and there's no
>>> need for a separate "dbc=" option.
>> But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
>> one would override the other, no?
> Not as long as both use string_param(), true. They'd need to both become
> custom_param(), doing at least some basic parsing right away.

I've looked through our string params, and none of them look like they
should be string params.

I have half a mind to transform them all, one at a time, and remove
string_param() to prevent problems like this in the future.

~Andrew

Re: [PATCH v5 8/9] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC

[Jan Beulich]

On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> That's possible, because the capability was designed specifically to
> allow separate driver handle it, in parallel to unmodified xhci driver
> (separate set of registers, pretending the port is "disconnected" for
> the main xhci driver etc). It works with Linux dom0, although requires
> an awful hack - re-enabling bus mastering behind dom0's backs.
> Linux driver does similar thing - see
> drivers/usb/early/xhci-dbc.c:xdbc_handle_events().
> 
> When controller sharing is enabled in kconfig (option marked as
> experimental), dom0 is allowed to use the controller even if Xen uses it
> for debug console. Additionally, option `dbgp=xhci,share=` is available
> to either prevent even dom0 from using it (`no` value), or allow any
> domain using it (`any` value).
> 
> In any case, to avoid Linux messing with the DbC, mark this MMIO area as
> read-only. This might cause issues for Linux's driver (if it tries to
> write something on the same page - like anoter xcap), but makes Xen's
> use safe. In practice, as of Linux 5.18, it seems to work without
> issues.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>

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

Re: [PATCH v5 9/9] drivers/char: fix handling cable re-plug in XHCI console driver

[Jan Beulich]

On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc)
>          writel(ctrl | (1U << DBC_CTRL_DRC), &reg->ctrl);
>          writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->portsc);
>          wmb();
> +        dbc_ring_doorbell(dbc, dbc->dbc_iring.db);
> +        dbc_ring_doorbell(dbc, dbc->dbc_oring.db);
>      }

You retain the wmb() here, but ...

> @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb,
>          }
>      }
>  
> -    wmb();
> -    writel(db, &reg->db);
> +    dbc_ring_doorbell(dbc, trb->db);
>  }

... you drop it here. Why the difference?

Jan

Re: [PATCH v5 9/9] drivers/char: fix handling cable re-plug in XHCI console driver

[Andrew Cooper]

On 26/08/2022 15:50, Jan Beulich wrote:
> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
>> @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc)
>>          writel(ctrl | (1U << DBC_CTRL_DRC), &reg->ctrl);
>>          writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->portsc);
>>          wmb();
>> +        dbc_ring_doorbell(dbc, dbc->dbc_iring.db);
>> +        dbc_ring_doorbell(dbc, dbc->dbc_oring.db);
>>      }
> You retain the wmb() here, but ...
>
>> @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb,
>>          }
>>      }
>>  
>> -    wmb();
>> -    writel(db, &reg->db);
>> +    dbc_ring_doorbell(dbc, trb->db);
>>  }
> ... you drop it here. Why the difference?

As a tangent, every single barrier in this file is buggy.  Should be
smp_*() variants, not mandatory variants.

All (interesting) data is in plain WB cached memory, and the few BAR
registers which are configured have a UC mapping which orders properly
WRT other writes on x86.

~Andrew

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Marek Marczykowski-Górecki]

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

On Fri, Aug 26, 2022 at 04:20:52PM +0200, Jan Beulich wrote:
> On 26.08.2022 13:46, Marek Marczykowski-Górecki wrote:
> > On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
> >> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> >>> This allows configuring EHCI and XHCI consoles separately,
> >>> simultaneously.
> >>>
> >>> Suggested-by: Jan Beulich <jbeulich@suse.com>
> >>
> >> But was I maybe confused, and much less of a change would suffice? After
> >> all ...
> >>
> >>> --- a/xen/drivers/char/xhci-dbc.c
> >>> +++ b/xen/drivers/char/xhci-dbc.c
> >>> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
> >>>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
> >>>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> >>>  
> >>> -static char __initdata opt_dbgp[30];
> >>> +static char __initdata opt_dbc[30];
> >>>  
> >>> -string_param("dbgp", opt_dbgp);
> >>> +string_param("dbc", opt_dbc);
> >>>  
> >>>  void __init xhci_dbc_uart_init(void)
> >>>  {
> >>> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
> >>>      struct dbc *dbc = &uart->dbc;
> >>>      const char *e;
> >>>  
> >>> -    if ( strncmp(opt_dbgp, "xhci", 4) )
> >>> +    if ( strncmp(opt_dbc, "xhci", 4) )
> >>>          return;
> >>
> >> ... this already avoids mixing up who's going to parse what. So right
> >> now I think that ...
> >>
> >>> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
> >>>      dbc->dbc_str = str_buf;
> >>>  
> >>>      if ( dbc_open(dbc) )
> >>> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
> >>> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
> >>>  }
> >>
> >> ... this and other SERHND_* related changes are enough, and there's no
> >> need for a separate "dbc=" option.
> > 
> > But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
> > one would override the other, no?
> 
> Not as long as both use string_param(), true. They'd need to both become
> custom_param(), doing at least some basic parsing right away.
> 
> But using two such options at the same time isn't of interest anyway
> without your multiple-serial-consoles change, so possibly not of
> immediate need (unless someone comes forward expressing interest and
> actually approving that change of yours).

Then why change at all? Since you can configure only one (dbgp=ehci _or_
dbgp=xhci), then there is not ambiguity what "console=dbgp" means.
Separating SERHND_DBC from SERHND_DBGP would IMO make sense only if you
can actually use them both (even if not both for console, but for
example one for debugger).

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

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

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Marek Marczykowski-Górecki]

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

On Mon, Aug 29, 2022 at 01:49:30PM +0200, Marek Marczykowski-Górecki wrote:
> On Fri, Aug 26, 2022 at 04:20:52PM +0200, Jan Beulich wrote:
> > On 26.08.2022 13:46, Marek Marczykowski-Górecki wrote:
> > > On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
> > >> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> > >>> This allows configuring EHCI and XHCI consoles separately,
> > >>> simultaneously.
> > >>>
> > >>> Suggested-by: Jan Beulich <jbeulich@suse.com>
> > >>
> > >> But was I maybe confused, and much less of a change would suffice? After
> > >> all ...
> > >>
> > >>> --- a/xen/drivers/char/xhci-dbc.c
> > >>> +++ b/xen/drivers/char/xhci-dbc.c
> > >>> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
> > >>>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
> > >>>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> > >>>  
> > >>> -static char __initdata opt_dbgp[30];
> > >>> +static char __initdata opt_dbc[30];
> > >>>  
> > >>> -string_param("dbgp", opt_dbgp);
> > >>> +string_param("dbc", opt_dbc);
> > >>>  
> > >>>  void __init xhci_dbc_uart_init(void)
> > >>>  {
> > >>> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
> > >>>      struct dbc *dbc = &uart->dbc;
> > >>>      const char *e;
> > >>>  
> > >>> -    if ( strncmp(opt_dbgp, "xhci", 4) )
> > >>> +    if ( strncmp(opt_dbc, "xhci", 4) )
> > >>>          return;
> > >>
> > >> ... this already avoids mixing up who's going to parse what. So right
> > >> now I think that ...
> > >>
> > >>> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
> > >>>      dbc->dbc_str = str_buf;
> > >>>  
> > >>>      if ( dbc_open(dbc) )
> > >>> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
> > >>> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
> > >>>  }
> > >>
> > >> ... this and other SERHND_* related changes are enough, and there's no
> > >> need for a separate "dbc=" option.
> > > 
> > > But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
> > > one would override the other, no?
> > 
> > Not as long as both use string_param(), true. They'd need to both become
> > custom_param(), doing at least some basic parsing right away.
> > 
> > But using two such options at the same time isn't of interest anyway
> > without your multiple-serial-consoles change, so possibly not of
> > immediate need (unless someone comes forward expressing interest and
> > actually approving that change of yours).
> 
> Then why change at all? Since you can configure only one (dbgp=ehci _or_
> dbgp=xhci), then there is not ambiguity what "console=dbgp" means.
> Separating SERHND_DBC from SERHND_DBGP would IMO make sense only if you
> can actually use them both (even if not both for console, but for
> example one for debugger).

Or do you mean to use custom_param() to actually make "dbgp=xhci
dbgp=ehci" working? But then IMO having "console=dbgp console=dbc" would
be confusing, as "dbc" has no obvious relation to neither side of
"dbgp=xhci". Maybe use "console=xhci" then?

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

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

Re: [PATCH v5 9/9] drivers/char: fix handling cable re-plug in XHCI console driver

[Jan Beulich]

On 26.08.2022 17:44, Andrew Cooper wrote:
> On 26/08/2022 15:50, Jan Beulich wrote:
>> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
>>> @@ -1023,6 +1032,8 @@ static bool dbc_ensure_running(struct dbc *dbc)
>>>          writel(ctrl | (1U << DBC_CTRL_DRC), &reg->ctrl);
>>>          writel(readl(&reg->portsc) | (1U << DBC_PSC_PED), &reg->portsc);
>>>          wmb();
>>> +        dbc_ring_doorbell(dbc, dbc->dbc_iring.db);
>>> +        dbc_ring_doorbell(dbc, dbc->dbc_oring.db);
>>>      }
>> You retain the wmb() here, but ...
>>
>>> @@ -1066,8 +1073,7 @@ static void dbc_flush(struct dbc *dbc, struct xhci_trb_ring *trb,
>>>          }
>>>      }
>>>  
>>> -    wmb();
>>> -    writel(db, &reg->db);
>>> +    dbc_ring_doorbell(dbc, trb->db);
>>>  }
>> ... you drop it here. Why the difference?
> 
> As a tangent, every single barrier in this file is buggy.  Should be
> smp_*() variants, not mandatory variants.
> 
> All (interesting) data is in plain WB cached memory, and the few BAR
> registers which are configured have a UC mapping which orders properly
> WRT other writes on x86.

But such drivers shouldn't be x86-specific when it comes to their use
of barriers. For this reason I specifically did not complain about any
of the barrier uses throughout the series (with the further thinking
of "better one too many than one too few").

Jan

Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option

[Jan Beulich]

On 29.08.2022 13:57, Marek Marczykowski-Górecki wrote:
> On Mon, Aug 29, 2022 at 01:49:30PM +0200, Marek Marczykowski-Górecki wrote:
>> On Fri, Aug 26, 2022 at 04:20:52PM +0200, Jan Beulich wrote:
>>> On 26.08.2022 13:46, Marek Marczykowski-Górecki wrote:
>>>> On Thu, Aug 25, 2022 at 05:44:54PM +0200, Jan Beulich wrote:
>>>>> On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
>>>>>> This allows configuring EHCI and XHCI consoles separately,
>>>>>> simultaneously.
>>>>>>
>>>>>> Suggested-by: Jan Beulich <jbeulich@suse.com>
>>>>>
>>>>> But was I maybe confused, and much less of a change would suffice? After
>>>>> all ...
>>>>>
>>>>>> --- a/xen/drivers/char/xhci-dbc.c
>>>>>> +++ b/xen/drivers/char/xhci-dbc.c
>>>>>> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
>>>>>>  static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
>>>>>>  static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
>>>>>>  
>>>>>> -static char __initdata opt_dbgp[30];
>>>>>> +static char __initdata opt_dbc[30];
>>>>>>  
>>>>>> -string_param("dbgp", opt_dbgp);
>>>>>> +string_param("dbc", opt_dbc);
>>>>>>  
>>>>>>  void __init xhci_dbc_uart_init(void)
>>>>>>  {
>>>>>> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
>>>>>>      struct dbc *dbc = &uart->dbc;
>>>>>>      const char *e;
>>>>>>  
>>>>>> -    if ( strncmp(opt_dbgp, "xhci", 4) )
>>>>>> +    if ( strncmp(opt_dbc, "xhci", 4) )
>>>>>>          return;
>>>>>
>>>>> ... this already avoids mixing up who's going to parse what. So right
>>>>> now I think that ...
>>>>>
>>>>>> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
>>>>>>      dbc->dbc_str = str_buf;
>>>>>>  
>>>>>>      if ( dbc_open(dbc) )
>>>>>> -        serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
>>>>>> +        serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
>>>>>>  }
>>>>>
>>>>> ... this and other SERHND_* related changes are enough, and there's no
>>>>> need for a separate "dbc=" option.
>>>>
>>>> But then you wouldn't be able to configure "dbgp=ehci dbgp=xhci" as
>>>> one would override the other, no?
>>>
>>> Not as long as both use string_param(), true. They'd need to both become
>>> custom_param(), doing at least some basic parsing right away.
>>>
>>> But using two such options at the same time isn't of interest anyway
>>> without your multiple-serial-consoles change, so possibly not of
>>> immediate need (unless someone comes forward expressing interest and
>>> actually approving that change of yours).
>>
>> Then why change at all? Since you can configure only one (dbgp=ehci _or_
>> dbgp=xhci), then there is not ambiguity what "console=dbgp" means.
>> Separating SERHND_DBC from SERHND_DBGP would IMO make sense only if you
>> can actually use them both (even if not both for console, but for
>> example one for debugger).
> 
> Or do you mean to use custom_param() to actually make "dbgp=xhci
> dbgp=ehci" working?

Yes.

> But then IMO having "console=dbgp console=dbc" would
> be confusing, as "dbc" has no obvious relation to neither side of
> "dbgp=xhci".

Well, there was never any idea of using multiple serial consoles, so
the present "console=dbgp" doesn't provide room for telling apart the
two. Just like there's no way to tell apart two EHCI controllers'
debug ports both (intended to be) used at the same time.

JanMaybe use "console=xhci" then?
>