* [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:19 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Connor Davis, Andrew Cooper,
George Dunlap, Jan Beulich, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné
From: Connor Davis <davisc@ainfosec.com>
[Connor]
Xue is a cross-platform USB 3 debugger that drives the Debug
Capability (DbC) of xHCI-compliant host controllers. This patch
implements the operations needed for xue to initialize the host
controller's DbC and communicate with it. It also implements a struct
uart_driver that uses xue as a backend. Note that only target -> host
communication is supported for now. To use Xue as a console, add
'console=dbgp dbgp=xhci' to the command line.
[Marek]
The Xue driver is taken from https://github.com/connojd/xue and heavily
refactored to fit into Xen code base. Major changes include:
- rename to xhci_dbc
- drop support for non-Xen systems
- drop xue_ops abstraction
- use Xen's native helper functions for PCI access
- move all the code to xue.c, drop "inline"
- build for x86 only
- annotate functions with cf_check
- adjust for Xen's code style
At this stage, only the first xHCI is considered. Later patch adds
support for choosing specific one.
The driver is initiallized before memory allocator works, so all the
transfer buffers (about 230KiB of them) are allocated statically and will
use memory even if XUE console is not selected. The driver can be
disabled build time to reclaim this memory.
Signed-off-by: Connor Davis <davisc@ainfosec.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- rename to xhci-dbc
- add empty stub for xhci_dbc_uart_init(), to avoid #ifdef in setup.c
- use PCI_BASE_ADDRESS_MEM_MASK
- make strings init more readable
- avoid infinite extended caps lookup
- size the whole 64bit BAR
- rename CONFIG_HAS_XHCI to CONFIG_XHCI
- use cpu_relax(), drop xue_sys_pause()
- disable memory decoding for the BAR sizing time, and enable it
explicitly
- drop mmio_size field - it's used only in init_xhc() function
internally
- use readl()/writel() for accessing MMIO
- add pci_ro_device(), to protect device before later patch(es) add
other protections
- fix setting dequeue pointer based on events: TRB ring was page
aligned, not 16-page aligned, so just taking DBC_TRB_RING_MASK bits
doesn't work; instead, calculate distance from the ring beginning;
while at it, fix off by one error there; dequeue pointer isn't used
much yet, but it will be useful for RX handling
- split dbc_ensure_running() out of dbc_flush() - it make dbc_flush()
more logical, and will make even more sense with RX support added
- make enum names upper case
Changes in v2:
- drop #pragma pack
- fix indentation in Kconfig
- minor style fixes
- use cache_flush()
- mark init functions as __init, and return bool
- fix PCI_SBDF usage, use constants from pci_regs.h
- add command line docs
- allow disabling the driver in menuconfig, to reclaim 2MB allocated
memory
- guard unused debug functions with #ifdef XUE_DEBUG
---
docs/misc/xen-command-line.pandoc | 5 +-
xen/arch/x86/include/asm/fixmap.h | 4 +-
xen/arch/x86/setup.c | 1 +-
xen/drivers/char/Kconfig | 9 +-
xen/drivers/char/Makefile | 1 +-
xen/drivers/char/xhci-dbc.c | 1024 ++++++++++++++++++++++++++++++-
xen/include/xen/serial.h | 5 +-
7 files changed, 1049 insertions(+)
create mode 100644 xen/drivers/char/xhci-dbc.c
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index da18172e50c5..f936283cd187 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -721,10 +721,15 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
+> `= xhci`
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.
+
### debug_stack_lines
> `= <integer>`
diff --git a/xen/arch/x86/include/asm/fixmap.h b/xen/arch/x86/include/asm/fixmap.h
index 20746afd0a2a..bc39ffe896b1 100644
--- a/xen/arch/x86/include/asm/fixmap.h
+++ b/xen/arch/x86/include/asm/fixmap.h
@@ -25,6 +25,8 @@
#include <asm/msi.h>
#include <acpi/apei.h>
+#define MAX_XHCI_PAGES 16
+
/*
* Here we define all the compile-time 'special' virtual
* addresses. The point is to have a constant address at
@@ -43,6 +45,8 @@ enum fixed_addresses {
FIX_COM_BEGIN,
FIX_COM_END,
FIX_EHCI_DBGP,
+ FIX_XHCI_BEGIN,
+ FIX_XHCI_END = FIX_XHCI_BEGIN + MAX_XHCI_PAGES - 1,
#ifdef CONFIG_XEN_GUEST
FIX_PV_CONSOLE,
FIX_XEN_SHARED_INFO,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b8dea6..e05189f64997 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -950,6 +950,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
ns16550.irq = 3;
ns16550_init(1, &ns16550);
ehci_dbgp_init();
+ xhci_dbc_uart_init();
console_init_preirq();
if ( pvh_boot )
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index dec58bc99360..06350c387371 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -84,3 +84,12 @@ config SERIAL_TX_BUFSIZE
the nearest power of 2.
Default value is 16384 (16kiB).
+
+config XHCI
+ bool "XHCI DbC UART driver"
+ depends on X86
+ help
+ This selects the USB based XHCI debug capability to be usable as a UART.
+ Enabling this option makes Xen use extra ~230KiB memory, even if XHCI UART
+ is not selected.
+ If you have an x86 based system with USB3, say Y.
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index 14e67cf072d7..e7e374775d32 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
obj-$(CONFIG_HAS_OMAP) += omap-uart.o
obj-$(CONFIG_HAS_SCIF) += scif-uart.o
obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
+obj-$(CONFIG_XHCI) += xhci-dbc.o
obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
obj-$(CONFIG_ARM) += arm-uart.o
obj-y += serial.o
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
new file mode 100644
index 000000000000..f0e60d1b86aa
--- /dev/null
+++ b/xen/drivers/char/xhci-dbc.c
@@ -0,0 +1,1024 @@
+/*
+ * drivers/char/xhci-dbc.c
+ *
+ * Xen port for the xue debugger
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2019 Assured Information Security.
+ */
+
+#include <xen/delay.h>
+#include <xen/mm.h>
+#include <xen/param.h>
+#include <xen/serial.h>
+#include <xen/timer.h>
+#include <xen/types.h>
+#include <asm/fixmap.h>
+#include <asm/io.h>
+#include <asm/string.h>
+#include <asm/system.h>
+
+/* uncomment to have dbc_uart_dump() debug function */
+/* #define DBC_DEBUG 1 */
+
+#define DBC_POLL_INTERVAL 100 /* us */
+
+#define DBC_PAGE_SIZE 4096U
+
+/* Supported xHC PCI configurations */
+#define DBC_XHC_CLASSC 0xC0330U
+
+/* DbC idVendor and idProduct */
+#define DBC_DBC_VENDOR 0x1D6B
+#define DBC_DBC_PRODUCT 0x0010
+#define DBC_DBC_PROTOCOL 0x0000
+
+/* DCCTRL fields */
+#define DBC_CTRL_DCR 0
+#define DBC_CTRL_HOT 2
+#define DBC_CTRL_HIT 3
+#define DBC_CTRL_DRC 4
+#define DBC_CTRL_DCE 31
+
+/* DCPORTSC fields */
+#define DBC_PSC_PED 1
+#define DBC_PSC_CSC 17
+#define DBC_PSC_PRC 21
+#define DBC_PSC_PLC 22
+#define DBC_PSC_CEC 23
+
+#define DBC_PSC_ACK_MASK \
+ ((1UL << DBC_PSC_CSC) | (1UL << DBC_PSC_PRC) | (1UL << DBC_PSC_PLC) | \
+ (1UL << DBC_PSC_CEC))
+
+#define dbc_debug(...) printk("dbc debug: " __VA_ARGS__)
+#define dbc_alert(...) printk("dbc alert: " __VA_ARGS__)
+#define dbc_error(...) printk("dbc error: " __VA_ARGS__)
+
+/******************************************************************************
+ * TRB ring (summarized from the manual):
+ *
+ * TRB rings are circular queues of TRBs shared between the xHC and the driver.
+ * Each ring has one producer and one consumer. The DbC has one event
+ * ring and two transfer rings; one IN and one OUT.
+ *
+ * The DbC hardware is the producer on the event ring, and
+ * dbc driver is the consumer. This means that event TRBs are read-only from
+ * the dbc driver.
+ *
+ * OTOH, dbc drive is the producer of transfer TRBs on the two transfer
+ * rings, so dbc driver enqueues transfers, and the hardware dequeues
+ * them. The dequeue pointer of a transfer ring is read by
+ * dbc driver by examining the latest transfer event TRB on the event ring. The
+ * transfer event TRB contains the address of the transfer TRB that generated
+ * the event.
+ *
+ * To make each transfer ring circular, the last TRB must be a link TRB, which
+ * points to the beginning of the next queue. Note that this implementation
+ * does not support multiple segments, so each link TRB points back to the
+ * beginning of its own segment.
+ ******************************************************************************/
+
+/* TRB types */
+enum {
+ XHCI_TRB_NORM = 1,
+ XHCI_TRB_LINK = 6,
+ XHCI_TRB_TFRE = 32,
+ XHCI_TRB_PSCE = 34
+};
+
+/* TRB completion codes */
+enum {
+ XHCI_TRB_CC_SUCCESS = 1,
+ XHCI_TRB_CC_TRB_ERR = 5,
+};
+
+/* DbC endpoint types */
+enum {
+ XHCI_EP_BULK_OUT = 2,
+ XHCI_EP_BULK_IN = 6,
+};
+
+/* DMA/MMIO structures */
+struct xhci_trb {
+ uint64_t params;
+ uint32_t status;
+ uint32_t ctrl;
+};
+
+/* log2(sizeof(struct xhci_trb)) */
+#define XHCI_TRB_SHIFT 4
+
+struct xhci_erst_segment {
+ uint64_t base;
+ uint16_t size;
+ uint8_t rsvdz[6];
+};
+
+/* Arbitrary length, must fit every DBC_STRING_* */
+#define MAX_STRING_LENGTH 16
+
+#define DBC_STRINGS_COUNT 4
+#define DBC_STRING_LANGID "\x09\x04"
+#define DBC_STRING_MANUFACTURER "Xen"
+#define DBC_STRING_PRODUCT "Debug console"
+#define DBC_STRING_SERIAL "0"
+
+#define XHCI_DT_STRING 3
+
+struct xhci_string_descriptor {
+ uint8_t size;
+ uint8_t type;
+ uint16_t string[MAX_STRING_LENGTH];
+};
+
+#define DBC_CTX_SIZE 16
+#define DBC_CTX_BYTES (DBC_CTX_SIZE * 4)
+
+struct xhci_dbc_ctx {
+ union {
+ uint32_t info[DBC_CTX_SIZE];
+ struct {
+ uint64_t string0_ptr;
+ uint64_t manufacturer_ptr;
+ uint64_t product_ptr;
+ uint64_t serial_ptr;
+ uint8_t string0_size;
+ uint8_t manufacturer_size;
+ uint8_t product_size;
+ uint8_t serial_size;
+ };
+ };
+ uint32_t ep_out[DBC_CTX_SIZE];
+ uint32_t ep_in[DBC_CTX_SIZE];
+};
+
+struct dbc_reg {
+ uint32_t id;
+ uint32_t db;
+ uint32_t erstsz;
+ uint32_t rsvdz;
+ uint64_t erstba;
+ uint64_t erdp;
+ uint32_t ctrl;
+ uint32_t st;
+ uint32_t portsc;
+ uint32_t rsvdp;
+ uint64_t cp;
+ uint32_t ddi1;
+ uint32_t ddi2;
+};
+
+#define DBC_TRB_MAX_TFR (DBC_PAGE_SIZE << 4)
+#define DBC_TRB_PER_PAGE (DBC_PAGE_SIZE / sizeof(struct xhci_trb))
+
+/* Defines the size in bytes of TRB rings as 2^DBC_TRB_RING_ORDER * 4096 */
+#ifndef DBC_TRB_RING_ORDER
+#define DBC_TRB_RING_ORDER 4
+#endif
+#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE * (1 << DBC_TRB_RING_ORDER))
+#define DBC_TRB_RING_BYTES (DBC_TRB_RING_CAP * sizeof(struct xhci_trb))
+#define DBC_TRB_RING_MASK (DBC_TRB_RING_BYTES - 1U)
+
+struct xhci_trb_ring {
+ struct xhci_trb *trb; /* Array of TRBs */
+ uint32_t enq; /* The offset of the enqueue ptr */
+ uint32_t deq; /* The offset of the dequeue ptr */
+ uint8_t cyc; /* Cycle state toggled on each wrap-around */
+ uint8_t db; /* Doorbell target */
+ uint64_t dma; /* Physical address (for the device) */
+};
+
+#define DBC_DB_OUT 0x0
+#define DBC_DB_IN 0x1
+#define DBC_DB_INVAL 0xFF
+
+/* Defines the size in bytes of work rings as 2^DBC_WORK_RING_ORDER * 4096 */
+#ifndef DBC_WORK_RING_ORDER
+#define DBC_WORK_RING_ORDER 3
+#endif
+#define DBC_WORK_RING_CAP (DBC_PAGE_SIZE * (1 << DBC_WORK_RING_ORDER))
+#define DBC_WORK_RING_BYTES DBC_WORK_RING_CAP
+
+#if DBC_WORK_RING_CAP > DBC_TRB_MAX_TFR
+#error "DBC_WORK_RING_ORDER must be at most 4"
+#endif
+
+struct dbc_work_ring {
+ uint8_t *buf;
+ uint32_t enq;
+ uint32_t deq;
+ uint64_t dma;
+};
+
+struct dbc {
+ struct dbc_reg __iomem *dbc_reg;
+ struct xhci_dbc_ctx *dbc_ctx;
+ struct xhci_erst_segment *dbc_erst;
+ struct xhci_trb_ring dbc_ering;
+ struct xhci_trb_ring dbc_oring;
+ struct xhci_trb_ring dbc_iring;
+ struct dbc_work_ring dbc_owork;
+ struct xhci_string_descriptor *dbc_str;
+
+ pci_sbdf_t sbdf;
+ uint64_t xhc_mmio_phys;
+ uint64_t xhc_dbc_offset;
+ void __iomem *xhc_mmio;
+
+ bool open;
+};
+
+static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
+{
+ size_t i;
+
+ if ( size != MAX_XHCI_PAGES * DBC_PAGE_SIZE )
+ return NULL;
+
+ for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
+ {
+ set_fixmap_nocache(i, phys);
+ phys += DBC_PAGE_SIZE;
+ }
+
+ /*
+ * The fixmap grows downward, so the lowest virt is
+ * at the highest index
+ */
+ return fix_to_virt(FIX_XHCI_END);
+}
+
+static bool __init dbc_init_xhc(struct dbc *dbc)
+{
+ uint32_t bar0;
+ uint64_t bar1;
+ uint64_t bar_size;
+ uint64_t devfn;
+ uint16_t cmd;
+ size_t xhc_mmio_size;
+
+ /*
+ * Search PCI bus 0 for the xHC. All the host controllers supported so far
+ * are part of the chipset and are on bus 0.
+ */
+ for ( devfn = 0; devfn < 256; devfn++ )
+ {
+ pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
+ uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
+
+ if ( hdr == 0 || hdr == 0x80 )
+ {
+ if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == DBC_XHC_CLASSC )
+ {
+ dbc->sbdf = sbdf;
+ break;
+ }
+ }
+ }
+
+ if ( !dbc->sbdf.sbdf )
+ {
+ dbc_error("Compatible xHC not found on bus 0\n");
+ return false;
+ }
+
+ /* ...we found it, so parse the BAR and map the registers */
+ bar0 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
+ bar1 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1);
+
+ /* IO BARs not allowed; BAR must be 64-bit */
+ if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY ||
+ (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != PCI_BASE_ADDRESS_MEM_TYPE_64 )
+ return false;
+
+ cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
+
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, 0xFFFFFFFF);
+ bar_size = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
+ bar_size |= (uint64_t)pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1) << 32;
+ xhc_mmio_size = ~(bar_size & PCI_BASE_ADDRESS_MEM_MASK) + 1;
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, bar0);
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, bar1);
+
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
+
+ dbc->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
+ dbc->xhc_mmio = dbc_sys_map_xhc(dbc->xhc_mmio_phys, xhc_mmio_size);
+
+ if ( dbc->xhc_mmio == NULL )
+ return false;
+
+ if ( (cmd & PCI_COMMAND_MEMORY) == 0 )
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
+
+ return true;
+}
+
+/**
+ * The first register of the debug capability is found by traversing the
+ * host controller's capability list (xcap) until a capability
+ * with ID = 0xA is found. The xHCI capability list begins at address
+ * mmio + (HCCPARAMS1[31:16] << 2).
+ */
+static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
+{
+ uint32_t *xcap;
+ uint32_t xcap_val;
+ uint32_t next;
+ uint32_t id = 0;
+ uint8_t *mmio = (uint8_t *)dbc->xhc_mmio;
+ uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
+ const uint32_t DBC_ID = 0xA;
+ int ttl = 48;
+
+ xcap = (uint32_t *)dbc->xhc_mmio;
+ /*
+ * This is initially an offset to the first capability. All the offsets
+ * (both in HCCP1 and then next capability pointer are dword-based.
+ */
+ next = (readl(hccp1) & 0xFFFF0000) >> 16;
+
+ while ( id != DBC_ID && next && ttl-- )
+ {
+ xcap += next;
+ xcap_val = readl(xcap);
+ id = xcap_val & 0xFF;
+ next = (xcap_val & 0xFF00) >> 8;
+ }
+
+ if ( id != DBC_ID )
+ return NULL;
+
+ dbc->xhc_dbc_offset = (uint64_t)xcap - (uint64_t)mmio;
+ return (struct dbc_reg __iomem *)xcap;
+}
+
+/**
+ * Fields with the same interpretation for every TRB type (section 4.11.1).
+ * These are the fields defined in the TRB template, minus the ENT bit. That
+ * bit is the toggle cycle bit in link TRBs, so it shouldn't be in the
+ * template.
+ */
+static uint32_t xhci_trb_cyc(const struct xhci_trb *trb)
+{
+ return trb->ctrl & 0x1;
+}
+
+static uint32_t xhci_trb_type(const struct xhci_trb *trb)
+{
+ return (trb->ctrl & 0xFC00) >> 10;
+}
+
+static void xhci_trb_set_cyc(struct xhci_trb *trb, uint32_t c)
+{
+ trb->ctrl &= ~0x1U;
+ trb->ctrl |= c;
+}
+
+static void xhci_trb_set_type(struct xhci_trb *trb, uint32_t t)
+{
+ trb->ctrl &= ~0xFC00U;
+ trb->ctrl |= (t << 10);
+}
+
+/* Fields for normal TRBs */
+static void xhci_trb_norm_set_buf(struct xhci_trb *trb, uint64_t addr)
+{
+ trb->params = addr;
+}
+
+static void xhci_trb_norm_set_len(struct xhci_trb *trb, uint32_t len)
+{
+ trb->status &= ~0x1FFFFU;
+ trb->status |= len;
+}
+
+static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
+{
+ trb->ctrl |= 0x20;
+}
+
+/**
+ * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
+ * TRBs are read-only from software
+ */
+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;
+}
+
+/* Fields for link TRBs (section 6.4.4.1) */
+static void xhci_trb_link_set_rsp(struct xhci_trb *trb, uint64_t rsp)
+{
+ trb->params = rsp;
+}
+
+static void xhci_trb_link_set_tc(struct xhci_trb *trb)
+{
+ trb->ctrl |= 0x2;
+}
+
+static void xhci_trb_ring_init(const struct dbc *dbc,
+ struct xhci_trb_ring *ring, int producer,
+ int doorbell)
+{
+ memset(ring->trb, 0, DBC_TRB_RING_CAP * sizeof(ring->trb[0]));
+
+ ring->enq = 0;
+ ring->deq = 0;
+ ring->cyc = 1;
+ ring->db = (uint8_t)doorbell;
+ ring->dma = virt_to_maddr(ring->trb);
+
+ /*
+ * Producer implies transfer ring, so we have to place a
+ * link TRB at the end that points back to trb[0]
+ */
+ if ( producer )
+ {
+ struct xhci_trb *trb = &ring->trb[DBC_TRB_RING_CAP - 1];
+ xhci_trb_set_type(trb, XHCI_TRB_LINK);
+ xhci_trb_link_set_tc(trb);
+ xhci_trb_link_set_rsp(trb, virt_to_maddr(ring->trb));
+ }
+}
+
+static int xhci_trb_ring_full(const struct xhci_trb_ring *ring)
+{
+ return ((ring->enq + 1) & (DBC_TRB_RING_CAP - 1)) == ring->deq;
+}
+
+static int dbc_work_ring_full(const struct dbc_work_ring *ring)
+{
+ return ((ring->enq + 1) & (DBC_WORK_RING_CAP - 1)) == ring->deq;
+}
+
+static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring)
+{
+ if ( ring->enq >= ring->deq )
+ return ring->enq - ring->deq;
+
+ return DBC_WORK_RING_CAP - ring->deq + ring->enq;
+}
+
+static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring,
+ uint64_t dma, uint64_t len)
+{
+ struct xhci_trb trb;
+
+ if ( ring->enq == DBC_TRB_RING_CAP - 1 )
+ {
+ /*
+ * We have to make sure the xHC processes the link TRB in order
+ * for wrap-around to work properly. We do this by marking the
+ * xHC as owner of the link TRB by setting the TRB's cycle bit
+ * (just like with normal TRBs).
+ */
+ struct xhci_trb *link = &ring->trb[ring->enq];
+ xhci_trb_set_cyc(link, ring->cyc);
+
+ ring->enq = 0;
+ ring->cyc ^= 1;
+ }
+
+ trb.params = 0;
+ trb.status = 0;
+ trb.ctrl = 0;
+
+ xhci_trb_set_type(&trb, XHCI_TRB_NORM);
+ xhci_trb_set_cyc(&trb, ring->cyc);
+
+ xhci_trb_norm_set_buf(&trb, dma);
+ xhci_trb_norm_set_len(&trb, (uint32_t)len);
+ xhci_trb_norm_set_ioc(&trb);
+
+ ring->trb[ring->enq++] = trb;
+ cache_flush(&ring->trb[ring->enq - 1], sizeof(trb));
+}
+
+static int64_t dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring,
+ const char *buf, unsigned int len)
+{
+ unsigned int i = 0;
+ unsigned int end, start = ring->enq;
+
+ while ( !dbc_work_ring_full(ring) && i < len )
+ {
+ ring->buf[ring->enq] = buf[i++];
+ ring->enq = (ring->enq + 1) & (DBC_WORK_RING_CAP - 1);
+ }
+
+ end = ring->enq;
+
+ if ( end > start )
+ cache_flush(&ring->buf[start], end - start);
+ else if ( i > 0 )
+ {
+ cache_flush(&ring->buf[start], DBC_WORK_RING_CAP - start);
+ cache_flush(&ring->buf[0], end);
+ }
+
+ return i;
+}
+
+/*
+ * 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;
+ struct xhci_trb_ring *er = &dbc->dbc_ering;
+ struct xhci_trb_ring *tr = &dbc->dbc_oring;
+ struct xhci_trb *event = &er->trb[er->deq];
+ uint64_t erdp = readq(®->erdp);
+ uint32_t portsc;
+ uint64_t event_ptr;
+ unsigned int trb_idx;
+
+ BUILD_BUG_ON((1 << XHCI_TRB_SHIFT) != sizeof(struct xhci_trb));
+
+ rmb();
+
+ while ( xhci_trb_cyc(event) == er->cyc )
+ {
+ switch (xhci_trb_type(event))
+ {
+ case XHCI_TRB_TFRE:
+ event_ptr = xhci_trb_tfre_ptr(event);
+ /*
+ * trb_idx is just completed TRB, so set the dequeue ptr one
+ * position further.
+ */
+ if ( event_ptr - tr->dma < DBC_TRB_RING_BYTES )
+ {
+ trb_idx = (event_ptr - tr->dma) >> XHCI_TRB_SHIFT;
+ tr->deq = (trb_idx + 1) & (DBC_TRB_RING_CAP - 1);
+ }
+ else
+ dbc_alert("event: TRB 0x%lx not found in any ring\n",
+ event_ptr);
+ break;
+ case XHCI_TRB_PSCE:
+ portsc = readl(®->portsc);
+ portsc |= DBC_PSC_ACK_MASK & portsc;
+ writel(portsc, ®->portsc);
+ break;
+ default:
+ break;
+ }
+
+ er->cyc = (er->deq == DBC_TRB_RING_CAP - 1) ? er->cyc ^ 1 : er->cyc;
+ er->deq = (er->deq + 1) & (DBC_TRB_RING_CAP - 1);
+ event = &er->trb[er->deq];
+ }
+
+ erdp = er->dma + (er->deq << XHCI_TRB_SHIFT);
+ wmb();
+ writeq(erdp, ®->erdp);
+}
+
+/**
+ * dbc_init_ep
+ *
+ * Initializes the endpoint as specified in sections 7.6.3.2 and 7.6.9.2.
+ * Each endpoint is Bulk, so the MaxPStreams, LSA, HID, CErr, FE,
+ * Interval, Mult, and Max ESIT Payload fields are all 0.
+ *
+ * Max packet size: 1024
+ * Max burst size: debug mbs (from dbc_reg->ctrl register)
+ * EP type: 2 for OUT bulk, 6 for IN bulk
+ * TR dequeue ptr: physical base address of transfer ring
+ * Avg TRB length: software defined (see 4.14.1.1 for suggested defaults)
+ */
+static void dbc_init_ep(uint32_t *ep, uint64_t mbs, uint32_t type,
+ uint64_t ring_dma)
+{
+ memset(ep, 0, DBC_CTX_BYTES);
+
+ ep[1] = (1024 << 16) | ((uint32_t)mbs << 8) | (type << 3);
+ ep[2] = (ring_dma & 0xFFFFFFFF) | 1;
+ ep[3] = ring_dma >> 32;
+ ep[4] = 3 * 1024;
+}
+
+static void dbc_init_string_single(struct xhci_string_descriptor *string,
+ char *ascii_str,
+ uint64_t *str_ptr,
+ uint8_t *str_size_ptr)
+{
+ size_t i, len = strlen(ascii_str);
+
+ string->size = offsetof(typeof(*string), string) + len * 2;
+ string->type = XHCI_DT_STRING;
+ /* ASCII to UTF16 conversion */
+ for (i = 0; i < len; i++)
+ string->string[i] = ascii_str[i];
+ *str_ptr = virt_to_maddr(string);
+ *str_size_ptr = string->size;
+}
+
+/* Initialize the DbC info with USB string descriptor addresses */
+static void dbc_init_strings(struct dbc *dbc, uint32_t *info)
+{
+ BUILD_BUG_ON(sizeof(DBC_STRING_LANGID) > MAX_STRING_LENGTH);
+ BUILD_BUG_ON(sizeof(DBC_STRING_MANUFACTURER) > MAX_STRING_LENGTH);
+ BUILD_BUG_ON(sizeof(DBC_STRING_PRODUCT) > MAX_STRING_LENGTH);
+ BUILD_BUG_ON(sizeof(DBC_STRING_SERIAL) > MAX_STRING_LENGTH);
+
+ dbc_init_string_single(&dbc->dbc_str[0], DBC_STRING_LANGID,
+ &dbc->dbc_ctx->string0_ptr,
+ &dbc->dbc_ctx->string0_size);
+ dbc_init_string_single(&dbc->dbc_str[1], DBC_STRING_MANUFACTURER,
+ &dbc->dbc_ctx->manufacturer_ptr,
+ &dbc->dbc_ctx->manufacturer_size);
+ dbc_init_string_single(&dbc->dbc_str[2], DBC_STRING_PRODUCT,
+ &dbc->dbc_ctx->product_ptr,
+ &dbc->dbc_ctx->product_size);
+ dbc_init_string_single(&dbc->dbc_str[3], DBC_STRING_SERIAL,
+ &dbc->dbc_ctx->serial_ptr,
+ &dbc->dbc_ctx->serial_size);
+}
+
+static void dbc_enable_dbc(struct dbc *dbc)
+{
+ struct dbc_reg *reg = dbc->dbc_reg;
+
+ wmb();
+ writel(readl(®->ctrl) | (1U << DBC_CTRL_DCE), ®->ctrl);
+ wmb();
+
+ while ( (readl(®->ctrl) & (1U << DBC_CTRL_DCE)) == 0 )
+ cpu_relax();
+
+ wmb();
+ writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc);
+ wmb();
+
+ while ( (readl(®->ctrl) & (1U << DBC_CTRL_DCR)) == 0 )
+ cpu_relax();
+}
+
+static void dbc_disable_dbc(struct dbc *dbc)
+{
+ struct dbc_reg *reg = dbc->dbc_reg;
+
+ writel(readl(®->portsc) & ~(1U << DBC_PSC_PED), ®->portsc);
+ wmb();
+ writel(readl(®->ctrl) & ~(1U << DBC_CTRL_DCE), ®->ctrl);
+
+ while ( readl(®->ctrl) & (1U << DBC_CTRL_DCE) )
+ cpu_relax();
+}
+
+static int dbc_init_dbc(struct dbc *dbc)
+{
+ uint64_t erdp = 0;
+ uint64_t mbs = 0;
+ uint16_t cmd;
+ struct dbc_reg *reg = xhci_find_dbc(dbc);
+
+ if ( !reg )
+ return 0;
+
+ dbc->dbc_reg = reg;
+ dbc_disable_dbc(dbc);
+
+ xhci_trb_ring_init(dbc, &dbc->dbc_ering, 0, DBC_DB_INVAL);
+ xhci_trb_ring_init(dbc, &dbc->dbc_oring, 1, DBC_DB_OUT);
+ xhci_trb_ring_init(dbc, &dbc->dbc_iring, 1, DBC_DB_IN);
+
+ erdp = virt_to_maddr(dbc->dbc_ering.trb);
+ if ( !erdp )
+ return 0;
+
+ memset(dbc->dbc_erst, 0, sizeof(*dbc->dbc_erst));
+ dbc->dbc_erst->base = erdp;
+ dbc->dbc_erst->size = DBC_TRB_RING_CAP;
+
+ mbs = (readl(®->ctrl) & 0xFF0000) >> 16;
+
+ memset(dbc->dbc_ctx, 0, sizeof(*dbc->dbc_ctx));
+ dbc_init_strings(dbc, dbc->dbc_ctx->info);
+ dbc_init_ep(dbc->dbc_ctx->ep_out, mbs, XHCI_EP_BULK_OUT,
+ dbc->dbc_oring.dma);
+ dbc_init_ep(dbc->dbc_ctx->ep_in, mbs, XHCI_EP_BULK_IN,
+ dbc->dbc_iring.dma);
+
+ writel(1, ®->erstsz);
+ writeq(virt_to_maddr(dbc->dbc_erst), ®->erstba);
+ writeq(erdp, ®->erdp);
+ writeq(virt_to_maddr(dbc->dbc_ctx), ®->cp);
+ writel((DBC_DBC_VENDOR << 16) | DBC_DBC_PROTOCOL, ®->ddi1);
+ writel(DBC_DBC_PRODUCT, ®->ddi2);
+
+ cache_flush(dbc->dbc_ctx, sizeof(*dbc->dbc_ctx));
+ cache_flush(dbc->dbc_erst, sizeof(*dbc->dbc_erst));
+ cache_flush(dbc->dbc_ering.trb, DBC_TRB_RING_BYTES);
+ cache_flush(dbc->dbc_oring.trb, DBC_TRB_RING_BYTES);
+ cache_flush(dbc->dbc_iring.trb, DBC_TRB_RING_BYTES);
+ cache_flush(dbc->dbc_owork.buf, DBC_WORK_RING_BYTES);
+
+ cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd | PCI_COMMAND_MASTER);
+
+ return 1;
+}
+
+static void dbc_init_work_ring(struct dbc *dbc,
+ struct dbc_work_ring *wrk)
+{
+ wrk->enq = 0;
+ wrk->deq = 0;
+ wrk->dma = virt_to_maddr(wrk->buf);
+}
+
+/**
+ * Initialize the DbC and enable it for transfers. First map in the DbC
+ * registers from the host controller's MMIO region. Then allocate and map
+ * DMA for the event and transfer rings. Finally, enable the DbC for
+ * the host to enumerate. On success, the DbC is ready to send packets.
+ *
+ * @param dbc the dbc to open (!= NULL)
+ * @return true iff dbc_open succeeded
+ */
+static bool __init dbc_open(struct dbc *dbc)
+{
+ if ( !dbc )
+ return false;
+
+ if ( !dbc_init_xhc(dbc) )
+ return false;
+
+ if ( !dbc_init_dbc(dbc) )
+ return false;
+
+ dbc_init_work_ring(dbc, &dbc->dbc_owork);
+ dbc_enable_dbc(dbc);
+ dbc->open = true;
+
+ return true;
+}
+
+/*
+ * Ensure DbC is still running, handle events, and possibly re-enable if cable
+ * was re-plugged. Returns true if DbC is operational.
+ */
+static bool dbc_ensure_running(struct dbc *dbc)
+{
+ struct dbc_reg *reg = dbc->dbc_reg;
+ uint32_t ctrl;
+ uint32_t cmd;
+
+ dbc_pop_events(dbc);
+
+ ctrl = readl(®->ctrl);
+ if ( !(ctrl & (1U << DBC_CTRL_DCR)) )
+ {
+ return false;
+ }
+
+ if ( ctrl & (1U << DBC_CTRL_DRC) )
+ {
+ writel(ctrl | (1U << DBC_CTRL_DRC), ®->ctrl);
+ writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc);
+ wmb();
+ }
+
+ return true;
+}
+
+/**
+ * Commit the pending transfer TRBs to the DbC. This notifies
+ * the DbC of any previously-queued data on the work ring and
+ * rings the doorbell.
+ *
+ * @param dbc the dbc to flush
+ * @param trb the ring containing the TRBs to transfer
+ * @param wrk the work ring containing data to be flushed
+ */
+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(®->db) & 0xFFFF00FF) | (trb->db << 8);
+
+ if ( xhci_trb_ring_full(trb) )
+ return;
+
+ if ( wrk->enq == wrk->deq )
+ return;
+ else if ( wrk->enq > wrk->deq )
+ {
+ dbc_push_trb(dbc, trb, wrk->dma + wrk->deq, wrk->enq - wrk->deq);
+ wrk->deq = wrk->enq;
+ }
+ else
+ {
+ dbc_push_trb(dbc, trb, wrk->dma + wrk->deq,
+ DBC_WORK_RING_CAP - wrk->deq);
+ wrk->deq = 0;
+ if ( wrk->enq > 0 && !xhci_trb_ring_full(trb) )
+ {
+ dbc_push_trb(dbc, trb, wrk->dma, wrk->enq);
+ wrk->deq = wrk->enq;
+ }
+ }
+
+ wmb();
+ writel(db, ®->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.
+ *
+ * @param dbc the dbc to write to
+ * @param c the character to write
+ * @return the number of bytes written
+ */
+static int64_t dbc_putc(struct dbc *dbc, char c)
+{
+ if ( !dbc_push_work(dbc, &dbc->dbc_owork, &c, 1) )
+ return 0;
+
+ if ( !dbc_ensure_running(dbc) )
+ return 1;
+
+ if ( c == '\n' )
+ dbc_flush(dbc, &dbc->dbc_oring, &dbc->dbc_owork);
+
+ return 1;
+}
+
+struct dbc_uart {
+ struct dbc dbc;
+ struct timer timer;
+ spinlock_t *lock;
+};
+
+static struct dbc_uart dbc_uart;
+
+static void cf_check dbc_uart_poll(void *data)
+{
+ struct serial_port *port = data;
+ struct dbc_uart *uart = port->uart;
+ struct dbc *dbc = &uart->dbc;
+ unsigned long flags = 0;
+
+ 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);
+ }
+
+ serial_tx_interrupt(port, guest_cpu_user_regs());
+ set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL));
+}
+
+static void __init cf_check dbc_uart_init_preirq(struct serial_port *port)
+{
+ struct dbc_uart *uart = port->uart;
+ uart->lock = &port->tx_lock;
+}
+
+static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
+{
+ struct dbc_uart *uart = port->uart;
+
+ serial_async_transmit(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);
+}
+
+static int cf_check dbc_uart_tx_ready(struct serial_port *port)
+{
+ struct dbc_uart *uart = port->uart;
+ struct dbc *dbc = &uart->dbc;
+
+ return DBC_WORK_RING_CAP - dbc_work_ring_size(&dbc->dbc_owork);
+}
+
+static void cf_check dbc_uart_putc(struct serial_port *port, char c)
+{
+ struct dbc_uart *uart = port->uart;
+ dbc_putc(&uart->dbc, c);
+}
+
+static void cf_check dbc_uart_flush(struct serial_port *port)
+{
+ s_time_t goal;
+ struct dbc_uart *uart = port->uart;
+ struct dbc *dbc = &uart->dbc;
+
+ if ( dbc_ensure_running(dbc) )
+ dbc_flush(dbc, &dbc->dbc_oring, &dbc->dbc_owork);
+
+ goal = NOW() + MICROSECS(DBC_POLL_INTERVAL);
+ if ( uart->timer.expires > goal )
+ set_timer(&uart->timer, goal);
+}
+
+static struct uart_driver dbc_uart_driver = {
+ .init_preirq = dbc_uart_init_preirq,
+ .init_postirq = dbc_uart_init_postirq,
+ .tx_ready = dbc_uart_tx_ready,
+ .putc = dbc_uart_putc,
+ .flush = dbc_uart_flush,
+};
+
+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(64);
+static struct xhci_dbc_ctx ctx __aligned(64);
+static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
+static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+static char __initdata opt_dbgp[30];
+
+string_param("dbgp", opt_dbgp);
+
+void __init xhci_dbc_uart_init(void)
+{
+ struct dbc_uart *uart = &dbc_uart;
+ struct dbc *dbc = &uart->dbc;
+
+ if ( strncmp(opt_dbgp, "xhci", 4) )
+ return;
+
+ memset(dbc, 0, sizeof(*dbc));
+
+ 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;
+
+ if ( dbc_open(dbc) )
+ serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
+}
+
+#ifdef DBC_DEBUG
+static void dbc_dump(struct dbc *dbc)
+{
+ struct dbc_reg *r = dbc->dbc_reg;
+
+ dbc_debug("XHCI DBC DUMP:\n");
+ dbc_debug(" ctrl: 0x%x stat: 0x%x psc: 0x%x\n",
+ readl(&r->ctrl), readl(&r->st), readl(&r->portsc));
+ dbc_debug(" id: 0x%x, db: 0x%x\n",
+ readl(&r->id), readl(&r->db));
+ dbc_debug(" erstsz: %u, erstba: 0x%lx\n",
+ readl(&r->erstsz), readq(&r->erstba));
+ dbc_debug(" erdp: 0x%lx, cp: 0x%lx\n",
+ readq(&r->erdp), readq(&r->cp));
+ dbc_debug(" ddi1: 0x%x, ddi2: 0x%x\n",
+ readl(&r->ddi1), readl(&r->ddi2));
+ dbc_debug(" erstba == virt_to_dma(erst): %d\n",
+ readq(&r->erstba) == virt_to_maddr(dbc->dbc_erst));
+ dbc_debug(" erdp == virt_to_dma(erst[0].base): %d\n",
+ readq(&r->erdp) == dbc->dbc_erst[0].base);
+ dbc_debug(" cp == virt_to_dma(ctx): %d\n",
+ readq(&r->cp) == virt_to_maddr(dbc->dbc_ctx));
+}
+
+static void dbc_uart_dump(void)
+{
+ struct dbc_uart *uart = &dbc_uart;
+ struct dbc *dbc = &uart->dbc;
+
+ dbc_dump(dbc);
+}
+#endif
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 6548f0b0a9cf..181e026967bc 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -171,6 +171,11 @@ struct ns16550_defaults {
};
void ns16550_init(int index, struct ns16550_defaults *defaults);
void ehci_dbgp_init(void);
+#ifdef CONFIG_XHCI
+void xhci_dbc_uart_init(void);
+#else
+static void inline xhci_dbc_uart_init(void) {};
+#endif
void arm_uart_init(void);
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability
@ 2022-07-26 3:23 Marek Marczykowski-Górecki
2022-07-26 3:23 ` Marek Marczykowski-Górecki
` (10 more replies)
0 siblings, 11 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Roger Pau Monné,
Paul Durrant, Kevin Tian, Connor Davis
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
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>
Connor Davis (1):
drivers/char: Add support for USB3 DbC debugger
Marek Marczykowski-Górecki (9):
drivers/char: reset XHCI ports when initializing dbc
drivers/char: add support for selecting specific xhci
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: allow driving the rest of XHCI by a domain while Xen uses DbC
driver/char: add RX support to the XHCI driver
docs/misc/xen-command-line.pandoc | 19 +-
xen/arch/x86/include/asm/fixmap.h | 4 +-
xen/arch/x86/setup.c | 1 +-
xen/drivers/char/Kconfig | 20 +-
xen/drivers/char/Makefile | 1 +-
xen/drivers/char/console.c | 97 +-
xen/drivers/char/xhci-dbc.c | 1367 +++++++++++++++++++++++-
xen/drivers/passthrough/amd/iommu_acpi.c | 21 +-
xen/drivers/passthrough/iommu.c | 45 +-
xen/drivers/passthrough/vtd/dmar.c | 201 +--
xen/include/xen/iommu.h | 13 +-
xen/include/xen/serial.h | 6 +-
12 files changed, 1690 insertions(+), 105 deletions(-)
create mode 100644 xen/drivers/char/xhci-dbc.c
base-commit: 4df2e99d731402da48afb19dc970ccab5a0814d6
--
git-series 0.9.1
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu,
Roger Pau Monné
From: Connor Davis <davisc@ainfosec.com>
[Connor]
Xue is a cross-platform USB 3 debugger that drives the Debug
Capability (DbC) of xHCI-compliant host controllers. This patch
implements the operations needed for xue to initialize the host
controller's DbC and communicate with it. It also implements a struct
uart_driver that uses xue as a backend. Note that only target -> host
communication is supported for now. To use Xue as a console, add
'console=dbgp dbgp=xhci' to the command line.
[Marek]
The Xue driver is taken from https://github.com/connojd/xue and heavily
refactored to fit into Xen code base. Major changes include:
- rename to xhci_dbc
- drop support for non-Xen systems
- drop xue_ops abstraction
- use Xen's native helper functions for PCI access
- move all the code to xue.c, drop "inline"
- build for x86 only
- annotate functions with cf_check
- adjust for Xen's code style
At this stage, only the first xHCI is considered. Later patch adds
support for choosing specific one.
The driver is initiallized before memory allocator works, so all the
transfer buffers (about 230KiB of them) are allocated statically and will
use memory even if XUE console is not selected. The driver can be
disabled build time to reclaim this memory.
Signed-off-by: Connor Davis <davisc@ainfosec.com>
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- rename to xhci-dbc
- add empty stub for xhci_dbc_uart_init(), to avoid #ifdef in setup.c
- use PCI_BASE_ADDRESS_MEM_MASK
- make strings init more readable
- avoid infinite extended caps lookup
- size the whole 64bit BAR
- rename CONFIG_HAS_XHCI to CONFIG_XHCI
- use cpu_relax(), drop xue_sys_pause()
- disable memory decoding for the BAR sizing time, and enable it
explicitly
- drop mmio_size field - it's used only in init_xhc() function
internally
- use readl()/writel() for accessing MMIO
- add pci_ro_device(), to protect device before later patch(es) add
other protections
- fix setting dequeue pointer based on events: TRB ring was page
aligned, not 16-page aligned, so just taking DBC_TRB_RING_MASK bits
doesn't work; instead, calculate distance from the ring beginning;
while at it, fix off by one error there; dequeue pointer isn't used
much yet, but it will be useful for RX handling
- split dbc_ensure_running() out of dbc_flush() - it make dbc_flush()
more logical, and will make even more sense with RX support added
- make enum names upper case
Changes in v2:
- drop #pragma pack
- fix indentation in Kconfig
- minor style fixes
- use cache_flush()
- mark init functions as __init, and return bool
- fix PCI_SBDF usage, use constants from pci_regs.h
- add command line docs
- allow disabling the driver in menuconfig, to reclaim 2MB allocated
memory
- guard unused debug functions with #ifdef XUE_DEBUG
---
docs/misc/xen-command-line.pandoc | 5 +-
xen/arch/x86/include/asm/fixmap.h | 4 +-
xen/arch/x86/setup.c | 1 +-
xen/drivers/char/Kconfig | 9 +-
xen/drivers/char/Makefile | 1 +-
xen/drivers/char/xhci-dbc.c | 1024 ++++++++++++++++++++++++++++++-
xen/include/xen/serial.h | 5 +-
7 files changed, 1049 insertions(+)
create mode 100644 xen/drivers/char/xhci-dbc.c
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index da18172e50c5..f936283cd187 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -721,10 +721,15 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
+> `= xhci`
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.
+
### debug_stack_lines
> `= <integer>`
diff --git a/xen/arch/x86/include/asm/fixmap.h b/xen/arch/x86/include/asm/fixmap.h
index 20746afd0a2a..bc39ffe896b1 100644
--- a/xen/arch/x86/include/asm/fixmap.h
+++ b/xen/arch/x86/include/asm/fixmap.h
@@ -25,6 +25,8 @@
#include <asm/msi.h>
#include <acpi/apei.h>
+#define MAX_XHCI_PAGES 16
+
/*
* Here we define all the compile-time 'special' virtual
* addresses. The point is to have a constant address at
@@ -43,6 +45,8 @@ enum fixed_addresses {
FIX_COM_BEGIN,
FIX_COM_END,
FIX_EHCI_DBGP,
+ FIX_XHCI_BEGIN,
+ FIX_XHCI_END = FIX_XHCI_BEGIN + MAX_XHCI_PAGES - 1,
#ifdef CONFIG_XEN_GUEST
FIX_PV_CONSOLE,
FIX_XEN_SHARED_INFO,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index f08b07b8dea6..e05189f64997 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -950,6 +950,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
ns16550.irq = 3;
ns16550_init(1, &ns16550);
ehci_dbgp_init();
+ xhci_dbc_uart_init();
console_init_preirq();
if ( pvh_boot )
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index dec58bc99360..06350c387371 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -84,3 +84,12 @@ config SERIAL_TX_BUFSIZE
the nearest power of 2.
Default value is 16384 (16kiB).
+
+config XHCI
+ bool "XHCI DbC UART driver"
+ depends on X86
+ help
+ This selects the USB based XHCI debug capability to be usable as a UART.
+ Enabling this option makes Xen use extra ~230KiB memory, even if XHCI UART
+ is not selected.
+ If you have an x86 based system with USB3, say Y.
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index 14e67cf072d7..e7e374775d32 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
obj-$(CONFIG_HAS_OMAP) += omap-uart.o
obj-$(CONFIG_HAS_SCIF) += scif-uart.o
obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
+obj-$(CONFIG_XHCI) += xhci-dbc.o
obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
obj-$(CONFIG_ARM) += arm-uart.o
obj-y += serial.o
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
new file mode 100644
index 000000000000..f0e60d1b86aa
--- /dev/null
+++ b/xen/drivers/char/xhci-dbc.c
@@ -0,0 +1,1024 @@
+/*
+ * drivers/char/xhci-dbc.c
+ *
+ * Xen port for the xue debugger
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Copyright (c) 2019 Assured Information Security.
+ */
+
+#include <xen/delay.h>
+#include <xen/mm.h>
+#include <xen/param.h>
+#include <xen/serial.h>
+#include <xen/timer.h>
+#include <xen/types.h>
+#include <asm/fixmap.h>
+#include <asm/io.h>
+#include <asm/string.h>
+#include <asm/system.h>
+
+/* uncomment to have dbc_uart_dump() debug function */
+/* #define DBC_DEBUG 1 */
+
+#define DBC_POLL_INTERVAL 100 /* us */
+
+#define DBC_PAGE_SIZE 4096U
+
+/* Supported xHC PCI configurations */
+#define DBC_XHC_CLASSC 0xC0330U
+
+/* DbC idVendor and idProduct */
+#define DBC_DBC_VENDOR 0x1D6B
+#define DBC_DBC_PRODUCT 0x0010
+#define DBC_DBC_PROTOCOL 0x0000
+
+/* DCCTRL fields */
+#define DBC_CTRL_DCR 0
+#define DBC_CTRL_HOT 2
+#define DBC_CTRL_HIT 3
+#define DBC_CTRL_DRC 4
+#define DBC_CTRL_DCE 31
+
+/* DCPORTSC fields */
+#define DBC_PSC_PED 1
+#define DBC_PSC_CSC 17
+#define DBC_PSC_PRC 21
+#define DBC_PSC_PLC 22
+#define DBC_PSC_CEC 23
+
+#define DBC_PSC_ACK_MASK \
+ ((1UL << DBC_PSC_CSC) | (1UL << DBC_PSC_PRC) | (1UL << DBC_PSC_PLC) | \
+ (1UL << DBC_PSC_CEC))
+
+#define dbc_debug(...) printk("dbc debug: " __VA_ARGS__)
+#define dbc_alert(...) printk("dbc alert: " __VA_ARGS__)
+#define dbc_error(...) printk("dbc error: " __VA_ARGS__)
+
+/******************************************************************************
+ * TRB ring (summarized from the manual):
+ *
+ * TRB rings are circular queues of TRBs shared between the xHC and the driver.
+ * Each ring has one producer and one consumer. The DbC has one event
+ * ring and two transfer rings; one IN and one OUT.
+ *
+ * The DbC hardware is the producer on the event ring, and
+ * dbc driver is the consumer. This means that event TRBs are read-only from
+ * the dbc driver.
+ *
+ * OTOH, dbc drive is the producer of transfer TRBs on the two transfer
+ * rings, so dbc driver enqueues transfers, and the hardware dequeues
+ * them. The dequeue pointer of a transfer ring is read by
+ * dbc driver by examining the latest transfer event TRB on the event ring. The
+ * transfer event TRB contains the address of the transfer TRB that generated
+ * the event.
+ *
+ * To make each transfer ring circular, the last TRB must be a link TRB, which
+ * points to the beginning of the next queue. Note that this implementation
+ * does not support multiple segments, so each link TRB points back to the
+ * beginning of its own segment.
+ ******************************************************************************/
+
+/* TRB types */
+enum {
+ XHCI_TRB_NORM = 1,
+ XHCI_TRB_LINK = 6,
+ XHCI_TRB_TFRE = 32,
+ XHCI_TRB_PSCE = 34
+};
+
+/* TRB completion codes */
+enum {
+ XHCI_TRB_CC_SUCCESS = 1,
+ XHCI_TRB_CC_TRB_ERR = 5,
+};
+
+/* DbC endpoint types */
+enum {
+ XHCI_EP_BULK_OUT = 2,
+ XHCI_EP_BULK_IN = 6,
+};
+
+/* DMA/MMIO structures */
+struct xhci_trb {
+ uint64_t params;
+ uint32_t status;
+ uint32_t ctrl;
+};
+
+/* log2(sizeof(struct xhci_trb)) */
+#define XHCI_TRB_SHIFT 4
+
+struct xhci_erst_segment {
+ uint64_t base;
+ uint16_t size;
+ uint8_t rsvdz[6];
+};
+
+/* Arbitrary length, must fit every DBC_STRING_* */
+#define MAX_STRING_LENGTH 16
+
+#define DBC_STRINGS_COUNT 4
+#define DBC_STRING_LANGID "\x09\x04"
+#define DBC_STRING_MANUFACTURER "Xen"
+#define DBC_STRING_PRODUCT "Debug console"
+#define DBC_STRING_SERIAL "0"
+
+#define XHCI_DT_STRING 3
+
+struct xhci_string_descriptor {
+ uint8_t size;
+ uint8_t type;
+ uint16_t string[MAX_STRING_LENGTH];
+};
+
+#define DBC_CTX_SIZE 16
+#define DBC_CTX_BYTES (DBC_CTX_SIZE * 4)
+
+struct xhci_dbc_ctx {
+ union {
+ uint32_t info[DBC_CTX_SIZE];
+ struct {
+ uint64_t string0_ptr;
+ uint64_t manufacturer_ptr;
+ uint64_t product_ptr;
+ uint64_t serial_ptr;
+ uint8_t string0_size;
+ uint8_t manufacturer_size;
+ uint8_t product_size;
+ uint8_t serial_size;
+ };
+ };
+ uint32_t ep_out[DBC_CTX_SIZE];
+ uint32_t ep_in[DBC_CTX_SIZE];
+};
+
+struct dbc_reg {
+ uint32_t id;
+ uint32_t db;
+ uint32_t erstsz;
+ uint32_t rsvdz;
+ uint64_t erstba;
+ uint64_t erdp;
+ uint32_t ctrl;
+ uint32_t st;
+ uint32_t portsc;
+ uint32_t rsvdp;
+ uint64_t cp;
+ uint32_t ddi1;
+ uint32_t ddi2;
+};
+
+#define DBC_TRB_MAX_TFR (DBC_PAGE_SIZE << 4)
+#define DBC_TRB_PER_PAGE (DBC_PAGE_SIZE / sizeof(struct xhci_trb))
+
+/* Defines the size in bytes of TRB rings as 2^DBC_TRB_RING_ORDER * 4096 */
+#ifndef DBC_TRB_RING_ORDER
+#define DBC_TRB_RING_ORDER 4
+#endif
+#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE * (1 << DBC_TRB_RING_ORDER))
+#define DBC_TRB_RING_BYTES (DBC_TRB_RING_CAP * sizeof(struct xhci_trb))
+#define DBC_TRB_RING_MASK (DBC_TRB_RING_BYTES - 1U)
+
+struct xhci_trb_ring {
+ struct xhci_trb *trb; /* Array of TRBs */
+ uint32_t enq; /* The offset of the enqueue ptr */
+ uint32_t deq; /* The offset of the dequeue ptr */
+ uint8_t cyc; /* Cycle state toggled on each wrap-around */
+ uint8_t db; /* Doorbell target */
+ uint64_t dma; /* Physical address (for the device) */
+};
+
+#define DBC_DB_OUT 0x0
+#define DBC_DB_IN 0x1
+#define DBC_DB_INVAL 0xFF
+
+/* Defines the size in bytes of work rings as 2^DBC_WORK_RING_ORDER * 4096 */
+#ifndef DBC_WORK_RING_ORDER
+#define DBC_WORK_RING_ORDER 3
+#endif
+#define DBC_WORK_RING_CAP (DBC_PAGE_SIZE * (1 << DBC_WORK_RING_ORDER))
+#define DBC_WORK_RING_BYTES DBC_WORK_RING_CAP
+
+#if DBC_WORK_RING_CAP > DBC_TRB_MAX_TFR
+#error "DBC_WORK_RING_ORDER must be at most 4"
+#endif
+
+struct dbc_work_ring {
+ uint8_t *buf;
+ uint32_t enq;
+ uint32_t deq;
+ uint64_t dma;
+};
+
+struct dbc {
+ struct dbc_reg __iomem *dbc_reg;
+ struct xhci_dbc_ctx *dbc_ctx;
+ struct xhci_erst_segment *dbc_erst;
+ struct xhci_trb_ring dbc_ering;
+ struct xhci_trb_ring dbc_oring;
+ struct xhci_trb_ring dbc_iring;
+ struct dbc_work_ring dbc_owork;
+ struct xhci_string_descriptor *dbc_str;
+
+ pci_sbdf_t sbdf;
+ uint64_t xhc_mmio_phys;
+ uint64_t xhc_dbc_offset;
+ void __iomem *xhc_mmio;
+
+ bool open;
+};
+
+static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
+{
+ size_t i;
+
+ if ( size != MAX_XHCI_PAGES * DBC_PAGE_SIZE )
+ return NULL;
+
+ for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
+ {
+ set_fixmap_nocache(i, phys);
+ phys += DBC_PAGE_SIZE;
+ }
+
+ /*
+ * The fixmap grows downward, so the lowest virt is
+ * at the highest index
+ */
+ return fix_to_virt(FIX_XHCI_END);
+}
+
+static bool __init dbc_init_xhc(struct dbc *dbc)
+{
+ uint32_t bar0;
+ uint64_t bar1;
+ uint64_t bar_size;
+ uint64_t devfn;
+ uint16_t cmd;
+ size_t xhc_mmio_size;
+
+ /*
+ * Search PCI bus 0 for the xHC. All the host controllers supported so far
+ * are part of the chipset and are on bus 0.
+ */
+ for ( devfn = 0; devfn < 256; devfn++ )
+ {
+ pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
+ uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
+
+ if ( hdr == 0 || hdr == 0x80 )
+ {
+ if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == DBC_XHC_CLASSC )
+ {
+ dbc->sbdf = sbdf;
+ break;
+ }
+ }
+ }
+
+ if ( !dbc->sbdf.sbdf )
+ {
+ dbc_error("Compatible xHC not found on bus 0\n");
+ return false;
+ }
+
+ /* ...we found it, so parse the BAR and map the registers */
+ bar0 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
+ bar1 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1);
+
+ /* IO BARs not allowed; BAR must be 64-bit */
+ if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY ||
+ (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != PCI_BASE_ADDRESS_MEM_TYPE_64 )
+ return false;
+
+ cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
+
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, 0xFFFFFFFF);
+ bar_size = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
+ bar_size |= (uint64_t)pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1) << 32;
+ xhc_mmio_size = ~(bar_size & PCI_BASE_ADDRESS_MEM_MASK) + 1;
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, bar0);
+ pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, bar1);
+
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
+
+ dbc->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
+ dbc->xhc_mmio = dbc_sys_map_xhc(dbc->xhc_mmio_phys, xhc_mmio_size);
+
+ if ( dbc->xhc_mmio == NULL )
+ return false;
+
+ if ( (cmd & PCI_COMMAND_MEMORY) == 0 )
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd | PCI_COMMAND_MEMORY);
+
+ return true;
+}
+
+/**
+ * The first register of the debug capability is found by traversing the
+ * host controller's capability list (xcap) until a capability
+ * with ID = 0xA is found. The xHCI capability list begins at address
+ * mmio + (HCCPARAMS1[31:16] << 2).
+ */
+static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
+{
+ uint32_t *xcap;
+ uint32_t xcap_val;
+ uint32_t next;
+ uint32_t id = 0;
+ uint8_t *mmio = (uint8_t *)dbc->xhc_mmio;
+ uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
+ const uint32_t DBC_ID = 0xA;
+ int ttl = 48;
+
+ xcap = (uint32_t *)dbc->xhc_mmio;
+ /*
+ * This is initially an offset to the first capability. All the offsets
+ * (both in HCCP1 and then next capability pointer are dword-based.
+ */
+ next = (readl(hccp1) & 0xFFFF0000) >> 16;
+
+ while ( id != DBC_ID && next && ttl-- )
+ {
+ xcap += next;
+ xcap_val = readl(xcap);
+ id = xcap_val & 0xFF;
+ next = (xcap_val & 0xFF00) >> 8;
+ }
+
+ if ( id != DBC_ID )
+ return NULL;
+
+ dbc->xhc_dbc_offset = (uint64_t)xcap - (uint64_t)mmio;
+ return (struct dbc_reg __iomem *)xcap;
+}
+
+/**
+ * Fields with the same interpretation for every TRB type (section 4.11.1).
+ * These are the fields defined in the TRB template, minus the ENT bit. That
+ * bit is the toggle cycle bit in link TRBs, so it shouldn't be in the
+ * template.
+ */
+static uint32_t xhci_trb_cyc(const struct xhci_trb *trb)
+{
+ return trb->ctrl & 0x1;
+}
+
+static uint32_t xhci_trb_type(const struct xhci_trb *trb)
+{
+ return (trb->ctrl & 0xFC00) >> 10;
+}
+
+static void xhci_trb_set_cyc(struct xhci_trb *trb, uint32_t c)
+{
+ trb->ctrl &= ~0x1U;
+ trb->ctrl |= c;
+}
+
+static void xhci_trb_set_type(struct xhci_trb *trb, uint32_t t)
+{
+ trb->ctrl &= ~0xFC00U;
+ trb->ctrl |= (t << 10);
+}
+
+/* Fields for normal TRBs */
+static void xhci_trb_norm_set_buf(struct xhci_trb *trb, uint64_t addr)
+{
+ trb->params = addr;
+}
+
+static void xhci_trb_norm_set_len(struct xhci_trb *trb, uint32_t len)
+{
+ trb->status &= ~0x1FFFFU;
+ trb->status |= len;
+}
+
+static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
+{
+ trb->ctrl |= 0x20;
+}
+
+/**
+ * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
+ * TRBs are read-only from software
+ */
+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;
+}
+
+/* Fields for link TRBs (section 6.4.4.1) */
+static void xhci_trb_link_set_rsp(struct xhci_trb *trb, uint64_t rsp)
+{
+ trb->params = rsp;
+}
+
+static void xhci_trb_link_set_tc(struct xhci_trb *trb)
+{
+ trb->ctrl |= 0x2;
+}
+
+static void xhci_trb_ring_init(const struct dbc *dbc,
+ struct xhci_trb_ring *ring, int producer,
+ int doorbell)
+{
+ memset(ring->trb, 0, DBC_TRB_RING_CAP * sizeof(ring->trb[0]));
+
+ ring->enq = 0;
+ ring->deq = 0;
+ ring->cyc = 1;
+ ring->db = (uint8_t)doorbell;
+ ring->dma = virt_to_maddr(ring->trb);
+
+ /*
+ * Producer implies transfer ring, so we have to place a
+ * link TRB at the end that points back to trb[0]
+ */
+ if ( producer )
+ {
+ struct xhci_trb *trb = &ring->trb[DBC_TRB_RING_CAP - 1];
+ xhci_trb_set_type(trb, XHCI_TRB_LINK);
+ xhci_trb_link_set_tc(trb);
+ xhci_trb_link_set_rsp(trb, virt_to_maddr(ring->trb));
+ }
+}
+
+static int xhci_trb_ring_full(const struct xhci_trb_ring *ring)
+{
+ return ((ring->enq + 1) & (DBC_TRB_RING_CAP - 1)) == ring->deq;
+}
+
+static int dbc_work_ring_full(const struct dbc_work_ring *ring)
+{
+ return ((ring->enq + 1) & (DBC_WORK_RING_CAP - 1)) == ring->deq;
+}
+
+static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring)
+{
+ if ( ring->enq >= ring->deq )
+ return ring->enq - ring->deq;
+
+ return DBC_WORK_RING_CAP - ring->deq + ring->enq;
+}
+
+static void dbc_push_trb(struct dbc *dbc, struct xhci_trb_ring *ring,
+ uint64_t dma, uint64_t len)
+{
+ struct xhci_trb trb;
+
+ if ( ring->enq == DBC_TRB_RING_CAP - 1 )
+ {
+ /*
+ * We have to make sure the xHC processes the link TRB in order
+ * for wrap-around to work properly. We do this by marking the
+ * xHC as owner of the link TRB by setting the TRB's cycle bit
+ * (just like with normal TRBs).
+ */
+ struct xhci_trb *link = &ring->trb[ring->enq];
+ xhci_trb_set_cyc(link, ring->cyc);
+
+ ring->enq = 0;
+ ring->cyc ^= 1;
+ }
+
+ trb.params = 0;
+ trb.status = 0;
+ trb.ctrl = 0;
+
+ xhci_trb_set_type(&trb, XHCI_TRB_NORM);
+ xhci_trb_set_cyc(&trb, ring->cyc);
+
+ xhci_trb_norm_set_buf(&trb, dma);
+ xhci_trb_norm_set_len(&trb, (uint32_t)len);
+ xhci_trb_norm_set_ioc(&trb);
+
+ ring->trb[ring->enq++] = trb;
+ cache_flush(&ring->trb[ring->enq - 1], sizeof(trb));
+}
+
+static int64_t dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring,
+ const char *buf, unsigned int len)
+{
+ unsigned int i = 0;
+ unsigned int end, start = ring->enq;
+
+ while ( !dbc_work_ring_full(ring) && i < len )
+ {
+ ring->buf[ring->enq] = buf[i++];
+ ring->enq = (ring->enq + 1) & (DBC_WORK_RING_CAP - 1);
+ }
+
+ end = ring->enq;
+
+ if ( end > start )
+ cache_flush(&ring->buf[start], end - start);
+ else if ( i > 0 )
+ {
+ cache_flush(&ring->buf[start], DBC_WORK_RING_CAP - start);
+ cache_flush(&ring->buf[0], end);
+ }
+
+ return i;
+}
+
+/*
+ * 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;
+ struct xhci_trb_ring *er = &dbc->dbc_ering;
+ struct xhci_trb_ring *tr = &dbc->dbc_oring;
+ struct xhci_trb *event = &er->trb[er->deq];
+ uint64_t erdp = readq(®->erdp);
+ uint32_t portsc;
+ uint64_t event_ptr;
+ unsigned int trb_idx;
+
+ BUILD_BUG_ON((1 << XHCI_TRB_SHIFT) != sizeof(struct xhci_trb));
+
+ rmb();
+
+ while ( xhci_trb_cyc(event) == er->cyc )
+ {
+ switch (xhci_trb_type(event))
+ {
+ case XHCI_TRB_TFRE:
+ event_ptr = xhci_trb_tfre_ptr(event);
+ /*
+ * trb_idx is just completed TRB, so set the dequeue ptr one
+ * position further.
+ */
+ if ( event_ptr - tr->dma < DBC_TRB_RING_BYTES )
+ {
+ trb_idx = (event_ptr - tr->dma) >> XHCI_TRB_SHIFT;
+ tr->deq = (trb_idx + 1) & (DBC_TRB_RING_CAP - 1);
+ }
+ else
+ dbc_alert("event: TRB 0x%lx not found in any ring\n",
+ event_ptr);
+ break;
+ case XHCI_TRB_PSCE:
+ portsc = readl(®->portsc);
+ portsc |= DBC_PSC_ACK_MASK & portsc;
+ writel(portsc, ®->portsc);
+ break;
+ default:
+ break;
+ }
+
+ er->cyc = (er->deq == DBC_TRB_RING_CAP - 1) ? er->cyc ^ 1 : er->cyc;
+ er->deq = (er->deq + 1) & (DBC_TRB_RING_CAP - 1);
+ event = &er->trb[er->deq];
+ }
+
+ erdp = er->dma + (er->deq << XHCI_TRB_SHIFT);
+ wmb();
+ writeq(erdp, ®->erdp);
+}
+
+/**
+ * dbc_init_ep
+ *
+ * Initializes the endpoint as specified in sections 7.6.3.2 and 7.6.9.2.
+ * Each endpoint is Bulk, so the MaxPStreams, LSA, HID, CErr, FE,
+ * Interval, Mult, and Max ESIT Payload fields are all 0.
+ *
+ * Max packet size: 1024
+ * Max burst size: debug mbs (from dbc_reg->ctrl register)
+ * EP type: 2 for OUT bulk, 6 for IN bulk
+ * TR dequeue ptr: physical base address of transfer ring
+ * Avg TRB length: software defined (see 4.14.1.1 for suggested defaults)
+ */
+static void dbc_init_ep(uint32_t *ep, uint64_t mbs, uint32_t type,
+ uint64_t ring_dma)
+{
+ memset(ep, 0, DBC_CTX_BYTES);
+
+ ep[1] = (1024 << 16) | ((uint32_t)mbs << 8) | (type << 3);
+ ep[2] = (ring_dma & 0xFFFFFFFF) | 1;
+ ep[3] = ring_dma >> 32;
+ ep[4] = 3 * 1024;
+}
+
+static void dbc_init_string_single(struct xhci_string_descriptor *string,
+ char *ascii_str,
+ uint64_t *str_ptr,
+ uint8_t *str_size_ptr)
+{
+ size_t i, len = strlen(ascii_str);
+
+ string->size = offsetof(typeof(*string), string) + len * 2;
+ string->type = XHCI_DT_STRING;
+ /* ASCII to UTF16 conversion */
+ for (i = 0; i < len; i++)
+ string->string[i] = ascii_str[i];
+ *str_ptr = virt_to_maddr(string);
+ *str_size_ptr = string->size;
+}
+
+/* Initialize the DbC info with USB string descriptor addresses */
+static void dbc_init_strings(struct dbc *dbc, uint32_t *info)
+{
+ BUILD_BUG_ON(sizeof(DBC_STRING_LANGID) > MAX_STRING_LENGTH);
+ BUILD_BUG_ON(sizeof(DBC_STRING_MANUFACTURER) > MAX_STRING_LENGTH);
+ BUILD_BUG_ON(sizeof(DBC_STRING_PRODUCT) > MAX_STRING_LENGTH);
+ BUILD_BUG_ON(sizeof(DBC_STRING_SERIAL) > MAX_STRING_LENGTH);
+
+ dbc_init_string_single(&dbc->dbc_str[0], DBC_STRING_LANGID,
+ &dbc->dbc_ctx->string0_ptr,
+ &dbc->dbc_ctx->string0_size);
+ dbc_init_string_single(&dbc->dbc_str[1], DBC_STRING_MANUFACTURER,
+ &dbc->dbc_ctx->manufacturer_ptr,
+ &dbc->dbc_ctx->manufacturer_size);
+ dbc_init_string_single(&dbc->dbc_str[2], DBC_STRING_PRODUCT,
+ &dbc->dbc_ctx->product_ptr,
+ &dbc->dbc_ctx->product_size);
+ dbc_init_string_single(&dbc->dbc_str[3], DBC_STRING_SERIAL,
+ &dbc->dbc_ctx->serial_ptr,
+ &dbc->dbc_ctx->serial_size);
+}
+
+static void dbc_enable_dbc(struct dbc *dbc)
+{
+ struct dbc_reg *reg = dbc->dbc_reg;
+
+ wmb();
+ writel(readl(®->ctrl) | (1U << DBC_CTRL_DCE), ®->ctrl);
+ wmb();
+
+ while ( (readl(®->ctrl) & (1U << DBC_CTRL_DCE)) == 0 )
+ cpu_relax();
+
+ wmb();
+ writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc);
+ wmb();
+
+ while ( (readl(®->ctrl) & (1U << DBC_CTRL_DCR)) == 0 )
+ cpu_relax();
+}
+
+static void dbc_disable_dbc(struct dbc *dbc)
+{
+ struct dbc_reg *reg = dbc->dbc_reg;
+
+ writel(readl(®->portsc) & ~(1U << DBC_PSC_PED), ®->portsc);
+ wmb();
+ writel(readl(®->ctrl) & ~(1U << DBC_CTRL_DCE), ®->ctrl);
+
+ while ( readl(®->ctrl) & (1U << DBC_CTRL_DCE) )
+ cpu_relax();
+}
+
+static int dbc_init_dbc(struct dbc *dbc)
+{
+ uint64_t erdp = 0;
+ uint64_t mbs = 0;
+ uint16_t cmd;
+ struct dbc_reg *reg = xhci_find_dbc(dbc);
+
+ if ( !reg )
+ return 0;
+
+ dbc->dbc_reg = reg;
+ dbc_disable_dbc(dbc);
+
+ xhci_trb_ring_init(dbc, &dbc->dbc_ering, 0, DBC_DB_INVAL);
+ xhci_trb_ring_init(dbc, &dbc->dbc_oring, 1, DBC_DB_OUT);
+ xhci_trb_ring_init(dbc, &dbc->dbc_iring, 1, DBC_DB_IN);
+
+ erdp = virt_to_maddr(dbc->dbc_ering.trb);
+ if ( !erdp )
+ return 0;
+
+ memset(dbc->dbc_erst, 0, sizeof(*dbc->dbc_erst));
+ dbc->dbc_erst->base = erdp;
+ dbc->dbc_erst->size = DBC_TRB_RING_CAP;
+
+ mbs = (readl(®->ctrl) & 0xFF0000) >> 16;
+
+ memset(dbc->dbc_ctx, 0, sizeof(*dbc->dbc_ctx));
+ dbc_init_strings(dbc, dbc->dbc_ctx->info);
+ dbc_init_ep(dbc->dbc_ctx->ep_out, mbs, XHCI_EP_BULK_OUT,
+ dbc->dbc_oring.dma);
+ dbc_init_ep(dbc->dbc_ctx->ep_in, mbs, XHCI_EP_BULK_IN,
+ dbc->dbc_iring.dma);
+
+ writel(1, ®->erstsz);
+ writeq(virt_to_maddr(dbc->dbc_erst), ®->erstba);
+ writeq(erdp, ®->erdp);
+ writeq(virt_to_maddr(dbc->dbc_ctx), ®->cp);
+ writel((DBC_DBC_VENDOR << 16) | DBC_DBC_PROTOCOL, ®->ddi1);
+ writel(DBC_DBC_PRODUCT, ®->ddi2);
+
+ cache_flush(dbc->dbc_ctx, sizeof(*dbc->dbc_ctx));
+ cache_flush(dbc->dbc_erst, sizeof(*dbc->dbc_erst));
+ cache_flush(dbc->dbc_ering.trb, DBC_TRB_RING_BYTES);
+ cache_flush(dbc->dbc_oring.trb, DBC_TRB_RING_BYTES);
+ cache_flush(dbc->dbc_iring.trb, DBC_TRB_RING_BYTES);
+ cache_flush(dbc->dbc_owork.buf, DBC_WORK_RING_BYTES);
+
+ cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
+ pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd | PCI_COMMAND_MASTER);
+
+ return 1;
+}
+
+static void dbc_init_work_ring(struct dbc *dbc,
+ struct dbc_work_ring *wrk)
+{
+ wrk->enq = 0;
+ wrk->deq = 0;
+ wrk->dma = virt_to_maddr(wrk->buf);
+}
+
+/**
+ * Initialize the DbC and enable it for transfers. First map in the DbC
+ * registers from the host controller's MMIO region. Then allocate and map
+ * DMA for the event and transfer rings. Finally, enable the DbC for
+ * the host to enumerate. On success, the DbC is ready to send packets.
+ *
+ * @param dbc the dbc to open (!= NULL)
+ * @return true iff dbc_open succeeded
+ */
+static bool __init dbc_open(struct dbc *dbc)
+{
+ if ( !dbc )
+ return false;
+
+ if ( !dbc_init_xhc(dbc) )
+ return false;
+
+ if ( !dbc_init_dbc(dbc) )
+ return false;
+
+ dbc_init_work_ring(dbc, &dbc->dbc_owork);
+ dbc_enable_dbc(dbc);
+ dbc->open = true;
+
+ return true;
+}
+
+/*
+ * Ensure DbC is still running, handle events, and possibly re-enable if cable
+ * was re-plugged. Returns true if DbC is operational.
+ */
+static bool dbc_ensure_running(struct dbc *dbc)
+{
+ struct dbc_reg *reg = dbc->dbc_reg;
+ uint32_t ctrl;
+ uint32_t cmd;
+
+ dbc_pop_events(dbc);
+
+ ctrl = readl(®->ctrl);
+ if ( !(ctrl & (1U << DBC_CTRL_DCR)) )
+ {
+ return false;
+ }
+
+ if ( ctrl & (1U << DBC_CTRL_DRC) )
+ {
+ writel(ctrl | (1U << DBC_CTRL_DRC), ®->ctrl);
+ writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc);
+ wmb();
+ }
+
+ return true;
+}
+
+/**
+ * Commit the pending transfer TRBs to the DbC. This notifies
+ * the DbC of any previously-queued data on the work ring and
+ * rings the doorbell.
+ *
+ * @param dbc the dbc to flush
+ * @param trb the ring containing the TRBs to transfer
+ * @param wrk the work ring containing data to be flushed
+ */
+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(®->db) & 0xFFFF00FF) | (trb->db << 8);
+
+ if ( xhci_trb_ring_full(trb) )
+ return;
+
+ if ( wrk->enq == wrk->deq )
+ return;
+ else if ( wrk->enq > wrk->deq )
+ {
+ dbc_push_trb(dbc, trb, wrk->dma + wrk->deq, wrk->enq - wrk->deq);
+ wrk->deq = wrk->enq;
+ }
+ else
+ {
+ dbc_push_trb(dbc, trb, wrk->dma + wrk->deq,
+ DBC_WORK_RING_CAP - wrk->deq);
+ wrk->deq = 0;
+ if ( wrk->enq > 0 && !xhci_trb_ring_full(trb) )
+ {
+ dbc_push_trb(dbc, trb, wrk->dma, wrk->enq);
+ wrk->deq = wrk->enq;
+ }
+ }
+
+ wmb();
+ writel(db, ®->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.
+ *
+ * @param dbc the dbc to write to
+ * @param c the character to write
+ * @return the number of bytes written
+ */
+static int64_t dbc_putc(struct dbc *dbc, char c)
+{
+ if ( !dbc_push_work(dbc, &dbc->dbc_owork, &c, 1) )
+ return 0;
+
+ if ( !dbc_ensure_running(dbc) )
+ return 1;
+
+ if ( c == '\n' )
+ dbc_flush(dbc, &dbc->dbc_oring, &dbc->dbc_owork);
+
+ return 1;
+}
+
+struct dbc_uart {
+ struct dbc dbc;
+ struct timer timer;
+ spinlock_t *lock;
+};
+
+static struct dbc_uart dbc_uart;
+
+static void cf_check dbc_uart_poll(void *data)
+{
+ struct serial_port *port = data;
+ struct dbc_uart *uart = port->uart;
+ struct dbc *dbc = &uart->dbc;
+ unsigned long flags = 0;
+
+ 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);
+ }
+
+ serial_tx_interrupt(port, guest_cpu_user_regs());
+ set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL));
+}
+
+static void __init cf_check dbc_uart_init_preirq(struct serial_port *port)
+{
+ struct dbc_uart *uart = port->uart;
+ uart->lock = &port->tx_lock;
+}
+
+static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
+{
+ struct dbc_uart *uart = port->uart;
+
+ serial_async_transmit(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);
+}
+
+static int cf_check dbc_uart_tx_ready(struct serial_port *port)
+{
+ struct dbc_uart *uart = port->uart;
+ struct dbc *dbc = &uart->dbc;
+
+ return DBC_WORK_RING_CAP - dbc_work_ring_size(&dbc->dbc_owork);
+}
+
+static void cf_check dbc_uart_putc(struct serial_port *port, char c)
+{
+ struct dbc_uart *uart = port->uart;
+ dbc_putc(&uart->dbc, c);
+}
+
+static void cf_check dbc_uart_flush(struct serial_port *port)
+{
+ s_time_t goal;
+ struct dbc_uart *uart = port->uart;
+ struct dbc *dbc = &uart->dbc;
+
+ if ( dbc_ensure_running(dbc) )
+ dbc_flush(dbc, &dbc->dbc_oring, &dbc->dbc_owork);
+
+ goal = NOW() + MICROSECS(DBC_POLL_INTERVAL);
+ if ( uart->timer.expires > goal )
+ set_timer(&uart->timer, goal);
+}
+
+static struct uart_driver dbc_uart_driver = {
+ .init_preirq = dbc_uart_init_preirq,
+ .init_postirq = dbc_uart_init_postirq,
+ .tx_ready = dbc_uart_tx_ready,
+ .putc = dbc_uart_putc,
+ .flush = dbc_uart_flush,
+};
+
+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(64);
+static struct xhci_dbc_ctx ctx __aligned(64);
+static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
+static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
+static char __initdata opt_dbgp[30];
+
+string_param("dbgp", opt_dbgp);
+
+void __init xhci_dbc_uart_init(void)
+{
+ struct dbc_uart *uart = &dbc_uart;
+ struct dbc *dbc = &uart->dbc;
+
+ if ( strncmp(opt_dbgp, "xhci", 4) )
+ return;
+
+ memset(dbc, 0, sizeof(*dbc));
+
+ 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;
+
+ if ( dbc_open(dbc) )
+ serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
+}
+
+#ifdef DBC_DEBUG
+static void dbc_dump(struct dbc *dbc)
+{
+ struct dbc_reg *r = dbc->dbc_reg;
+
+ dbc_debug("XHCI DBC DUMP:\n");
+ dbc_debug(" ctrl: 0x%x stat: 0x%x psc: 0x%x\n",
+ readl(&r->ctrl), readl(&r->st), readl(&r->portsc));
+ dbc_debug(" id: 0x%x, db: 0x%x\n",
+ readl(&r->id), readl(&r->db));
+ dbc_debug(" erstsz: %u, erstba: 0x%lx\n",
+ readl(&r->erstsz), readq(&r->erstba));
+ dbc_debug(" erdp: 0x%lx, cp: 0x%lx\n",
+ readq(&r->erdp), readq(&r->cp));
+ dbc_debug(" ddi1: 0x%x, ddi2: 0x%x\n",
+ readl(&r->ddi1), readl(&r->ddi2));
+ dbc_debug(" erstba == virt_to_dma(erst): %d\n",
+ readq(&r->erstba) == virt_to_maddr(dbc->dbc_erst));
+ dbc_debug(" erdp == virt_to_dma(erst[0].base): %d\n",
+ readq(&r->erdp) == dbc->dbc_erst[0].base);
+ dbc_debug(" cp == virt_to_dma(ctx): %d\n",
+ readq(&r->cp) == virt_to_maddr(dbc->dbc_ctx));
+}
+
+static void dbc_uart_dump(void)
+{
+ struct dbc_uart *uart = &dbc_uart;
+ struct dbc *dbc = &uart->dbc;
+
+ dbc_dump(dbc);
+}
+#endif
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 6548f0b0a9cf..181e026967bc 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -171,6 +171,11 @@ struct ns16550_defaults {
};
void ns16550_init(int index, struct ns16550_defaults *defaults);
void ehci_dbgp_init(void);
+#ifdef CONFIG_XHCI
+void xhci_dbc_uart_init(void);
+#else
+static void inline xhci_dbc_uart_init(void) {};
+#endif
void arm_uart_init(void);
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 02/10] drivers/char: reset XHCI ports when initializing dbc
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
2022-07-26 3:23 ` Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-04 13:14 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 03/10] drivers/char: add support for selecting specific xhci Marek Marczykowski-Górecki
` (8 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
Reset ports, to force host system to re-enumerate devices. Otheriwse it
will require the cable to be re-plugged, or will wait in the
"configuring" state indefinitely.
Trick and code copied from Linux:
drivers/usb/early/xhci-dbc.c:xdbc_start()->xdbc_reset_debug_port()
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
Changes in v3:
- adjust for renamed driver
- use readl() etc for MMIO
- simplify xcap lookup
- drop acked-by
Changes in v2:
- use uint32_t instead of u32
- code style
---
xen/drivers/char/xhci-dbc.c | 75 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 75 insertions(+)
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index f0e60d1b86aa..53c39eedd4a6 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -63,6 +63,10 @@
((1UL << DBC_PSC_CSC) | (1UL << DBC_PSC_PRC) | (1UL << DBC_PSC_PLC) | \
(1UL << DBC_PSC_CEC))
+#define XHC_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff)
+#define PORT_RESET (1 << 4)
+#define PORT_CONNECT (1 << 0)
+
#define dbc_debug(...) printk("dbc debug: " __VA_ARGS__)
#define dbc_alert(...) printk("dbc alert: " __VA_ARGS__)
#define dbc_error(...) printk("dbc error: " __VA_ARGS__)
@@ -660,6 +664,73 @@ static void dbc_init_strings(struct dbc *dbc, uint32_t *info)
&dbc->dbc_ctx->serial_size);
}
+static void dbc_do_reset_debug_port(struct dbc *dbc,
+ unsigned int id, unsigned int count)
+{
+ uint32_t __iomem *ops_reg;
+ uint32_t __iomem *portsc;
+ uint32_t val, cap_length;
+ unsigned int i;
+
+ cap_length = readl(dbc->xhc_mmio) & 0xff;
+ ops_reg = dbc->xhc_mmio + cap_length;
+
+ id--;
+ for ( i = id; i < (id + count); i++ )
+ {
+ portsc = ops_reg + 0x100 + i * 0x4;
+ val = readl(portsc);
+ if ( !(val & PORT_CONNECT) )
+ writel(val | PORT_RESET, portsc);
+ }
+}
+
+static void dbc_reset_debug_port(struct dbc *dbc)
+{
+ uint32_t val, port_offset, port_count;
+ uint32_t __iomem *xcap;
+ uint32_t xcap_val;
+ uint32_t next;
+ uint32_t id;
+ uint8_t __iomem *mmio = (uint8_t *)dbc->xhc_mmio;
+ uint32_t __iomem *hccp1 = (uint32_t *)(mmio + 0x10);
+ const uint32_t PROTOCOL_ID = 0x2;
+ int ttl = 48;
+
+ xcap = (uint32_t *)dbc->xhc_mmio;
+ /*
+ * This is initially an offset to the first capability. All the offsets
+ * (both in HCCP1 and then next capability pointer are dword-based.
+ */
+ next = (readl(hccp1) & 0xFFFF0000) >> 16;
+
+ /*
+ * Look for "supported protocol" capability, major revision 3.
+ * There may be multiple of them.
+ */
+ while ( next && ttl-- )
+ {
+ xcap += next;
+ xcap_val = readl(xcap);
+ id = xcap_val & 0xFF;
+ next = (xcap_val & 0xFF00) >> 8;
+
+ if ( id != PROTOCOL_ID )
+ continue;
+
+ if ( XHC_EXT_PORT_MAJOR(xcap_val) != 0x3 )
+ continue;
+
+ /* extract ports offset and count from the capability structure */
+ val = readl(xcap + 2);
+ port_offset = val & 0xff;
+ port_count = (val >> 8) & 0xff;
+
+ /* and reset them all */
+ dbc_do_reset_debug_port(dbc, port_offset, port_count);
+ }
+}
+
static void dbc_enable_dbc(struct dbc *dbc)
{
struct dbc_reg *reg = dbc->dbc_reg;
@@ -671,6 +742,10 @@ static void dbc_enable_dbc(struct dbc *dbc)
while ( (readl(®->ctrl) & (1U << DBC_CTRL_DCE)) == 0 )
cpu_relax();
+ /* reset ports on initial open, to force re-enumerating by the host */
+ if ( !dbc->open )
+ dbc_reset_debug_port(dbc);
+
wmb();
writel(readl(®->portsc) | (1U << DBC_PSC_PED), ®->portsc);
wmb();
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 03/10] drivers/char: add support for selecting specific xhci
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 02/10] drivers/char: reset XHCI ports when initializing dbc Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 04/10] console: support multiple serial console simultaneously Marek Marczykowski-Górecki
` (7 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Jan Beulich, Andrew Cooper,
George Dunlap, Julien Grall, Stefano Stabellini, Wei Liu
Handle parameters similar to dbgp=ehci.
Implement this by not resettting dbc->sbdf again in dbc_init_xhc(), but
using a value found there if non-zero. Additionally, add xue->xhc_num to
select n-th controller.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v3:
- adjust for xhci-dbc rename
- drop redundant check in parsing dbgp= option
Changes in v2:
- unsigned int xhc_num
- code style
---
docs/misc/xen-command-line.pandoc | 2 +-
xen/drivers/char/xhci-dbc.c | 54 ++++++++++++++++++++++++--------
2 files changed, 43 insertions(+), 13 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index f936283cd187..d594bd5c7436 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -721,7 +721,7 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
-> `= xhci`
+> `= 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).
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 53c39eedd4a6..11026d3b71f0 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -243,6 +243,7 @@ struct dbc {
void __iomem *xhc_mmio;
bool open;
+ unsigned int xhc_num; /* look for n-th xhc */
};
static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
@@ -274,24 +275,37 @@ static bool __init dbc_init_xhc(struct dbc *dbc)
uint16_t cmd;
size_t xhc_mmio_size;
- /*
- * Search PCI bus 0 for the xHC. All the host controllers supported so far
- * are part of the chipset and are on bus 0.
- */
- for ( devfn = 0; devfn < 256; devfn++ )
+ if ( dbc->sbdf.sbdf == 0 )
{
- pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
- uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
-
- if ( hdr == 0 || hdr == 0x80 )
+ /*
+ * Search PCI bus 0 for the xHC. All the host controllers supported so
+ * far are part of the chipset and are on bus 0.
+ */
+ for ( devfn = 0; devfn < 256; devfn++ )
{
- if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == DBC_XHC_CLASSC )
+ pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
+ uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
+
+ if ( hdr == 0 || hdr == 0x80 )
{
- dbc->sbdf = sbdf;
- break;
+ if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) ==
+ DBC_XHC_CLASSC )
+ {
+ if ( dbc->xhc_num-- )
+ continue;
+ dbc->sbdf = sbdf;
+ break;
+ }
}
}
}
+ else
+ {
+ /* Verify if selected device is really xHC */
+ if ( (pci_conf_read32(dbc->sbdf, PCI_CLASS_REVISION) >> 8) !=
+ DBC_XHC_CLASSC )
+ dbc->sbdf.sbdf = 0;
+ }
if ( !dbc->sbdf.sbdf )
{
@@ -1047,12 +1061,28 @@ void __init xhci_dbc_uart_init(void)
{
struct dbc_uart *uart = &dbc_uart;
struct dbc *dbc = &uart->dbc;
+ const char *e;
if ( strncmp(opt_dbgp, "xhci", 4) )
return;
memset(dbc, 0, sizeof(*dbc));
+ if ( isdigit(opt_dbgp[4]) )
+ {
+ dbc->xhc_num = simple_strtoul(opt_dbgp + 4, &e, 10);
+ }
+ else if ( strncmp(opt_dbgp + 4, "@pci", 4) == 0 )
+ {
+ unsigned int bus, slot, func;
+
+ e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
+ if ( !e || *e )
+ return;
+
+ dbc->sbdf = PCI_SBDF(0, bus, slot, func);
+ }
+
dbc->dbc_ctx = &ctx;
dbc->dbc_erst = &erst;
dbc->dbc_ering.trb = evt_trb;
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 04/10] console: support multiple serial console simultaneously
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (2 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 03/10] drivers/char: add support for selecting specific xhci Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-04 14:13 ` Jan Beulich
2022-08-05 7:41 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 05/10] IOMMU: add common API for device reserved memory Marek Marczykowski-Górecki
` (6 subsequent siblings)
10 siblings, 2 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
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 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 | 97 ++++++++++++++++++++++++--------
xen/drivers/char/xhci-dbc.c | 6 +-
xen/include/xen/serial.h | 1 +-
5 files changed, 95 insertions(+), 24 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index d594bd5c7436..e53efdb324b3 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -433,6 +433,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`
@@ -2372,6 +2375,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..1010436d245c 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 f9937c5134c0..2ffc919445c6 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 int __read_mostly nr_sercon_handle = 0;
#ifdef CONFIG_X86
/* Tristate: 0 disabled, 1 user enabled, -1 default enabled */
@@ -393,32 +395,59 @@ 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[MAX_SERCONS+1])(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;
+ 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);
+ 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);
@@ -956,7 +985,7 @@ void guest_printk(const struct domain *d, const char *fmt, ...)
void __init console_init_preirq(void)
{
char *p;
- int sh;
+ int sh, i;
serial_init_preirq();
@@ -977,8 +1006,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 +1029,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 +1048,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 +1156,19 @@ int __init console_has(const char *device)
void console_start_log_everything(void)
{
- serial_start_log_everything(sercon_handle);
+ 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);
+ int i;
+
+ for ( i = 0; i < nr_sercon_handle; i++ )
+ serial_end_log_everything(sercon_handle[i]);
atomic_dec(&print_everything);
}
@@ -1149,23 +1190,32 @@ void console_unlock_recursive_irqrestore(unsigned long flags)
void console_force_unlock(void)
{
+ 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)
{
+ 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);
+ int i;
+
+ for ( i = 0; i < nr_sercon_handle; i++ )
+ serial_end_sync(sercon_handle[i]);
atomic_dec(&print_everything);
}
@@ -1291,7 +1341,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/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 11026d3b71f0..14a2d3eb0ee2 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1078,8 +1078,12 @@ void __init xhci_dbc_uart_init(void)
e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
if ( !e || *e )
+ {
+ printk(XENLOG_ERR
+ "Invalid dbgp= PCI device spec: '%s'\n",
+ opt_dbgp);
return;
-
+ }
dbc->sbdf = PCI_SBDF(0, bus, slot, func);
}
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 181e026967bc..c3bca000e238 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -99,6 +99,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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 05/10] IOMMU: add common API for device reserved memory
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (3 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 04/10] console: support multiple serial console simultaneously Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-04 14:25 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 06/10] IOMMU/VT-d: wire common device reserved memory API Marek Marczykowski-Górecki
` (5 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Jan Beulich, Paul Durrant,
Roger Pau Monné
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 v3:
- adjust code style
---
xen/drivers/passthrough/iommu.c | 45 ++++++++++++++++++++++++++++++++++-
xen/include/xen/iommu.h | 13 ++++++++++-
2 files changed, 58 insertions(+)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 77f64e61748d..74efd865ab69 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -651,6 +651,51 @@ 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;
+ uint32_t sbdf;
+};
+static unsigned int __initdata nr_extra_reserved_ranges;
+static struct extra_reserved_range __initdata
+ extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
+
+int iommu_add_extra_reserved_device_memory(unsigned long start,
+ unsigned long nr,
+ uint32_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 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,
+ 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 79529adf1fa5..aa87c3fd9ebc 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -297,6 +297,19 @@ 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,
+ uint32_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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 06/10] IOMMU/VT-d: wire common device reserved memory API
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (4 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 05/10] IOMMU: add common API for device reserved memory Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 07/10] IOMMU/AMD: " Marek Marczykowski-Górecki
` (4 subsequent siblings)
10 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel; +Cc: Marek Marczykowski-Górecki, Kevin Tian
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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 07/10] IOMMU/AMD: wire common device reserved memory API
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (5 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 06/10] IOMMU/VT-d: wire common device reserved memory API Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-04 14:53 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI Marek Marczykowski-Górecki
` (3 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel; +Cc: Marek Marczykowski-Górecki, Jan Beulich, Andrew Cooper
Register common device reserved memory similar to how ivmd= parameter is
handled.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (6 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 07/10] IOMMU/AMD: " Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-05 7:05 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC Marek Marczykowski-Górecki
` (2 subsequent siblings)
10 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
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 v3:
- adjust for xhci-dbc rename
- do not raise MAX_USER_RMRR_PAGES
- adjust alignment of DMA buffers
---
xen/drivers/char/xhci-dbc.c | 42 +++++++++++++++++++++++++-------------
1 file changed, 28 insertions(+), 14 deletions(-)
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 14a2d3eb0ee2..546231a75894 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>
@@ -1046,13 +1047,20 @@ static struct uart_driver dbc_uart_driver = {
.flush = dbc_uart_flush,
};
-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(64);
-static struct xhci_dbc_ctx ctx __aligned(64);
-static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
-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] __aligned(DBC_PAGE_SIZE);
+ 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 dbc_dma_bufs __section(".bss.page_aligned");
static char __initdata opt_dbgp[30];
string_param("dbgp", opt_dbgp);
@@ -1087,16 +1095,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.sbdf);
serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
+ }
}
#ifdef DBC_DEBUG
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (7 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-05 8:15 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 10/10] driver/char: add RX support to the XHCI driver Marek Marczykowski-Górecki
2022-07-26 6:18 ` [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Jan Beulich
10 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
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().
Add an option to allow/deny other domains to use the USB controller. By
default, if XHCI console is enabled, Xen will take the whole controller
for itself, using `dbgp=xhci,share=hwdom` or `=any` allows other ports
to be used by either only dom0 or any dom0 that get this PCI device
assigned.
In any case, to avoid Linux messing with the DbC, mark this MMIO area as
read-only.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
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 | 12 ++-
xen/drivers/char/xhci-dbc.c | 115 ++++++++++++++++++++++++++++---
2 files changed, 118 insertions(+), 9 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index e53efdb324b3..cc1e1989b17e 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -724,7 +724,7 @@ Available alternatives, with their meaning, are:
### dbgp
> `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
-> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
+> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=none|hwdom|any]`
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).
@@ -732,6 +732,16 @@ 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.
+The `share` option for xhci controls who else can use the controller:
+* `none`: use the controller exclusively for console, even hardware domain
+ (dom0) cannot use it; this is the default
+* `hwdom`: hardware domain may use the controller too, ports not used for debug
+ console will be available for normal devices
+* `any`: the controller can be assigned to any domain; it is not safe to assign
+ the controller to untrusted domain
+
+Choosing `share=hwdom` or `share=any` allows a domain to reset the controller,
+which may cause small portion of the console output to be lost.
### debug_stack_lines
> `= <integer>`
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 546231a75894..805b447f2300 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>
@@ -228,6 +229,12 @@ struct dbc_work_ring {
uint64_t dma;
};
+enum xhci_share {
+ XHCI_SHARE_NONE = 0,
+ XHCI_SHARE_HWDOM,
+ XHCI_SHARE_ANY
+};
+
struct dbc {
struct dbc_reg __iomem *dbc_reg;
struct xhci_dbc_ctx *dbc_ctx;
@@ -244,6 +251,7 @@ struct dbc {
void __iomem *xhc_mmio;
bool open;
+ enum xhci_share share;
unsigned int xhc_num; /* look for n-th xhc */
};
@@ -871,8 +879,9 @@ 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)
{
@@ -880,6 +889,42 @@ static bool dbc_ensure_running(struct dbc *dbc)
uint32_t ctrl;
uint32_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);
+ }
+
+ if ( dbc->open && !(readl(®->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);
ctrl = readl(®->ctrl);
@@ -1005,10 +1050,32 @@ 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
+ 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)
@@ -1069,13 +1136,14 @@ 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;
if ( strncmp(opt_dbgp, "xhci", 4) )
return;
memset(dbc, 0, sizeof(*dbc));
+ e = &opt_dbgp[4];
if ( isdigit(opt_dbgp[4]) )
{
dbc->xhc_num = simple_strtoul(opt_dbgp + 4, &e, 10);
@@ -1085,7 +1153,7 @@ void __init xhci_dbc_uart_init(void)
unsigned int bus, slot, func;
e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
- if ( !e || *e )
+ if ( !e || (*e && *e != ',') )
{
printk(XENLOG_ERR
"Invalid dbgp= PCI device spec: '%s'\n",
@@ -1094,6 +1162,37 @@ void __init xhci_dbc_uart_init(void)
}
dbc->sbdf = PCI_SBDF(0, bus, slot, func);
}
+ opt = e;
+
+ /* other options */
+ while ( opt && *opt == ',' )
+ {
+ opt++;
+ e = strchr(opt, ',');
+ if ( !e )
+ e = strchr(opt, '\0');
+
+ if ( !strncmp(opt, "share=", 6) )
+ {
+ if ( !cmdline_strcmp(opt + 6, "none") )
+ dbc->share = XHCI_SHARE_NONE;
+ else if ( !cmdline_strcmp(opt + 6, "hwdom") )
+ dbc->share = XHCI_SHARE_HWDOM;
+ else if ( !cmdline_strcmp(opt + 6, "any") )
+ dbc->share = XHCI_SHARE_ANY;
+ else
+ break;
+ }
+ else
+ break;
+
+ opt = e;
+ }
+ if ( !opt || *opt )
+ {
+ printk(XENLOG_ERR "Invalid dbgp= parameters: '%s'\n", opt_dbgp);
+ return;
+ }
dbc->dbc_ctx = &dbc_dma_bufs.ctx;
dbc->dbc_erst = &dbc_dma_bufs.erst;
--
git-series 0.9.1
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [PATCH v3 10/10] driver/char: add RX support to the XHCI driver
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (8 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC Marek Marczykowski-Górecki
@ 2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-05 8:38 ` Jan Beulich
2022-07-26 6:18 ` [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Jan Beulich
10 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 3:23 UTC (permalink / raw)
To: xen-devel
Cc: Marek Marczykowski-Górecki, Andrew Cooper, George Dunlap,
Jan Beulich, Julien Grall, Stefano Stabellini, Wei Liu
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>
---
New patch in v3
---
docs/misc/xen-command-line.pandoc | 4 +-
xen/drivers/char/xhci-dbc.c | 121 +++++++++++++++++++++++++++++++-
2 files changed, 123 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index cc1e1989b17e..07174badac8f 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -729,8 +729,8 @@ Available alternatives, with their meaning, are:
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
+Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability.
+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:
* `none`: use the controller exclusively for console, even hardware domain
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 805b447f2300..ccf4f9bbe2b7 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -109,6 +109,7 @@ enum {
enum {
XHCI_TRB_CC_SUCCESS = 1,
XHCI_TRB_CC_TRB_ERR = 5,
+ XHCI_TRB_CC_SHORT_PACKET = 13,
};
/* DbC endpoint types */
@@ -243,6 +244,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;
@@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
trb->ctrl |= 0x20;
}
+static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb)
+{
+ return trb->params;
+}
+
+static uint32_t xhci_trb_norm_len(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
@@ -454,6 +466,12 @@ 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)
{
@@ -495,6 +513,14 @@ static int xhci_trb_ring_full(const struct xhci_trb_ring *ring)
return ((ring->enq + 1) & (DBC_TRB_RING_CAP - 1)) == ring->deq;
}
+static 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 int dbc_work_ring_full(const struct dbc_work_ring *ring)
{
return ((ring->enq + 1) & (DBC_WORK_RING_CAP - 1)) == ring->deq;
@@ -508,6 +534,14 @@ static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring)
return DBC_WORK_RING_CAP - ring->deq + ring->enq;
}
+static uint64_t 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)
{
@@ -568,6 +602,31 @@ static int64_t 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
@@ -577,6 +636,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(®->erdp);
uint32_t portsc;
@@ -602,6 +662,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);
@@ -872,6 +940,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;
@@ -985,6 +1054,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(®->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, ®->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.
@@ -1007,6 +1103,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;
@@ -1032,6 +1141,9 @@ static void cf_check dbc_uart_poll(void *data)
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));
}
@@ -1092,6 +1204,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;
@@ -1111,6 +1229,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,
};
@@ -1119,6 +1238,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] __aligned(DBC_PAGE_SIZE);
+ uint8_t in_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
struct xhci_erst_segment erst __aligned(16);
struct xhci_dbc_ctx ctx __aligned(16);
struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
@@ -1200,6 +1320,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
^ permalink raw reply related [flat|nested] 45+ messages in thread
* Re: [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
` (9 preceding siblings ...)
2022-07-26 3:23 ` [PATCH v3 10/10] driver/char: add RX support to the XHCI driver Marek Marczykowski-Górecki
@ 2022-07-26 6:18 ` Jan Beulich
2022-07-26 9:26 ` Marek Marczykowski-Górecki
10 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-07-26 6:18 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
Paul Durrant, Kevin Tian, Connor Davis, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> 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
>
> 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>
>
> Connor Davis (1):
> drivers/char: Add support for USB3 DbC debugger
I notice this patch was sent twice, about 4 min ahead of the full set.
I take it that the set having come close together is the definitive
one, and that one extra copy is the one to be ignored.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability
2022-07-26 6:18 ` [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Jan Beulich
@ 2022-07-26 9:26 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-07-26 9:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
Paul Durrant, Kevin Tian, Connor Davis, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2084 bytes --]
On Tue, Jul 26, 2022 at 08:18:40AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > 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
> >
> > 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>
> >
> > Connor Davis (1):
> > drivers/char: Add support for USB3 DbC debugger
>
> I notice this patch was sent twice, about 4 min ahead of the full set.
> I take it that the set having come close together is the definitive
> one, and that one extra copy is the one to be ignored.
Yes, I noticed too late I forgot to drop Connor's broken address, that's
the only difference.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-07-26 3:23 ` Marek Marczykowski-Górecki
(?)
@ 2022-08-04 12:57 ` Jan Beulich
2022-08-04 13:43 ` Marek Marczykowski-Górecki
-1 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 12:57 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> +/* Defines the size in bytes of TRB rings as 2^DBC_TRB_RING_ORDER * 4096 */
> +#ifndef DBC_TRB_RING_ORDER
> +#define DBC_TRB_RING_ORDER 4
> +#endif
> +#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE * (1 << DBC_TRB_RING_ORDER))
I have to admit that I'm always puzzled when seeing such - why not
#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE << DBC_TRB_RING_ORDER)
?
> +struct dbc {
> + struct dbc_reg __iomem *dbc_reg;
> + struct xhci_dbc_ctx *dbc_ctx;
> + struct xhci_erst_segment *dbc_erst;
> + struct xhci_trb_ring dbc_ering;
> + struct xhci_trb_ring dbc_oring;
> + struct xhci_trb_ring dbc_iring;
> + struct dbc_work_ring dbc_owork;
> + struct xhci_string_descriptor *dbc_str;
I'm afraid I still don't see why the static page this field is being
initialized with is necessary. Can't you have a static variable (of
some struct type) all pre-filled at build time, which you then apply
virt_to_maddr() to in order to fill the respective dbc_ctx fields?
That struct will be quite a bit less than a page's worth in size.
If you build the file with -fshort-wchar, you may even be able to
use easy to read string literals for the initializer.
> +static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
> +{
> + size_t i;
> +
> + if ( size != MAX_XHCI_PAGES * DBC_PAGE_SIZE )
> + return NULL;
> +
> + for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
> + {
> + set_fixmap_nocache(i, phys);
> + phys += DBC_PAGE_SIZE;
While there may be an assumption of DBC_PAGE_SIZE == PAGE_SIZE, the
constant used here clearly needs to be PAGE_SIZE, as that's the unit
set_fixmap_nocache() deals with.
> +static bool __init dbc_init_xhc(struct dbc *dbc)
> +{
> + uint32_t bar0;
> + uint64_t bar1;
> + uint64_t bar_size;
> + uint64_t devfn;
> + uint16_t cmd;
> + size_t xhc_mmio_size;
> +
> + /*
> + * Search PCI bus 0 for the xHC. All the host controllers supported so far
> + * are part of the chipset and are on bus 0.
> + */
> + for ( devfn = 0; devfn < 256; devfn++ )
> + {
> + pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> + uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> +
> + if ( hdr == 0 || hdr == 0x80 )
> + {
> + if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == DBC_XHC_CLASSC )
> + {
> + dbc->sbdf = sbdf;
> + break;
> + }
> + }
> + }
> +
> + if ( !dbc->sbdf.sbdf )
> + {
> + dbc_error("Compatible xHC not found on bus 0\n");
> + return false;
> + }
> +
> + /* ...we found it, so parse the BAR and map the registers */
> + bar0 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> + bar1 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1);
> +
> + /* IO BARs not allowed; BAR must be 64-bit */
> + if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY ||
> + (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != PCI_BASE_ADDRESS_MEM_TYPE_64 )
> + return false;
> +
> + cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
> + pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
> +
> + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
> + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, 0xFFFFFFFF);
> + bar_size = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> + bar_size |= (uint64_t)pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1) << 32;
> + xhc_mmio_size = ~(bar_size & PCI_BASE_ADDRESS_MEM_MASK) + 1;
> + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, bar0);
> + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, bar1);
> +
> + pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
> +
> + dbc->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
> + dbc->xhc_mmio = dbc_sys_map_xhc(dbc->xhc_mmio_phys, xhc_mmio_size);
Before actually using the address to map the MMIO you will want to make
somewhat sure firmware did initialize it: The EHCI driver checks for
all zeroes or all ones in the writable bits.
> +/**
> + * The first register of the debug capability is found by traversing the
> + * host controller's capability list (xcap) until a capability
> + * with ID = 0xA is found. The xHCI capability list begins at address
> + * mmio + (HCCPARAMS1[31:16] << 2).
> + */
> +static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
> +{
> + uint32_t *xcap;
const?
> + uint32_t xcap_val;
> + uint32_t next;
> + uint32_t id = 0;
> + uint8_t *mmio = (uint8_t *)dbc->xhc_mmio;
Can't this be const void * (and probably wants to also use __iomem),
avoiding the cast here, ...
> + uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
... here, ...
> + const uint32_t DBC_ID = 0xA;
> + int ttl = 48;
> +
> + xcap = (uint32_t *)dbc->xhc_mmio;
... and here (if actually using the local variable you've got).
> +/*
> + * Note that if IN transfer support is added, then this
> + * will need to be changed; it assumes an OUT transfer ring only
> + */
Hmm, is this comment telling me that this driver is an output-only one?
Or is it simply that input doesn't use this code path?
> +static void dbc_init_string_single(struct xhci_string_descriptor *string,
> + char *ascii_str,
If this function has to survive, then const please here and ...
> + uint64_t *str_ptr,
> + uint8_t *str_size_ptr)
> +{
> + size_t i, len = strlen(ascii_str);
> +
> + string->size = offsetof(typeof(*string), string) + len * 2;
> + string->type = XHCI_DT_STRING;
> + /* ASCII to UTF16 conversion */
> + for (i = 0; i < len; i++)
... this missing blanks added here.
> +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(64);
> +static struct xhci_dbc_ctx ctx __aligned(64);
> +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> +static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> +static char __initdata opt_dbgp[30];
> +
> +string_param("dbgp", opt_dbgp);
This duplicates what ehci-dbgp.c already has. I guess we can live with
it for now and de-duplicate later, but it's still a little odd. In any
even please move the blank line up be a line, so that string_param()
and its referenced array are kept together.
> +void __init xhci_dbc_uart_init(void)
> +{
> + struct dbc_uart *uart = &dbc_uart;
> + struct dbc *dbc = &uart->dbc;
> +
> + if ( strncmp(opt_dbgp, "xhci", 4) )
> + return;
> +
> + memset(dbc, 0, sizeof(*dbc));
Why? dbc_uart is a static variable, and hence already zero-initialized.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 02/10] drivers/char: reset XHCI ports when initializing dbc
2022-07-26 3:23 ` [PATCH v3 02/10] drivers/char: reset XHCI ports when initializing dbc Marek Marczykowski-Górecki
@ 2022-08-04 13:14 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 13:14 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> Reset ports, to force host system to re-enumerate devices. Otheriwse it
> will require the cable to be re-plugged, or will wait in the
> "configuring" state indefinitely.
>
> Trick and code copied from Linux:
> drivers/usb/early/xhci-dbc.c:xdbc_start()->xdbc_reset_debug_port()
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 12:57 ` Jan Beulich
@ 2022-08-04 13:43 ` Marek Marczykowski-Górecki
2022-08-04 14:21 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-04 13:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 8694 bytes --]
On Thu, Aug 04, 2022 at 02:57:49PM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > +/* Defines the size in bytes of TRB rings as 2^DBC_TRB_RING_ORDER * 4096 */
> > +#ifndef DBC_TRB_RING_ORDER
> > +#define DBC_TRB_RING_ORDER 4
> > +#endif
> > +#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE * (1 << DBC_TRB_RING_ORDER))
>
> I have to admit that I'm always puzzled when seeing such - why not
>
> #define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE << DBC_TRB_RING_ORDER)
>
> ?
I think the former is a bit more clear what's the actual size, but the
end result is the same, I can change that.
> > +struct dbc {
> > + struct dbc_reg __iomem *dbc_reg;
> > + struct xhci_dbc_ctx *dbc_ctx;
> > + struct xhci_erst_segment *dbc_erst;
> > + struct xhci_trb_ring dbc_ering;
> > + struct xhci_trb_ring dbc_oring;
> > + struct xhci_trb_ring dbc_iring;
> > + struct dbc_work_ring dbc_owork;
> > + struct xhci_string_descriptor *dbc_str;
>
> I'm afraid I still don't see why the static page this field is being
> initialized with is necessary. Can't you have a static variable (of
> some struct type) all pre-filled at build time, which you then apply
> virt_to_maddr() to in order to fill the respective dbc_ctx fields?
I need to keep this structure somewhere DMA-reachable for the device (as
in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
also, patch 8/10 is putting it together with other DMA-reachable
structures (not a separate page on its own). If I'd make it a separate
static variable (not part of that later struct), I'd need to reserve the
whole page for it - to guarantee no unrelated data lives on the same
(DMA-reachable) page.
As for statically initializing it, if would require the whole
(multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
binary (not a huge concern due to compression, but still). But more
importantly, I don't know how to do it in a readable way, and you have
complained about readability of initializer of this structure in v2.
> That struct will be quite a bit less than a page's worth in size.
See above - it cannot share page with unrelated Xen data.
> If you build the file with -fshort-wchar, you may even be able to
> use easy to read string literals for the initializer.
I can try, but I'm not exactly sure how to make readable UTF-16
literals...
> > +static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
> > +{
> > + size_t i;
> > +
> > + if ( size != MAX_XHCI_PAGES * DBC_PAGE_SIZE )
> > + return NULL;
> > +
> > + for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
> > + {
> > + set_fixmap_nocache(i, phys);
> > + phys += DBC_PAGE_SIZE;
>
> While there may be an assumption of DBC_PAGE_SIZE == PAGE_SIZE, the
> constant used here clearly needs to be PAGE_SIZE, as that's the unit
> set_fixmap_nocache() deals with.
Ok.
> > +static bool __init dbc_init_xhc(struct dbc *dbc)
> > +{
> > + uint32_t bar0;
> > + uint64_t bar1;
> > + uint64_t bar_size;
> > + uint64_t devfn;
> > + uint16_t cmd;
> > + size_t xhc_mmio_size;
> > +
> > + /*
> > + * Search PCI bus 0 for the xHC. All the host controllers supported so far
> > + * are part of the chipset and are on bus 0.
> > + */
> > + for ( devfn = 0; devfn < 256; devfn++ )
> > + {
> > + pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> > + uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> > +
> > + if ( hdr == 0 || hdr == 0x80 )
> > + {
> > + if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == DBC_XHC_CLASSC )
> > + {
> > + dbc->sbdf = sbdf;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if ( !dbc->sbdf.sbdf )
> > + {
> > + dbc_error("Compatible xHC not found on bus 0\n");
> > + return false;
> > + }
> > +
> > + /* ...we found it, so parse the BAR and map the registers */
> > + bar0 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> > + bar1 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1);
> > +
> > + /* IO BARs not allowed; BAR must be 64-bit */
> > + if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY ||
> > + (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != PCI_BASE_ADDRESS_MEM_TYPE_64 )
> > + return false;
> > +
> > + cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
> > + pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
> > +
> > + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
> > + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, 0xFFFFFFFF);
> > + bar_size = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> > + bar_size |= (uint64_t)pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1) << 32;
> > + xhc_mmio_size = ~(bar_size & PCI_BASE_ADDRESS_MEM_MASK) + 1;
> > + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, bar0);
> > + pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, bar1);
> > +
> > + pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
> > +
> > + dbc->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
> > + dbc->xhc_mmio = dbc_sys_map_xhc(dbc->xhc_mmio_phys, xhc_mmio_size);
>
> Before actually using the address to map the MMIO you will want to make
> somewhat sure firmware did initialize it: The EHCI driver checks for
> all zeroes or all ones in the writable bits.
Ok.
>
> > +/**
> > + * The first register of the debug capability is found by traversing the
> > + * host controller's capability list (xcap) until a capability
> > + * with ID = 0xA is found. The xHCI capability list begins at address
> > + * mmio + (HCCPARAMS1[31:16] << 2).
> > + */
> > +static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
> > +{
> > + uint32_t *xcap;
>
> const?
>
> > + uint32_t xcap_val;
> > + uint32_t next;
> > + uint32_t id = 0;
> > + uint8_t *mmio = (uint8_t *)dbc->xhc_mmio;
>
> Can't this be const void * (and probably wants to also use __iomem),
> avoiding the cast here, ...
>
> > + uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
>
> ... here, ...
>
> > + const uint32_t DBC_ID = 0xA;
> > + int ttl = 48;
> > +
> > + xcap = (uint32_t *)dbc->xhc_mmio;
>
> ... and here (if actually using the local variable you've got).
Ok.
> > +/*
> > + * Note that if IN transfer support is added, then this
> > + * will need to be changed; it assumes an OUT transfer ring only
> > + */
>
> Hmm, is this comment telling me that this driver is an output-only one?
At this point, yes. Input support is added in patch 10/10.
> Or is it simply that input doesn't use this code path?
>
> > +static void dbc_init_string_single(struct xhci_string_descriptor *string,
> > + char *ascii_str,
>
> If this function has to survive, then const please here and ...
>
> > + uint64_t *str_ptr,
> > + uint8_t *str_size_ptr)
> > +{
> > + size_t i, len = strlen(ascii_str);
> > +
> > + string->size = offsetof(typeof(*string), string) + len * 2;
> > + string->type = XHCI_DT_STRING;
> > + /* ASCII to UTF16 conversion */
> > + for (i = 0; i < len; i++)
>
> ... this missing blanks added here.
Ok.
> > +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(64);
> > +static struct xhci_dbc_ctx ctx __aligned(64);
> > +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> > +static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> > +static char __initdata opt_dbgp[30];
> > +
> > +string_param("dbgp", opt_dbgp);
>
> This duplicates what ehci-dbgp.c already has. I guess we can live with
> it for now and de-duplicate later, but it's still a little odd. In any
> even please move the blank line up be a line, so that string_param()
> and its referenced array are kept together.
>
> > +void __init xhci_dbc_uart_init(void)
> > +{
> > + struct dbc_uart *uart = &dbc_uart;
> > + struct dbc *dbc = &uart->dbc;
> > +
> > + if ( strncmp(opt_dbgp, "xhci", 4) )
> > + return;
> > +
> > + memset(dbc, 0, sizeof(*dbc));
>
> Why? dbc_uart is a static variable, and hence already zero-initialized.
Ok.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 04/10] console: support multiple serial console simultaneously
2022-07-26 3:23 ` [PATCH v3 04/10] console: support multiple serial console simultaneously Marek Marczykowski-Górecki
@ 2022-08-04 14:13 ` Jan Beulich
2022-08-05 7:41 ` Jan Beulich
1 sibling, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:13 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> --- 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.
Indentation (the help text ought to be indented by a tab and two spaces).
> --- 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 int __read_mostly nr_sercon_handle = 0;
unsigned int please.
> @@ -393,32 +395,59 @@ 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[MAX_SERCONS+1])(const char *, size_t nr) = {
Nit: blanks please around + . But with ...
> + [MAX_SERCONS] = early_puts,
... this initializer you could as well omit the dimension.
> +};
>
> +/*
> + * 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;
> + int i;
While from the use inside the function this would better be unsigned int,
I can see how that would be slightly odd with the return value. But with
overly high a MAX_SERCONS ...
> + 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] )
... the array access will degenerate when i is "int", but will be okay
when i is "unsigned int" (and of course everything will break if
MAX_SERCONS > UINT_MAX).
> + 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);
> + int i;
unsigned int please, again (yet more further down).
> + 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);
This for() would better gain braces.
> @@ -977,8 +1006,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);
In order to use p here, I think we want (need?) to make
serial_parse_handle()'s parameter const char *.
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1078,8 +1078,12 @@ void __init xhci_dbc_uart_init(void)
>
> e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
> if ( !e || *e )
> + {
> + printk(XENLOG_ERR
> + "Invalid dbgp= PCI device spec: '%s'\n",
> + opt_dbgp);
> return;
> -
> + }
> dbc->sbdf = PCI_SBDF(0, bus, slot, func);
> }
Does this change belong elsewhere?
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 13:43 ` Marek Marczykowski-Górecki
@ 2022-08-04 14:21 ` Jan Beulich
2022-08-04 14:28 ` Marek Marczykowski-Górecki
2022-08-04 14:34 ` Jan Beulich
0 siblings, 2 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:21 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 04.08.2022 15:43, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 04, 2022 at 02:57:49PM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>> +struct dbc {
>>> + struct dbc_reg __iomem *dbc_reg;
>>> + struct xhci_dbc_ctx *dbc_ctx;
>>> + struct xhci_erst_segment *dbc_erst;
>>> + struct xhci_trb_ring dbc_ering;
>>> + struct xhci_trb_ring dbc_oring;
>>> + struct xhci_trb_ring dbc_iring;
>>> + struct dbc_work_ring dbc_owork;
>>> + struct xhci_string_descriptor *dbc_str;
>>
>> I'm afraid I still don't see why the static page this field is being
>> initialized with is necessary. Can't you have a static variable (of
>> some struct type) all pre-filled at build time, which you then apply
>> virt_to_maddr() to in order to fill the respective dbc_ctx fields?
>
> I need to keep this structure somewhere DMA-reachable for the device (as
> in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
> also, patch 8/10 is putting it together with other DMA-reachable
> structures (not a separate page on its own). If I'd make it a separate
> static variable (not part of that later struct), I'd need to reserve the
> whole page for it - to guarantee no unrelated data lives on the same
> (DMA-reachable) page.
>
> As for statically initializing it, if would require the whole
> (multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
> binary (not a huge concern due to compression, but still). But more
> importantly, I don't know how to do it in a readable way, and you have
> complained about readability of initializer of this structure in v2.
>
>> That struct will be quite a bit less than a page's worth in size.
>
> See above - it cannot share page with unrelated Xen data.
I have to admit that I'd see no issue if these lived side by side with
e.g. other string literals. The more that the device is supposed to be
exposed to Dom0 only anyway, and hence that'll be the only domain able
to get at that data.
>> If you build the file with -fshort-wchar, you may even be able to
>> use easy to read string literals for the initializer.
>
> I can try, but I'm not exactly sure how to make readable UTF-16
> literals...
L"Xen" looks sufficiently readable to me. We use this all over the
place in the EFI interfacing code.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 05/10] IOMMU: add common API for device reserved memory
2022-07-26 3:23 ` [PATCH v3 05/10] IOMMU: add common API for device reserved memory Marek Marczykowski-Górecki
@ 2022-08-04 14:25 ` Jan Beulich
2022-08-04 14:38 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:25 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Paul Durrant, Roger Pau Monné, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -651,6 +651,51 @@ 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;
> + uint32_t sbdf;
It's not easy to judge why this isn't pci_sbdf_t when no callers
exist at this point.
> +};
> +static unsigned int __initdata nr_extra_reserved_ranges;
> +static struct extra_reserved_range __initdata
> + extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
With __initdata here, ...
> +int iommu_add_extra_reserved_device_memory(unsigned long start,
> + unsigned long nr,
> + uint32_t sbdf)
... this and the other function want to be __init.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 14:21 ` Jan Beulich
@ 2022-08-04 14:28 ` Marek Marczykowski-Górecki
2022-08-04 14:36 ` Jan Beulich
2022-08-04 14:34 ` Jan Beulich
1 sibling, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-04 14:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]
On Thu, Aug 04, 2022 at 04:21:01PM +0200, Jan Beulich wrote:
> On 04.08.2022 15:43, Marek Marczykowski-Górecki wrote:
> > I need to keep this structure somewhere DMA-reachable for the device (as
> > in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
> > also, patch 8/10 is putting it together with other DMA-reachable
> > structures (not a separate page on its own). If I'd make it a separate
> > static variable (not part of that later struct), I'd need to reserve the
> > whole page for it - to guarantee no unrelated data lives on the same
> > (DMA-reachable) page.
> >
> > As for statically initializing it, if would require the whole
> > (multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
> > binary (not a huge concern due to compression, but still). But more
> > importantly, I don't know how to do it in a readable way, and you have
> > complained about readability of initializer of this structure in v2.
> >
> >> That struct will be quite a bit less than a page's worth in size.
> >
> > See above - it cannot share page with unrelated Xen data.
>
> I have to admit that I'd see no issue if these lived side by side with
> e.g. other string literals. The more that the device is supposed to be
> exposed to Dom0 only anyway, and hence that'll be the only domain able
> to get at that data.
Other string literals are fine. But for example `struct dbc` itself is
not.
See how it is combined with other data in patch 8.
> >> If you build the file with -fshort-wchar, you may even be able to
> >> use easy to read string literals for the initializer.
> >
> > I can try, but I'm not exactly sure how to make readable UTF-16
> > literals...
>
> L"Xen" looks sufficiently readable to me. We use this all over the
> place in the EFI interfacing code.
Ok, I can try that. But given later adjustments, IIUC it will make the
whole 50+ pages structure land in .data. Is that okay?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 14:21 ` Jan Beulich
2022-08-04 14:28 ` Marek Marczykowski-Górecki
@ 2022-08-04 14:34 ` Jan Beulich
2022-08-05 6:14 ` Jan Beulich
1 sibling, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:34 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 04.08.2022 16:21, Jan Beulich wrote:
> On 04.08.2022 15:43, Marek Marczykowski-Górecki wrote:
>> On Thu, Aug 04, 2022 at 02:57:49PM +0200, Jan Beulich wrote:
>>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>>> +struct dbc {
>>>> + struct dbc_reg __iomem *dbc_reg;
>>>> + struct xhci_dbc_ctx *dbc_ctx;
>>>> + struct xhci_erst_segment *dbc_erst;
>>>> + struct xhci_trb_ring dbc_ering;
>>>> + struct xhci_trb_ring dbc_oring;
>>>> + struct xhci_trb_ring dbc_iring;
>>>> + struct dbc_work_ring dbc_owork;
>>>> + struct xhci_string_descriptor *dbc_str;
>>>
>>> I'm afraid I still don't see why the static page this field is being
>>> initialized with is necessary. Can't you have a static variable (of
>>> some struct type) all pre-filled at build time, which you then apply
>>> virt_to_maddr() to in order to fill the respective dbc_ctx fields?
>>
>> I need to keep this structure somewhere DMA-reachable for the device (as
>> in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
>> also, patch 8/10 is putting it together with other DMA-reachable
>> structures (not a separate page on its own). If I'd make it a separate
>> static variable (not part of that later struct), I'd need to reserve the
>> whole page for it - to guarantee no unrelated data lives on the same
>> (DMA-reachable) page.
>>
>> As for statically initializing it, if would require the whole
>> (multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
>> binary (not a huge concern due to compression, but still). But more
>> importantly, I don't know how to do it in a readable way, and you have
>> complained about readability of initializer of this structure in v2.
>>
>>> That struct will be quite a bit less than a page's worth in size.
>>
>> See above - it cannot share page with unrelated Xen data.
>
> I have to admit that I'd see no issue if these lived side by side with
> e.g. other string literals. The more that the device is supposed to be
> exposed to Dom0 only anyway, and hence that'll be the only domain able
> to get at that data.
Actually: With your plan to expose the device to a DomU, how is that
going to work without tool stack adjustments? Wouldn't you need to
prevent in particular HVM/PVH guests from using the GFNs corresponding
to the MFNs where this Xen data is? The minimal requirement then would
seem to be an E820 reserved range for the area.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 14:28 ` Marek Marczykowski-Górecki
@ 2022-08-04 14:36 ` Jan Beulich
2022-08-04 14:41 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:36 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 04.08.2022 16:28, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 04, 2022 at 04:21:01PM +0200, Jan Beulich wrote:
>> On 04.08.2022 15:43, Marek Marczykowski-Górecki wrote:
>>> I need to keep this structure somewhere DMA-reachable for the device (as
>>> in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
>>> also, patch 8/10 is putting it together with other DMA-reachable
>>> structures (not a separate page on its own). If I'd make it a separate
>>> static variable (not part of that later struct), I'd need to reserve the
>>> whole page for it - to guarantee no unrelated data lives on the same
>>> (DMA-reachable) page.
>>>
>>> As for statically initializing it, if would require the whole
>>> (multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
>>> binary (not a huge concern due to compression, but still). But more
>>> importantly, I don't know how to do it in a readable way, and you have
>>> complained about readability of initializer of this structure in v2.
>>>
>>>> That struct will be quite a bit less than a page's worth in size.
>>>
>>> See above - it cannot share page with unrelated Xen data.
>>
>> I have to admit that I'd see no issue if these lived side by side with
>> e.g. other string literals. The more that the device is supposed to be
>> exposed to Dom0 only anyway, and hence that'll be the only domain able
>> to get at that data.
>
> Other string literals are fine. But for example `struct dbc` itself is
> not.
> See how it is combined with other data in patch 8.
>
>>>> If you build the file with -fshort-wchar, you may even be able to
>>>> use easy to read string literals for the initializer.
>>>
>>> I can try, but I'm not exactly sure how to make readable UTF-16
>>> literals...
>>
>> L"Xen" looks sufficiently readable to me. We use this all over the
>> place in the EFI interfacing code.
>
> Ok, I can try that. But given later adjustments, IIUC it will make the
> whole 50+ pages structure land in .data. Is that okay?
No. I was actually expecting the piece of data we're talking about here
to land in .rodata, and hence be re-usable at the same address for all
devices. Hence my reference to string literals.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 05/10] IOMMU: add common API for device reserved memory
2022-08-04 14:25 ` Jan Beulich
@ 2022-08-04 14:38 ` Marek Marczykowski-Górecki
2022-08-04 14:41 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-04 14:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: Paul Durrant, Roger Pau Monné, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]
On Thu, Aug 04, 2022 at 04:25:38PM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -651,6 +651,51 @@ 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;
> > + uint32_t sbdf;
>
> It's not easy to judge why this isn't pci_sbdf_t when no callers
> exist at this point.
I'm following here types used in the rest of IOMMU code. Especially,
this field is later passed to iommu_grdm_t func, which is:
typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt);
^^^^
I can probably use pci_sbdf_t here, but it will be cast to u32 later
anyway...
> > +};
> > +static unsigned int __initdata nr_extra_reserved_ranges;
> > +static struct extra_reserved_range __initdata
> > + extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
>
> With __initdata here, ...
>
> > +int iommu_add_extra_reserved_device_memory(unsigned long start,
> > + unsigned long nr,
> > + uint32_t sbdf)
>
> ... this and the other function want to be __init.
Ok.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 14:36 ` Jan Beulich
@ 2022-08-04 14:41 ` Marek Marczykowski-Górecki
2022-08-04 14:49 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-04 14:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 1030 bytes --]
On Thu, Aug 04, 2022 at 04:36:35PM +0200, Jan Beulich wrote:
> On 04.08.2022 16:28, Marek Marczykowski-Górecki wrote:
> > On Thu, Aug 04, 2022 at 04:21:01PM +0200, Jan Beulich wrote:
> >> L"Xen" looks sufficiently readable to me. We use this all over the
> >> place in the EFI interfacing code.
> >
> > Ok, I can try that. But given later adjustments, IIUC it will make the
> > whole 50+ pages structure land in .data. Is that okay?
>
> No. I was actually expecting the piece of data we're talking about here
> to land in .rodata, and hence be re-usable at the same address for all
> devices. Hence my reference to string literals.
"all devices" - this driver supports only a single xhci debug console at
a time. Anyway, as explained earlier, it would require reserving the
whole page for it (there are no other xhci-related structures that can
live in .rodata), which given your earlier comments about memory usage
is probably worse.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 05/10] IOMMU: add common API for device reserved memory
2022-08-04 14:38 ` Marek Marczykowski-Górecki
@ 2022-08-04 14:41 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:41 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Paul Durrant, Roger Pau Monné, xen-devel
On 04.08.2022 16:38, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 04, 2022 at 04:25:38PM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -651,6 +651,51 @@ 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;
>>> + uint32_t sbdf;
>>
>> It's not easy to judge why this isn't pci_sbdf_t when no callers
>> exist at this point.
>
> I'm following here types used in the rest of IOMMU code. Especially,
> this field is later passed to iommu_grdm_t func, which is:
>
> typedef int iommu_grdm_t(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt);
> ^^^^
>
> I can probably use pci_sbdf_t here, but it will be cast to u32 later
> anyway...
No, rather than a cast you'd use the union's sbdf field. And yes, eventually
that function typedef you refer to will want switching to pci_sbdf_t as well.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 14:41 ` Marek Marczykowski-Górecki
@ 2022-08-04 14:49 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:49 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 04.08.2022 16:41, Marek Marczykowski-Górecki wrote:
> On Thu, Aug 04, 2022 at 04:36:35PM +0200, Jan Beulich wrote:
>> On 04.08.2022 16:28, Marek Marczykowski-Górecki wrote:
>>> On Thu, Aug 04, 2022 at 04:21:01PM +0200, Jan Beulich wrote:
>>>> L"Xen" looks sufficiently readable to me. We use this all over the
>>>> place in the EFI interfacing code.
>>>
>>> Ok, I can try that. But given later adjustments, IIUC it will make the
>>> whole 50+ pages structure land in .data. Is that okay?
>>
>> No. I was actually expecting the piece of data we're talking about here
>> to land in .rodata, and hence be re-usable at the same address for all
>> devices. Hence my reference to string literals.
>
> "all devices" - this driver supports only a single xhci debug console at
> a time.
Oh, sorry - I've got confused by your multiple consoles patch here.
> Anyway, as explained earlier, it would require reserving the
> whole page for it (there are no other xhci-related structures that can
> live in .rodata), which given your earlier comments about memory usage
> is probably worse.
In your earlier reply you did say you'd see no issue with this sitting
side by side with other string literals. Which is precisely how I
would envision to avoid the need to reserve the entire page. But yes,
if that's not feasible, then the current model of keeping the stuff
in .bss is likely best. A remark in the description towards the purpose
here and the further intentions might help, not the least to avoid me
coming up with the same comment again for a future version of the
series.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 07/10] IOMMU/AMD: wire common device reserved memory API
2022-07-26 3:23 ` [PATCH v3 07/10] IOMMU/AMD: " Marek Marczykowski-Górecki
@ 2022-08-04 14:53 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-04 14:53 UTC (permalink / raw)
To: Marek Marczykowski-Górecki; +Cc: Andrew Cooper, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> 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>
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-04 14:34 ` Jan Beulich
@ 2022-08-05 6:14 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 6:14 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 04.08.2022 16:34, Jan Beulich wrote:
> On 04.08.2022 16:21, Jan Beulich wrote:
>> On 04.08.2022 15:43, Marek Marczykowski-Górecki wrote:
>>> On Thu, Aug 04, 2022 at 02:57:49PM +0200, Jan Beulich wrote:
>>>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>>>> +struct dbc {
>>>>> + struct dbc_reg __iomem *dbc_reg;
>>>>> + struct xhci_dbc_ctx *dbc_ctx;
>>>>> + struct xhci_erst_segment *dbc_erst;
>>>>> + struct xhci_trb_ring dbc_ering;
>>>>> + struct xhci_trb_ring dbc_oring;
>>>>> + struct xhci_trb_ring dbc_iring;
>>>>> + struct dbc_work_ring dbc_owork;
>>>>> + struct xhci_string_descriptor *dbc_str;
>>>>
>>>> I'm afraid I still don't see why the static page this field is being
>>>> initialized with is necessary. Can't you have a static variable (of
>>>> some struct type) all pre-filled at build time, which you then apply
>>>> virt_to_maddr() to in order to fill the respective dbc_ctx fields?
>>>
>>> I need to keep this structure somewhere DMA-reachable for the device (as
>>> in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
>>> also, patch 8/10 is putting it together with other DMA-reachable
>>> structures (not a separate page on its own). If I'd make it a separate
>>> static variable (not part of that later struct), I'd need to reserve the
>>> whole page for it - to guarantee no unrelated data lives on the same
>>> (DMA-reachable) page.
>>>
>>> As for statically initializing it, if would require the whole
>>> (multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
>>> binary (not a huge concern due to compression, but still). But more
>>> importantly, I don't know how to do it in a readable way, and you have
>>> complained about readability of initializer of this structure in v2.
>>>
>>>> That struct will be quite a bit less than a page's worth in size.
>>>
>>> See above - it cannot share page with unrelated Xen data.
>>
>> I have to admit that I'd see no issue if these lived side by side with
>> e.g. other string literals. The more that the device is supposed to be
>> exposed to Dom0 only anyway, and hence that'll be the only domain able
>> to get at that data.
>
> Actually: With your plan to expose the device to a DomU, how is that
> going to work without tool stack adjustments? Wouldn't you need to
> prevent in particular HVM/PVH guests from using the GFNs corresponding
> to the MFNs where this Xen data is? The minimal requirement then would
> seem to be an E820 reserved range for the area.
I guess this was rubbish - by mimic-ing RMRRs or their AMD equivalents,
the tool stack ought to be taking care of this already.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI
2022-07-26 3:23 ` [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI Marek Marczykowski-Górecki
@ 2022-08-05 7:05 ` Jan Beulich
2022-08-05 10:11 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 7:05 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> @@ -1046,13 +1047,20 @@ static struct uart_driver dbc_uart_driver = {
> .flush = dbc_uart_flush,
> };
>
> -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(64);
> -static struct xhci_dbc_ctx ctx __aligned(64);
Why the change from 64 ...
> -static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> -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] __aligned(DBC_PAGE_SIZE);
> + struct xhci_erst_segment erst __aligned(16);
> + struct xhci_dbc_ctx ctx __aligned(16);
... to 16?
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-07-26 3:23 ` Marek Marczykowski-Górecki
(?)
(?)
@ 2022-08-05 7:23 ` Jan Beulich
2022-08-05 9:51 ` Marek Marczykowski-Górecki
-1 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 7:23 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> +static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring)
> +{
> + if ( ring->enq >= ring->deq )
> + return ring->enq - ring->deq;
> +
> + return DBC_WORK_RING_CAP - ring->deq + ring->enq;
> +}
Doesn't unsigned int suffice as a return type here?
> +static int64_t dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring,
> + const char *buf, unsigned int len)
> +{
> + unsigned int i = 0;
> + unsigned int end, start = ring->enq;
> +
> + while ( !dbc_work_ring_full(ring) && i < len )
> + {
> + ring->buf[ring->enq] = buf[i++];
> + ring->enq = (ring->enq + 1) & (DBC_WORK_RING_CAP - 1);
> + }
> +
> + end = ring->enq;
> +
> + if ( end > start )
> + cache_flush(&ring->buf[start], end - start);
> + else if ( i > 0 )
> + {
> + cache_flush(&ring->buf[start], DBC_WORK_RING_CAP - start);
> + cache_flush(&ring->buf[0], end);
> + }
> +
> + return i;
> +}
The function's return type is int64_t but the sole return statement
hands back an unsigned int - what's the deal here?
> +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(64);
> +static struct xhci_dbc_ctx ctx __aligned(64);
> +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
I've been trying to identify the reason for the alignment here,
compared to the other buffers which are no longer page-aligned. I
haven't even been able to locate the place where the address of
this buffer is actually written to hardware; all I could find was
the respective virt_to_maddr(). Could you please point me at that?
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 04/10] console: support multiple serial console simultaneously
2022-07-26 3:23 ` [PATCH v3 04/10] console: support multiple serial console simultaneously Marek Marczykowski-Górecki
2022-08-04 14:13 ` Jan Beulich
@ 2022-08-05 7:41 ` Jan Beulich
2022-08-05 13:11 ` Marek Marczykowski-Górecki
1 sibling, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 7:41 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 26.07.2022 05:23, 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>
Actually I should have clarified yesterday that despite my effort to
review this change, I'm not convinced of its need. I simply don't see
the use case of having multiple serial consoles at a time, and afaict
console and gdb connection can already be run over different channels.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
2022-07-26 3:23 ` [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC Marek Marczykowski-Górecki
@ 2022-08-05 8:15 ` Jan Beulich
2022-08-05 15:49 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 8:15 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 26.07.2022 05:23, 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.
Which is one of the main reasons why I view DomU exposure as
going too far, despite recognizing the argument that this would only
be done if that DomU is fully trusted.
Furthermore - what's the effect of this? It would seem to me that
while bus mastering is off, the device will not function. What happens
to output occurring during that time window? Rather than needing to
re-enable bus mastering behind the owning domain's back, can't the
disabling of bus mastering be avoided in the driver there? As we can
see from the EHCI driver, there certainly can be communication
between Xen and Dom0 for functionality-impacting operations Dom0
might perform (there it's a device reset iirc).
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -724,7 +724,7 @@ Available alternatives, with their meaning, are:
>
> ### dbgp
> > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
> -> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
> +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=none|hwdom|any]`
>
> 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).
> @@ -732,6 +732,16 @@ 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.
> +The `share` option for xhci controls who else can use the controller:
> +* `none`: use the controller exclusively for console, even hardware domain
> + (dom0) cannot use it; this is the default
> +* `hwdom`: hardware domain may use the controller too, ports not used for debug
> + console will be available for normal devices
> +* `any`: the controller can be assigned to any domain; it is not safe to assign
> + the controller to untrusted domain
I'm sorry, upon looking here more closely, can we use proper boolean
here as we do elsewhere, i.e. share=no|yes|hwdom (or more generically
expressed share=<boolean>|hwdom)?
I also think 'hwdom' should be the default, like we do for EHCI (with,
at present, not even a way to override).
> +Choosing `share=hwdom` or `share=any` allows a domain to reset the controller,
> +which may cause small portion of the console output to be lost.
As said above - this ought to be avoidable if the period of time the
reset takes is bounded and if the controlling domain announces the
reset and its completion. See ehci-dbgp.c:dbgp_op().
In any event I'd like to ask that you add a statement to the effect of
"no security support when using 'any'".
> @@ -1005,10 +1050,32 @@ 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
> + 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");
How can this allow use of the device by a domain? Is there some sort of
guarantee that nothing else will live in the same 4k range? I can't
infer such from xhci_find_dbc().
> @@ -1085,7 +1153,7 @@ void __init xhci_dbc_uart_init(void)
> unsigned int bus, slot, func;
>
> e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
> - if ( !e || *e )
> + if ( !e || (*e && *e != ',') )
> {
> printk(XENLOG_ERR
> "Invalid dbgp= PCI device spec: '%s'\n",
> @@ -1094,6 +1162,37 @@ void __init xhci_dbc_uart_init(void)
> }
> dbc->sbdf = PCI_SBDF(0, bus, slot, func);
> }
> + opt = e;
Looks like e (and hence opt) cannot be NULL here, ...
> + /* other options */
> + while ( opt && *opt == ',' )
> + {
> + opt++;
> + e = strchr(opt, ',');
> + if ( !e )
> + e = strchr(opt, '\0');
> +
> + if ( !strncmp(opt, "share=", 6) )
> + {
> + if ( !cmdline_strcmp(opt + 6, "none") )
> + dbc->share = XHCI_SHARE_NONE;
> + else if ( !cmdline_strcmp(opt + 6, "hwdom") )
> + dbc->share = XHCI_SHARE_HWDOM;
> + else if ( !cmdline_strcmp(opt + 6, "any") )
> + dbc->share = XHCI_SHARE_ANY;
> + else
> + break;
> + }
> + else
> + break;
> +
> + opt = e;
... nor here. Hence I wonder why the while() and ...
> + }
> + if ( !opt || *opt )
... this if() check for it being (non-)NULL. At which point ...
> + {
> + printk(XENLOG_ERR "Invalid dbgp= parameters: '%s'\n", opt_dbgp);
... you could make the message here more specific by passing "opt"
instead of the full "opt_dbgp".
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver
2022-07-26 3:23 ` [PATCH v3 10/10] driver/char: add RX support to the XHCI driver Marek Marczykowski-Górecki
@ 2022-08-05 8:38 ` Jan Beulich
2022-08-05 9:58 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 8:38 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> @@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
> trb->ctrl |= 0x20;
> }
>
> +static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb)
const please.
> +{
> + return trb->params;
> +}
> +
> +static uint32_t xhci_trb_norm_len(struct xhci_trb *trb)
And again.
> +{
> + return trb->status & 0x1FFFF;
> +}
> +
> /**
> * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
> * TRBs are read-only from software
> @@ -454,6 +466,12 @@ 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;
> +}
Same as xhci_trb_norm_len()?
> @@ -985,6 +1054,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)
I can't seem to be able to spot any use of this function - it being
static, how do things build for you?
> +{
> + struct dbc_reg *reg = dbc->dbc_reg;
> + uint32_t db = (readl(®->db) & 0xFFFF00FF) | (trb->db << 8);
I think I've seen this constant in earlier patches. Can this be
a #define please, such that one can easily connect all the places
where the same things is meant?
> +
> + /* Check if there is already queued TRB */
> + if ( xhci_trb_ring_size(trb) >= 1 )
> + return;
> +
> + if ( dbc_work_ring_full(wrk) )
> + return;
What made me spot the lack of caller are these return statements.
Without letting the caller know of the failure, how would it know
to make another attempt later?
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-05 7:23 ` Jan Beulich
@ 2022-08-05 9:51 ` Marek Marczykowski-Górecki
2022-08-05 9:54 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 9:51 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 2376 bytes --]
On Fri, Aug 05, 2022 at 09:23:32AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > +static uint64_t dbc_work_ring_size(const struct dbc_work_ring *ring)
> > +{
> > + if ( ring->enq >= ring->deq )
> > + return ring->enq - ring->deq;
> > +
> > + return DBC_WORK_RING_CAP - ring->deq + ring->enq;
> > +}
>
> Doesn't unsigned int suffice as a return type here?
Yes, it does.
> > +static int64_t dbc_push_work(struct dbc *dbc, struct dbc_work_ring *ring,
> > + const char *buf, unsigned int len)
> > +{
> > + unsigned int i = 0;
> > + unsigned int end, start = ring->enq;
> > +
> > + while ( !dbc_work_ring_full(ring) && i < len )
> > + {
> > + ring->buf[ring->enq] = buf[i++];
> > + ring->enq = (ring->enq + 1) & (DBC_WORK_RING_CAP - 1);
> > + }
> > +
> > + end = ring->enq;
> > +
> > + if ( end > start )
> > + cache_flush(&ring->buf[start], end - start);
> > + else if ( i > 0 )
> > + {
> > + cache_flush(&ring->buf[start], DBC_WORK_RING_CAP - start);
> > + cache_flush(&ring->buf[0], end);
> > + }
> > +
> > + return i;
> > +}
>
> The function's return type is int64_t but the sole return statement
> hands back an unsigned int - what's the deal here?
And also, the only use for the return value is comparing to 0. So, yes,
should be unsigned int.
> > +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(64);
> > +static struct xhci_dbc_ctx ctx __aligned(64);
> > +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
>
> I've been trying to identify the reason for the alignment here,
> compared to the other buffers which are no longer page-aligned. I
> haven't even been able to locate the place where the address of
> this buffer is actually written to hardware; all I could find was
> the respective virt_to_maddr(). Could you please point me at that?
It's dbc_flush() -> dbc_push_trb().
And indeed, I think I can drop the alignment when it's moved into
structure dedicated for DMA-accessible buffers.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-05 9:51 ` Marek Marczykowski-Górecki
@ 2022-08-05 9:54 ` Jan Beulich
2022-08-05 10:01 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 9:54 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
On 05.08.2022 11:51, Marek Marczykowski-Górecki wrote:
> On Fri, Aug 05, 2022 at 09:23:32AM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>> +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(64);
>>> +static struct xhci_dbc_ctx ctx __aligned(64);
>>> +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
>>
>> I've been trying to identify the reason for the alignment here,
>> compared to the other buffers which are no longer page-aligned. I
>> haven't even been able to locate the place where the address of
>> this buffer is actually written to hardware; all I could find was
>> the respective virt_to_maddr(). Could you please point me at that?
>
> It's dbc_flush() -> dbc_push_trb().
> And indeed, I think I can drop the alignment when it's moved into
> structure dedicated for DMA-accessible buffers.
Why would you be able to drop the alignment then, but not here?
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver
2022-08-05 8:38 ` Jan Beulich
@ 2022-08-05 9:58 ` Marek Marczykowski-Górecki
2022-08-05 12:38 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 9:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2593 bytes --]
On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > @@ -440,6 +442,16 @@ static void xhci_trb_norm_set_ioc(struct xhci_trb *trb)
> > trb->ctrl |= 0x20;
> > }
> >
> > +static uint64_t xhci_trb_norm_buf(struct xhci_trb *trb)
>
> const please.
>
> > +{
> > + return trb->params;
> > +}
> > +
> > +static uint32_t xhci_trb_norm_len(struct xhci_trb *trb)
>
> And again.
>
> > +{
> > + return trb->status & 0x1FFFF;
> > +}
> > +
> > /**
> > * Fields for Transfer Event TRBs (see section 6.4.2.1). Note that event
> > * TRBs are read-only from software
> > @@ -454,6 +466,12 @@ 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;
> > +}
>
> Same as xhci_trb_norm_len()?
Yes, I was considering to use that, but technically those are different
packets, only incidentally using the same bits.
>
> > @@ -985,6 +1054,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)
>
> I can't seem to be able to spot any use of this function - it being
> static, how do things build for you?
It's in dbc_uart_poll().
>
> > +{
> > + struct dbc_reg *reg = dbc->dbc_reg;
> > + uint32_t db = (readl(®->db) & 0xFFFF00FF) | (trb->db << 8);
>
> I think I've seen this constant in earlier patches. Can this be
> a #define please, such that one can easily connect all the places
> where the same things is meant?
Ok.
> > +
> > + /* Check if there is already queued TRB */
> > + if ( xhci_trb_ring_size(trb) >= 1 )
> > + return;
> > +
> > + if ( dbc_work_ring_full(wrk) )
> > + return;
>
> What made me spot the lack of caller are these return statements.
> Without letting the caller know of the failure, how would it know
> to make another attempt later?
Next iteration of dbc_uart_poll() will take care of it.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger
2022-08-05 9:54 ` Jan Beulich
@ 2022-08-05 10:01 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 10:01 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, Roger Pau Monné,
xen-devel
[-- Attachment #1: Type: text/plain, Size: 1600 bytes --]
On Fri, Aug 05, 2022 at 11:54:06AM +0200, Jan Beulich wrote:
> On 05.08.2022 11:51, Marek Marczykowski-Górecki wrote:
> > On Fri, Aug 05, 2022 at 09:23:32AM +0200, Jan Beulich wrote:
> >> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> >>> +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(64);
> >>> +static struct xhci_dbc_ctx ctx __aligned(64);
> >>> +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> >>
> >> I've been trying to identify the reason for the alignment here,
> >> compared to the other buffers which are no longer page-aligned. I
> >> haven't even been able to locate the place where the address of
> >> this buffer is actually written to hardware; all I could find was
> >> the respective virt_to_maddr(). Could you please point me at that?
> >
> > It's dbc_flush() -> dbc_push_trb().
> > And indeed, I think I can drop the alignment when it's moved into
> > structure dedicated for DMA-accessible buffers.
>
> Why would you be able to drop the alignment then, but not here?
Similar reason as previously - to guarantee it isn't put with unrelated
stuff on the same page, as this array will be accessed by the controller
directly. But, since this patch doesn't attempt to handle IOMMU
situation yet, I think I can drop it here too as there is little benefit
for trying here.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI
2022-08-05 7:05 ` Jan Beulich
@ 2022-08-05 10:11 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 10:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]
On Fri, Aug 05, 2022 at 09:05:27AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > @@ -1046,13 +1047,20 @@ static struct uart_driver dbc_uart_driver = {
> > .flush = dbc_uart_flush,
> > };
> >
> > -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(64);
> > -static struct xhci_dbc_ctx ctx __aligned(64);
>
> Why the change from 64 ...
>
> > -static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> > -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] __aligned(DBC_PAGE_SIZE);
> > + struct xhci_erst_segment erst __aligned(16);
> > + struct xhci_dbc_ctx ctx __aligned(16);
>
> ... to 16?
That's rebase fail, it should be changed to 16 initial patch too.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver
2022-08-05 9:58 ` Marek Marczykowski-Górecki
@ 2022-08-05 12:38 ` Jan Beulich
2022-08-05 12:47 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 12:38 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote:
> On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>> @@ -454,6 +466,12 @@ 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;
>>> +}
>>
>> Same as xhci_trb_norm_len()?
>
> Yes, I was considering to use that, but technically those are different
> packets, only incidentally using the same bits.
I see. That's the problem with using literal numbers rather than #define-s.
But for a driver like this I didn't want to be overly picky, and hence
would have wanted to let you get away without introducing many more.
>>> @@ -985,6 +1054,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)
>>
>> I can't seem to be able to spot any use of this function - it being
>> static, how do things build for you?
>
> It's in dbc_uart_poll().
Oh, interesting. This then means that patch 1 on its own doesn't build.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver
2022-08-05 12:38 ` Jan Beulich
@ 2022-08-05 12:47 ` Marek Marczykowski-Górecki
2022-08-05 12:51 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 12:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]
On Fri, Aug 05, 2022 at 02:38:26PM +0200, Jan Beulich wrote:
> On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote:
> > On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote:
> >> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> >>> @@ -454,6 +466,12 @@ 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;
> >>> +}
> >>
> >> Same as xhci_trb_norm_len()?
> >
> > Yes, I was considering to use that, but technically those are different
> > packets, only incidentally using the same bits.
>
> I see. That's the problem with using literal numbers rather than #define-s.
> But for a driver like this I didn't want to be overly picky, and hence
> would have wanted to let you get away without introducing many more.
That's kind of the purpose of those helper functions - to extract
specific fields according to the xhci interface, one per function. An
alternative could be #define _instead of_ those functions, but I think
that would be worse. What I mean, is the function name serves as
description of that those constants are about.
> >>> @@ -985,6 +1054,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)
> >>
> >> I can't seem to be able to spot any use of this function - it being
> >> static, how do things build for you?
> >
> > It's in dbc_uart_poll().
>
> Oh, interesting. This then means that patch 1 on its own doesn't build.
Right, I need to move the call into this patch.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver
2022-08-05 12:47 ` Marek Marczykowski-Górecki
@ 2022-08-05 12:51 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-05 12:51 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 05.08.2022 14:47, Marek Marczykowski-Górecki wrote:
> On Fri, Aug 05, 2022 at 02:38:26PM +0200, Jan Beulich wrote:
>> On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote:
>>> On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote:
>>>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>>>> @@ -454,6 +466,12 @@ 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;
>>>>> +}
>>>>
>>>> Same as xhci_trb_norm_len()?
>>>
>>> Yes, I was considering to use that, but technically those are different
>>> packets, only incidentally using the same bits.
>>
>> I see. That's the problem with using literal numbers rather than #define-s.
>> But for a driver like this I didn't want to be overly picky, and hence
>> would have wanted to let you get away without introducing many more.
>
> That's kind of the purpose of those helper functions - to extract
> specific fields according to the xhci interface, one per function. An
> alternative could be #define _instead of_ those functions, but I think
> that would be worse. What I mean, is the function name serves as
> description of that those constants are about.
Right - as said, fair enough for a driver like this.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 04/10] console: support multiple serial console simultaneously
2022-08-05 7:41 ` Jan Beulich
@ 2022-08-05 13:11 ` Marek Marczykowski-Górecki
0 siblings, 0 replies; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 13:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 1437 bytes --]
On Fri, Aug 05, 2022 at 09:41:09AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, 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>
>
> Actually I should have clarified yesterday that despite my effort to
> review this change, I'm not convinced of its need. I simply don't see
> the use case of having multiple serial consoles at a time, and afaict
> console and gdb connection can already be run over different channels.
I agree the usefulness is limited. I needed this only to debug the xhci
console driver itself. But since I did this change already, I figured
I'd share it as it might be useful for others in similar situation.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
2022-08-05 8:15 ` Jan Beulich
@ 2022-08-05 15:49 ` Marek Marczykowski-Górecki
2022-08-09 6:24 ` Jan Beulich
0 siblings, 1 reply; 45+ messages in thread
From: Marek Marczykowski-Górecki @ 2022-08-05 15:49 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
[-- Attachment #1: Type: text/plain, Size: 10429 bytes --]
On Fri, Aug 05, 2022 at 10:15:59AM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, 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.
>
> Which is one of the main reasons why I view DomU exposure as
> going too far, despite recognizing the argument that this would only
> be done if that DomU is fully trusted.
>
> Furthermore - what's the effect of this? It would seem to me that
> while bus mastering is off, the device will not function. What happens
> to output occurring during that time window?
If no reset happens, the controller will continue sending the data after
bus mastering is enabled back - no data lost in this case. If reset does
happen, data that was already handed off to the controller (TRB queued)
but not sent yet, is lost. But data that is still queued only in the
work_ring, will be sent after controller is re-initialized. I did
several tests of this, and I have not noticed any data loss in practice.
> Rather than needing to
> re-enable bus mastering behind the owning domain's back, can't the
> disabling of bus mastering be avoided in the driver there?
Linux disables bus mastering when PCI devices are enumerated (before
xhci driver is loaded at all), and enables it back only when xhci driver
tells it so. So, if xhci driver in dom0 is blacklisted (which is the
case in qubes by default...), the console would be much less useful, so
to say. And I don't think Linux maintainers will appreciate xen-xhci-dbc
specific code in core PCI handling...
It isn't an issue for EHCI driver, because EHCI debug port
interface does not seem to use DMA.
> As we can
> see from the EHCI driver, there certainly can be communication
> between Xen and Dom0 for functionality-impacting operations Dom0
> might perform (there it's a device reset iirc).
Yes, I can see how controller reset can be coordinated this way. But
also, I see it more like a future improvement if it deemed to be
necessary, than a strict requirement, as the controller reset is a quick
event that in practice does not impact the functionality in any
significant way (with the current code shape). On the other hand, adding
such synchronization feels like several more iterations of this
series...
And BTW, if Linux crashes in the middle of controller reset, with such
synchronization you would not get the crash message at all. While
admittedly such issue is rather unlikely, I see it as a potential
downside of coordination here (you could still avoid it by
`share=none`/`share=no`, with all its consequences).
Generally, what would be your minimal acceptable version? If support for
sharing the controller with a domain (including dom0) requires
significantly more work to be accepted, I'd much prefer to drop it in
this series and have it possibly introduced later (and in the meantime,
possibly carry as a downstream patch). Unfortunately, I have limited
time to work on the series, but also, I think having this feature
upstream - even in partial form - will be very useful for many Xen users
and developers. Especially, I'd like this series (in some shape) to be
included in Xen 4.17.
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -724,7 +724,7 @@ Available alternatives, with their meaning, are:
> >
> > ### dbgp
> > > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
> > -> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
> > +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=none|hwdom|any]`
> >
> > 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).
> > @@ -732,6 +732,16 @@ 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.
> > +The `share` option for xhci controls who else can use the controller:
> > +* `none`: use the controller exclusively for console, even hardware domain
> > + (dom0) cannot use it; this is the default
> > +* `hwdom`: hardware domain may use the controller too, ports not used for debug
> > + console will be available for normal devices
> > +* `any`: the controller can be assigned to any domain; it is not safe to assign
> > + the controller to untrusted domain
>
> I'm sorry, upon looking here more closely, can we use proper boolean
> here as we do elsewhere, i.e. share=no|yes|hwdom (or more generically
> expressed share=<boolean>|hwdom)?
>
> I also think 'hwdom' should be the default, like we do for EHCI (with,
> at present, not even a way to override).
Yes, I can do that.
> > +Choosing `share=hwdom` or `share=any` allows a domain to reset the controller,
> > +which may cause small portion of the console output to be lost.
>
> As said above - this ought to be avoidable if the period of time the
> reset takes is bounded and if the controlling domain announces the
> reset and its completion. See ehci-dbgp.c:dbgp_op().
>
> In any event I'd like to ask that you add a statement to the effect of
> "no security support when using 'any'".
Sure, I can add it more even explicitly (there is already "it is not
safe to assign the controller to untrusted domain").
>
> > @@ -1005,10 +1050,32 @@ 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
> > + 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");
>
> How can this allow use of the device by a domain? Is there some sort of
> guarantee that nothing else will live in the same 4k range? I can't
> infer such from xhci_find_dbc().
That's a very good question. From what I see, it lives on a page
together with other extended capabilities (but nothing else). Most of
registers in other capabilities are read-only, but there are some
read-write. It seems Linux driver works fine without writing to any of
them, but it sounds very fragile...
The main reason for this code is to prevent Linux initializing DbC for
itself. But AFAIK Linux does not do it on its own, it requires explicit
action from the system admin (a write to sysfs or kernel option).
I'm not exactly sure what will happen if Linux will try to use DbC too,
my guess is either Xen console will stall, or they will fight each other
by re-initializing DbC over and over. Neither of them look appealing...
Would you prefer to drop this part, in favor of documenting it's the
system admin responsibility to prevent Linux from using it? In that
case, I think the default should remain `share=no` (possibly changing
only after implementing some coordination with Linux side).
> > @@ -1085,7 +1153,7 @@ void __init xhci_dbc_uart_init(void)
> > unsigned int bus, slot, func;
> >
> > e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func);
> > - if ( !e || *e )
> > + if ( !e || (*e && *e != ',') )
> > {
> > printk(XENLOG_ERR
> > "Invalid dbgp= PCI device spec: '%s'\n",
> > @@ -1094,6 +1162,37 @@ void __init xhci_dbc_uart_init(void)
> > }
> > dbc->sbdf = PCI_SBDF(0, bus, slot, func);
> > }
> > + opt = e;
>
> Looks like e (and hence opt) cannot be NULL here, ...
>
> > + /* other options */
> > + while ( opt && *opt == ',' )
> > + {
> > + opt++;
> > + e = strchr(opt, ',');
> > + if ( !e )
> > + e = strchr(opt, '\0');
> > +
> > + if ( !strncmp(opt, "share=", 6) )
> > + {
> > + if ( !cmdline_strcmp(opt + 6, "none") )
> > + dbc->share = XHCI_SHARE_NONE;
> > + else if ( !cmdline_strcmp(opt + 6, "hwdom") )
> > + dbc->share = XHCI_SHARE_HWDOM;
> > + else if ( !cmdline_strcmp(opt + 6, "any") )
> > + dbc->share = XHCI_SHARE_ANY;
> > + else
> > + break;
> > + }
> > + else
> > + break;
> > +
> > + opt = e;
>
> ... nor here. Hence I wonder why the while() and ...
>
> > + }
> > + if ( !opt || *opt )
>
> ... this if() check for it being (non-)NULL. At which point ...
>
> > + {
> > + printk(XENLOG_ERR "Invalid dbgp= parameters: '%s'\n", opt_dbgp);
>
> ... you could make the message here more specific by passing "opt"
> instead of the full "opt_dbgp".
Yes, indeed.
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
2022-08-05 15:49 ` Marek Marczykowski-Górecki
@ 2022-08-09 6:24 ` Jan Beulich
0 siblings, 0 replies; 45+ messages in thread
From: Jan Beulich @ 2022-08-09 6:24 UTC (permalink / raw)
To: Marek Marczykowski-Górecki
Cc: Andrew Cooper, George Dunlap, Julien Grall, Stefano Stabellini,
Wei Liu, xen-devel
On 05.08.2022 17:49, Marek Marczykowski-Górecki wrote:
> On Fri, Aug 05, 2022 at 10:15:59AM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, 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.
>>
>> Which is one of the main reasons why I view DomU exposure as
>> going too far, despite recognizing the argument that this would only
>> be done if that DomU is fully trusted.
>>
>> Furthermore - what's the effect of this? It would seem to me that
>> while bus mastering is off, the device will not function. What happens
>> to output occurring during that time window?
>
> If no reset happens, the controller will continue sending the data after
> bus mastering is enabled back - no data lost in this case. If reset does
> happen, data that was already handed off to the controller (TRB queued)
> but not sent yet, is lost. But data that is still queued only in the
> work_ring, will be sent after controller is re-initialized. I did
> several tests of this, and I have not noticed any data loss in practice.
>
>> Rather than needing to
>> re-enable bus mastering behind the owning domain's back, can't the
>> disabling of bus mastering be avoided in the driver there?
>
> Linux disables bus mastering when PCI devices are enumerated (before
> xhci driver is loaded at all), and enables it back only when xhci driver
> tells it so. So, if xhci driver in dom0 is blacklisted (which is the
> case in qubes by default...), the console would be much less useful, so
> to say. And I don't think Linux maintainers will appreciate xen-xhci-dbc
> specific code in core PCI handling...
> It isn't an issue for EHCI driver, because EHCI debug port
> interface does not seem to use DMA.
>
>> As we can
>> see from the EHCI driver, there certainly can be communication
>> between Xen and Dom0 for functionality-impacting operations Dom0
>> might perform (there it's a device reset iirc).
>
> Yes, I can see how controller reset can be coordinated this way. But
> also, I see it more like a future improvement if it deemed to be
> necessary, than a strict requirement, as the controller reset is a quick
> event that in practice does not impact the functionality in any
> significant way (with the current code shape). On the other hand, adding
> such synchronization feels like several more iterations of this
> series...
>
> And BTW, if Linux crashes in the middle of controller reset, with such
> synchronization you would not get the crash message at all. While
> admittedly such issue is rather unlikely, I see it as a potential
> downside of coordination here (you could still avoid it by
> `share=none`/`share=no`, with all its consequences).
>
> Generally, what would be your minimal acceptable version? If support for
> sharing the controller with a domain (including dom0) requires
> significantly more work to be accepted, I'd much prefer to drop it in
> this series and have it possibly introduced later (and in the meantime,
> possibly carry as a downstream patch). Unfortunately, I have limited
> time to work on the series, but also, I think having this feature
> upstream - even in partial form - will be very useful for many Xen users
> and developers. Especially, I'd like this series (in some shape) to be
> included in Xen 4.17.
I think I could agree with such logic as a temporary measure, i.e. marked
clearly with a FIXME: or alike. The Kconfig option then also would want
marking "experimental" (or maybe it already is).
>>> @@ -1005,10 +1050,32 @@ 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
>>> + 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");
>>
>> How can this allow use of the device by a domain? Is there some sort of
>> guarantee that nothing else will live in the same 4k range? I can't
>> infer such from xhci_find_dbc().
>
> That's a very good question. From what I see, it lives on a page
> together with other extended capabilities (but nothing else). Most of
> registers in other capabilities are read-only, but there are some
> read-write. It seems Linux driver works fine without writing to any of
> them, but it sounds very fragile...
>
> The main reason for this code is to prevent Linux initializing DbC for
> itself. But AFAIK Linux does not do it on its own, it requires explicit
> action from the system admin (a write to sysfs or kernel option).
> I'm not exactly sure what will happen if Linux will try to use DbC too,
> my guess is either Xen console will stall, or they will fight each other
> by re-initializing DbC over and over. Neither of them look appealing...
>
> Would you prefer to drop this part, in favor of documenting it's the
> system admin responsibility to prevent Linux from using it? In that
> case, I think the default should remain `share=no` (possibly changing
> only after implementing some coordination with Linux side).
No, quite the other way around - it being there makes Xen's use safe,
at the risk of breaking Dom0 (or, for your purposes, DomU) functionality.
The latter, if necessary, would imo need restoring by way of emulating
all write accesses to the page.
Jan
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2022-08-09 6:25 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-26 3:23 [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Marek Marczykowski-Górecki
2022-07-26 3:19 ` [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger Marek Marczykowski-Górecki
2022-07-26 3:23 ` Marek Marczykowski-Górecki
2022-08-04 12:57 ` Jan Beulich
2022-08-04 13:43 ` Marek Marczykowski-Górecki
2022-08-04 14:21 ` Jan Beulich
2022-08-04 14:28 ` Marek Marczykowski-Górecki
2022-08-04 14:36 ` Jan Beulich
2022-08-04 14:41 ` Marek Marczykowski-Górecki
2022-08-04 14:49 ` Jan Beulich
2022-08-04 14:34 ` Jan Beulich
2022-08-05 6:14 ` Jan Beulich
2022-08-05 7:23 ` Jan Beulich
2022-08-05 9:51 ` Marek Marczykowski-Górecki
2022-08-05 9:54 ` Jan Beulich
2022-08-05 10:01 ` Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 02/10] drivers/char: reset XHCI ports when initializing dbc Marek Marczykowski-Górecki
2022-08-04 13:14 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 03/10] drivers/char: add support for selecting specific xhci Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 04/10] console: support multiple serial console simultaneously Marek Marczykowski-Górecki
2022-08-04 14:13 ` Jan Beulich
2022-08-05 7:41 ` Jan Beulich
2022-08-05 13:11 ` Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 05/10] IOMMU: add common API for device reserved memory Marek Marczykowski-Górecki
2022-08-04 14:25 ` Jan Beulich
2022-08-04 14:38 ` Marek Marczykowski-Górecki
2022-08-04 14:41 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 06/10] IOMMU/VT-d: wire common device reserved memory API Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 07/10] IOMMU/AMD: " Marek Marczykowski-Górecki
2022-08-04 14:53 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 08/10] drivers/char: mark DMA buffers as reserved for the XHCI Marek Marczykowski-Górecki
2022-08-05 7:05 ` Jan Beulich
2022-08-05 10:11 ` Marek Marczykowski-Górecki
2022-07-26 3:23 ` [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC Marek Marczykowski-Górecki
2022-08-05 8:15 ` Jan Beulich
2022-08-05 15:49 ` Marek Marczykowski-Górecki
2022-08-09 6:24 ` Jan Beulich
2022-07-26 3:23 ` [PATCH v3 10/10] driver/char: add RX support to the XHCI driver Marek Marczykowski-Górecki
2022-08-05 8:38 ` Jan Beulich
2022-08-05 9:58 ` Marek Marczykowski-Górecki
2022-08-05 12:38 ` Jan Beulich
2022-08-05 12:47 ` Marek Marczykowski-Górecki
2022-08-05 12:51 ` Jan Beulich
2022-07-26 6:18 ` [PATCH v3 00/10] Add Xue - console over USB 3 Debug Capability Jan Beulich
2022-07-26 9:26 ` Marek Marczykowski-Górecki
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.