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

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

[Marek Marczykowski-Górecki]

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

The driver developed by Connor supports output-only 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)

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 Xue USB3 debugger

Marek Marczykowski-Górecki (8):
  xue: reset XHCI ports when initializing dbc
  xue: 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
  xue: mark DMA buffers as reserved for the device
  xue: allow driving the rest of XHCI by a domain while Xen uses DbC

 docs/misc/xen-command-line.pandoc        |    5 +-
 xen/arch/x86/include/asm/fixmap.h        |    4 +-
 xen/arch/x86/setup.c                     |    3 +-
 xen/drivers/char/Kconfig                 |    9 +-
 xen/drivers/char/Makefile                |    1 +-
 xen/drivers/char/console.c               |   58 +-
 xen/drivers/char/xue.c                   | 1066 +++++++++++++++++++++++-
 xen/drivers/passthrough/amd/iommu_acpi.c |   16 +-
 xen/drivers/passthrough/iommu.c          |   40 +-
 xen/drivers/passthrough/vtd/dmar.c       |  203 ++--
 xen/include/xen/iommu.h                  |   11 +-
 xen/include/xen/serial.h                 |    3 +-
 12 files changed, 1323 insertions(+), 96 deletions(-)
 create mode 100644 xen/drivers/char/xue.c

base-commit: 0544c4ee4b48f7e2715e69ff3e73c3d5545b0526
-- 
git-series 0.9.1

[PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki]

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=xue' 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:
- 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 2MB 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 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              |   3 +-
 xen/drivers/char/Kconfig          |   9 +-
 xen/drivers/char/Makefile         |   1 +-
 xen/drivers/char/xue.c            | 933 +++++++++++++++++++++++++++++++-
 xen/include/xen/serial.h          |   3 +-
 7 files changed, 958 insertions(+)
 create mode 100644 xen/drivers/char/xue.c

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index a92b7d228cae..f9fa857bd84e 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> ]`
+> `= xue`
 
 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 `xue` for XHCI debug capability.
+Xue 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 53a73010e029..58a0723a7501 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -946,6 +946,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     ns16550.irq     = 3;
     ns16550_init(1, &ns16550);
     ehci_dbgp_init();
+#ifdef CONFIG_HAS_XHCI
+    xue_uart_init();
+#endif
     console_init_preirq();
 
     if ( pvh_boot )
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e5f7b1d8eb8a..d12b2205dafc 100644
--- a/xen/drivers/char/Kconfig
+++ b/xen/drivers/char/Kconfig
@@ -74,3 +74,12 @@ config HAS_EHCI
 	help
 	  This selects the USB based EHCI debug port to be used as a UART. If
 	  you have an x86 based system with USB, say Y.
+
+config HAS_XHCI
+	bool "XHCI DbC UART driver"
+	depends on X86
+	help
+	  This selects the USB based XHCI debug capability to be used as a UART.
+	  Enabling this option makes Xen use extra ~2MB memory, even if XHCI UART
+	  (XUE) 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..bda1e44d3f39 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_HAS_XHCI) += xue.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/xue.c b/xen/drivers/char/xue.c
new file mode 100644
index 000000000000..234b07b563bb
--- /dev/null
+++ b/xen/drivers/char/xue.c
@@ -0,0 +1,933 @@
+/*
+ * drivers/char/xue.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/types.h>
+#include <asm/string.h>
+#include <asm/system.h>
+#include <xen/serial.h>
+#include <xen/timer.h>
+#include <xen/param.h>
+#include <asm/fixmap.h>
+#include <asm/io.h>
+#include <xen/mm.h>
+
+/* uncomment to have xue_uart_dump() debug function */
+/* #define XUE_DEBUG 1 */
+
+#define XUE_POLL_INTERVAL 100 /* us */
+
+#define XUE_PAGE_SIZE 4096ULL
+
+/* Supported xHC PCI configurations */
+#define XUE_XHC_CLASSC 0xC0330U
+
+/* DbC idVendor and idProduct */
+#define XUE_DBC_VENDOR 0x1D6B
+#define XUE_DBC_PRODUCT 0x0010
+#define XUE_DBC_PROTOCOL 0x0000
+
+/* DCCTRL fields */
+#define XUE_CTRL_DCR 0
+#define XUE_CTRL_HOT 2
+#define XUE_CTRL_HIT 3
+#define XUE_CTRL_DRC 4
+#define XUE_CTRL_DCE 31
+
+/* DCPORTSC fields */
+#define XUE_PSC_PED 1
+#define XUE_PSC_CSC 17
+#define XUE_PSC_PRC 21
+#define XUE_PSC_PLC 22
+#define XUE_PSC_CEC 23
+
+#define XUE_PSC_ACK_MASK                                                       \
+    ((1UL << XUE_PSC_CSC) | (1UL << XUE_PSC_PRC) | (1UL << XUE_PSC_PLC) |      \
+     (1UL << XUE_PSC_CEC))
+
+#define xue_debug(...) printk("xue debug: " __VA_ARGS__)
+#define xue_alert(...) printk("xue alert: " __VA_ARGS__)
+#define xue_error(...) printk("xue 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
+ * xue is the consumer. This means that event TRBs are read-only from
+ * the xue.
+ *
+ * OTOH, xue is the producer of transfer TRBs on the two transfer
+ * rings, so xue enqueues transfers, and the hardware dequeues
+ * them. The dequeue pointer of a transfer ring is read by
+ * xue 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 {
+    xue_trb_norm = 1,
+    xue_trb_link = 6,
+    xue_trb_tfre = 32,
+    xue_trb_psce = 34
+};
+
+/* TRB completion codes */
+enum { xue_trb_cc_success = 1, xue_trb_cc_trb_err = 5 };
+
+/* DbC endpoint types */
+enum { xue_ep_bulk_out = 2, xue_ep_bulk_in = 6 };
+
+/* DMA/MMIO structures */
+struct xue_trb {
+    uint64_t params;
+    uint32_t status;
+    uint32_t ctrl;
+};
+
+struct xue_erst_segment {
+    uint64_t base;
+    uint16_t size;
+    uint8_t rsvdz[6];
+};
+
+#define XUE_CTX_SIZE 16
+#define XUE_CTX_BYTES (XUE_CTX_SIZE * 4)
+
+struct xue_dbc_ctx {
+    uint32_t info[XUE_CTX_SIZE];
+    uint32_t ep_out[XUE_CTX_SIZE];
+    uint32_t ep_in[XUE_CTX_SIZE];
+};
+
+struct xue_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 XUE_TRB_MAX_TFR (XUE_PAGE_SIZE << 4)
+#define XUE_TRB_PER_PAGE (XUE_PAGE_SIZE / sizeof(struct xue_trb))
+
+/* Defines the size in bytes of TRB rings as 2^XUE_TRB_RING_ORDER * 4096 */
+#ifndef XUE_TRB_RING_ORDER
+#define XUE_TRB_RING_ORDER 4
+#endif
+#define XUE_TRB_RING_CAP (XUE_TRB_PER_PAGE * (1ULL << XUE_TRB_RING_ORDER))
+#define XUE_TRB_RING_BYTES (XUE_TRB_RING_CAP * sizeof(struct xue_trb))
+#define XUE_TRB_RING_MASK (XUE_TRB_RING_BYTES - 1U)
+
+struct xue_trb_ring {
+    struct xue_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 */
+};
+
+#define XUE_DB_OUT 0x0
+#define XUE_DB_IN 0x1
+#define XUE_DB_INVAL 0xFF
+
+/* Defines the size in bytes of work rings as 2^XUE_WORK_RING_ORDER * 4096 */
+#ifndef XUE_WORK_RING_ORDER
+#define XUE_WORK_RING_ORDER 3
+#endif
+#define XUE_WORK_RING_CAP (XUE_PAGE_SIZE * (1ULL << XUE_WORK_RING_ORDER))
+#define XUE_WORK_RING_BYTES XUE_WORK_RING_CAP
+
+#if XUE_WORK_RING_CAP > XUE_TRB_MAX_TFR
+#error "XUE_WORK_RING_ORDER must be at most 4"
+#endif
+
+struct xue_work_ring {
+    uint8_t *buf;
+    uint32_t enq;
+    uint32_t deq;
+    uint64_t dma;
+};
+
+struct xue {
+    struct xue_dbc_reg *dbc_reg;
+    struct xue_dbc_ctx *dbc_ctx;
+    struct xue_erst_segment *dbc_erst;
+    struct xue_trb_ring dbc_ering;
+    struct xue_trb_ring dbc_oring;
+    struct xue_trb_ring dbc_iring;
+    struct xue_work_ring dbc_owork;
+    char *dbc_str;
+
+    pci_sbdf_t sbdf;
+    uint64_t xhc_mmio_phys;
+    uint64_t xhc_mmio_size;
+    uint64_t xhc_dbc_offset;
+    void *xhc_mmio;
+
+    int open;
+};
+
+static void xue_sys_pause(void)
+{
+    asm volatile("pause" ::: "memory");
+}
+
+static void *xue_sys_map_xhc(uint64_t phys, uint64_t size)
+{
+    size_t i;
+
+    if ( size != MAX_XHCI_PAGES * XUE_PAGE_SIZE )
+        return NULL;
+
+    for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
+    {
+        set_fixmap_nocache(i, phys);
+        phys += XUE_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 xue_init_xhc(struct xue *xue)
+{
+    uint32_t bar0;
+    uint64_t bar1;
+    uint64_t devfn;
+
+    /*
+     * 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);
+        uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
+
+        if ( hdr == 0 || hdr == 0x80 )
+        {
+            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == XUE_XHC_CLASSC )
+            {
+                xue->sbdf = sbdf;
+                break;
+            }
+        }
+    }
+
+    if ( !xue->sbdf.sbdf )
+    {
+        xue_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(xue->sbdf, PCI_BASE_ADDRESS_0);
+    bar1 = pci_conf_read32(xue->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;
+
+    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
+    xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) & 0xFFFFFFF0) + 1;
+    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, bar0);
+
+    xue->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
+    xue->xhc_mmio = xue_sys_map_xhc(xue->xhc_mmio_phys, xue->xhc_mmio_size);
+
+    return xue->xhc_mmio != NULL;
+}
+
+/**
+ * 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 xue_dbc_reg *xue_find_dbc(struct xue *xue)
+{
+    uint32_t *xcap;
+    uint32_t next;
+    uint32_t id;
+    uint8_t *mmio = (uint8_t *)xue->xhc_mmio;
+    uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
+    const uint32_t DBC_ID = 0xA;
+
+    /**
+     * Paranoid check against a zero value. The spec mandates that
+     * at least one "supported protocol" capability must be implemented,
+     * so this should always be false.
+     */
+    if ( (*hccp1 & 0xFFFF0000) == 0 )
+        return NULL;
+
+    xcap = (uint32_t *)(mmio + (((*hccp1 & 0xFFFF0000) >> 16) << 2));
+    next = (*xcap & 0xFF00) >> 8;
+    id = *xcap & 0xFF;
+
+    /**
+     * Table 7-1 states that 'next' is relative to
+     * the current value of xcap and is a dword offset.
+     */
+    while ( id != DBC_ID && next ) {
+        xcap += next;
+        id = *xcap & 0xFF;
+        next = (*xcap & 0xFF00) >> 8;
+    }
+
+    if ( id != DBC_ID )
+        return NULL;
+
+    xue->xhc_dbc_offset = (uint64_t)xcap - (uint64_t)mmio;
+    return (struct xue_dbc_reg *)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 xue_trb_cyc(const struct xue_trb *trb)
+{
+    return trb->ctrl & 0x1;
+}
+
+static uint32_t xue_trb_type(const struct xue_trb *trb)
+{
+    return (trb->ctrl & 0xFC00) >> 10;
+}
+
+static void xue_trb_set_cyc(struct xue_trb *trb, uint32_t c)
+{
+    trb->ctrl &= ~0x1UL;
+    trb->ctrl |= c;
+}
+
+static void xue_trb_set_type(struct xue_trb *trb, uint32_t t)
+{
+    trb->ctrl &= ~0xFC00UL;
+    trb->ctrl |= (t << 10);
+}
+
+/* Fields for normal TRBs */
+static void xue_trb_norm_set_buf(struct xue_trb *trb, uint64_t addr)
+{
+    trb->params = addr;
+}
+
+static void xue_trb_norm_set_len(struct xue_trb *trb, uint32_t len)
+{
+    trb->status &= ~0x1FFFFUL;
+    trb->status |= len;
+}
+
+static void xue_trb_norm_set_ioc(struct xue_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 xue_trb_tfre_ptr(const struct xue_trb *trb)
+{
+    return trb->params;
+}
+
+static uint32_t xue_trb_tfre_cc(const struct xue_trb *trb)
+{
+    return trb->status >> 24;
+}
+
+/* Fields for link TRBs (section 6.4.4.1) */
+static void xue_trb_link_set_rsp(struct xue_trb *trb, uint64_t rsp)
+{
+    trb->params = rsp;
+}
+
+static void xue_trb_link_set_tc(struct xue_trb *trb)
+{
+    trb->ctrl |= 0x2;
+}
+
+static void xue_trb_ring_init(const struct xue *xue,
+                              struct xue_trb_ring *ring, int producer,
+                              int doorbell)
+{
+    memset(ring->trb, 0, XUE_TRB_RING_CAP * sizeof(ring->trb[0]));
+
+    ring->enq = 0;
+    ring->deq = 0;
+    ring->cyc = 1;
+    ring->db = (uint8_t)doorbell;
+
+    /*
+     * Producer implies transfer ring, so we have to place a
+     * link TRB at the end that points back to trb[0]
+     */
+    if ( producer )
+    {
+        struct xue_trb *trb = &ring->trb[XUE_TRB_RING_CAP - 1];
+        xue_trb_set_type(trb, xue_trb_link);
+        xue_trb_link_set_tc(trb);
+        xue_trb_link_set_rsp(trb, virt_to_maddr(ring->trb));
+    }
+}
+
+static int xue_trb_ring_full(const struct xue_trb_ring *ring)
+{
+    return ((ring->enq + 1) & (XUE_TRB_RING_CAP - 1)) == ring->deq;
+}
+
+static int xue_work_ring_full(const struct xue_work_ring *ring)
+{
+    return ((ring->enq + 1) & (XUE_WORK_RING_CAP - 1)) == ring->deq;
+}
+
+static uint64_t xue_work_ring_size(const struct xue_work_ring *ring)
+{
+    if ( ring->enq >= ring->deq )
+        return ring->enq - ring->deq;
+
+    return XUE_WORK_RING_CAP - ring->deq + ring->enq;
+}
+
+static void xue_push_trb(struct xue *xue, struct xue_trb_ring *ring,
+                         uint64_t dma, uint64_t len)
+{
+    struct xue_trb trb;
+
+    if ( ring->enq == XUE_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 xue_trb *link = &ring->trb[ring->enq];
+        xue_trb_set_cyc(link, ring->cyc);
+
+        ring->enq = 0;
+        ring->cyc ^= 1;
+    }
+
+    trb.params = 0;
+    trb.status = 0;
+    trb.ctrl = 0;
+
+    xue_trb_set_type(&trb, xue_trb_norm);
+    xue_trb_set_cyc(&trb, ring->cyc);
+
+    xue_trb_norm_set_buf(&trb, dma);
+    xue_trb_norm_set_len(&trb, (uint32_t)len);
+    xue_trb_norm_set_ioc(&trb);
+
+    ring->trb[ring->enq++] = trb;
+    cache_flush(&ring->trb[ring->enq - 1], sizeof(trb));
+}
+
+static int64_t xue_push_work(struct xue *xue, struct xue_work_ring *ring,
+                             const char *buf, int64_t len)
+{
+    int64_t i = 0;
+    uint32_t start = ring->enq;
+    uint32_t end = 0;
+
+    while ( !xue_work_ring_full(ring) && i < len )
+    {
+        ring->buf[ring->enq] = buf[i++];
+        ring->enq = (ring->enq + 1) & (XUE_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], XUE_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 xue_pop_events(struct xue *xue)
+{
+    const int trb_shift = 4;
+
+    struct xue_dbc_reg *reg = xue->dbc_reg;
+    struct xue_trb_ring *er = &xue->dbc_ering;
+    struct xue_trb_ring *tr = &xue->dbc_oring;
+    struct xue_trb *event = &er->trb[er->deq];
+    uint64_t erdp = reg->erdp;
+
+    rmb();
+
+    while ( xue_trb_cyc(event) == er->cyc ) {
+        switch (xue_trb_type(event)) {
+        case xue_trb_tfre:
+            if ( xue_trb_tfre_cc(event) != xue_trb_cc_success )
+            {
+                xue_alert("tfre error cc: %u\n", xue_trb_tfre_cc(event));
+                break;
+            }
+            tr->deq =
+                (xue_trb_tfre_ptr(event) & XUE_TRB_RING_MASK) >> trb_shift;
+            break;
+        case xue_trb_psce:
+            reg->portsc |= (XUE_PSC_ACK_MASK & reg->portsc);
+            break;
+        default:
+            break;
+        }
+
+        er->cyc = (er->deq == XUE_TRB_RING_CAP - 1) ? er->cyc ^ 1 : er->cyc;
+        er->deq = (er->deq + 1) & (XUE_TRB_RING_CAP - 1);
+        event = &er->trb[er->deq];
+    }
+
+    erdp &= ~XUE_TRB_RING_MASK;
+    erdp |= (er->deq << trb_shift);
+    wmb();
+    reg->erdp = erdp;
+}
+
+/**
+ * xue_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 xue_init_ep(uint32_t *ep, uint64_t mbs, uint32_t type,
+                        uint64_t ring_dma)
+{
+    memset(ep, 0, XUE_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;
+}
+
+/* Initialize the DbC info with USB string descriptor addresses */
+static void xue_init_strings(struct xue *xue, uint32_t *info)
+{
+    uint64_t *sda;
+
+    /* clang-format off */
+    const char strings[] = {
+        6,  3, 9, 0, 4, 0,
+        8,  3, 'A', 0, 'I', 0, 'S', 0,
+        30, 3, 'X', 0, 'u', 0, 'e', 0, ' ', 0,
+               'D', 0, 'b', 0, 'C', 0, ' ', 0,
+               'D', 0, 'e', 0, 'v', 0, 'i', 0, 'c', 0, 'e', 0,
+        4, 3, '0', 0
+    };
+    /* clang-format on */
+
+    memcpy(xue->dbc_str, strings, sizeof(strings));
+
+    sda = (uint64_t *)&info[0];
+    sda[0] = virt_to_maddr(xue->dbc_str);
+    sda[1] = sda[0] + 6;
+    sda[2] = sda[0] + 6 + 8;
+    sda[3] = sda[0] + 6 + 8 + 30;
+    info[8] = (4 << 24) | (30 << 16) | (8 << 8) | 6;
+}
+
+static void xue_enable_dbc(struct xue *xue)
+{
+    struct xue_dbc_reg *reg = xue->dbc_reg;
+
+    wmb();
+    reg->ctrl |= (1UL << XUE_CTRL_DCE);
+    wmb();
+
+    while ( (reg->ctrl & (1UL << XUE_CTRL_DCE)) == 0 )
+        xue_sys_pause();
+
+    wmb();
+    reg->portsc |= (1UL << XUE_PSC_PED);
+    wmb();
+
+    while ( (reg->ctrl & (1UL << XUE_CTRL_DCR)) == 0 )
+        xue_sys_pause();
+}
+
+static void xue_disable_dbc(struct xue *xue)
+{
+    struct xue_dbc_reg *reg = xue->dbc_reg;
+
+    reg->portsc &= ~(1UL << XUE_PSC_PED);
+    wmb();
+    reg->ctrl &= ~(1UL << XUE_CTRL_DCE);
+
+    while ( reg->ctrl & (1UL << XUE_CTRL_DCE) )
+        xue_sys_pause();
+}
+
+static int xue_init_dbc(struct xue *xue)
+{
+    uint64_t erdp = 0;
+    uint64_t out = 0;
+    uint64_t in = 0;
+    uint64_t mbs = 0;
+    struct xue_dbc_reg *reg = xue_find_dbc(xue);
+
+    if ( !reg )
+        return 0;
+
+    xue->dbc_reg = reg;
+    xue_disable_dbc(xue);
+
+    xue_trb_ring_init(xue, &xue->dbc_ering, 0, XUE_DB_INVAL);
+    xue_trb_ring_init(xue, &xue->dbc_oring, 1, XUE_DB_OUT);
+    xue_trb_ring_init(xue, &xue->dbc_iring, 1, XUE_DB_IN);
+
+    erdp = virt_to_maddr(xue->dbc_ering.trb);
+    if ( !erdp )
+        return 0;
+
+    memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
+    xue->dbc_erst->base = erdp;
+    xue->dbc_erst->size = XUE_TRB_RING_CAP;
+
+    mbs = (reg->ctrl & 0xFF0000) >> 16;
+    out = virt_to_maddr(xue->dbc_oring.trb);
+    in = virt_to_maddr(xue->dbc_iring.trb);
+
+    memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
+    xue_init_strings(xue, xue->dbc_ctx->info);
+    xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
+    xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
+
+    reg->erstsz = 1;
+    reg->erstba = virt_to_maddr(xue->dbc_erst);
+    reg->erdp = erdp;
+    reg->cp = virt_to_maddr(xue->dbc_ctx);
+    reg->ddi1 = (XUE_DBC_VENDOR << 16) | XUE_DBC_PROTOCOL;
+    reg->ddi2 = XUE_DBC_PRODUCT;
+
+    cache_flush(xue->dbc_ctx, sizeof(*xue->dbc_ctx));
+    cache_flush(xue->dbc_erst, sizeof(*xue->dbc_erst));
+    cache_flush(xue->dbc_ering.trb, XUE_TRB_RING_BYTES);
+    cache_flush(xue->dbc_oring.trb, XUE_TRB_RING_BYTES);
+    cache_flush(xue->dbc_iring.trb, XUE_TRB_RING_BYTES);
+    cache_flush(xue->dbc_owork.buf, XUE_WORK_RING_BYTES);
+
+    return 1;
+}
+
+static void xue_init_work_ring(struct xue *xue,
+                               struct xue_work_ring *wrk)
+{
+    wrk->enq = 0;
+    wrk->deq = 0;
+    wrk->dma = virt_to_maddr(wrk->buf);
+}
+
+/* @endcond */
+
+/**
+ * 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 xue the xue to open (!= NULL)
+ * @return true iff xue_open succeeded
+ */
+static bool __init xue_open(struct xue *xue)
+{
+    if ( !xue )
+        return false;
+
+    if ( !xue_init_xhc(xue) )
+        return false;
+
+    if ( !xue_init_dbc(xue) )
+        return false;
+
+    xue_init_work_ring(xue, &xue->dbc_owork);
+    xue_enable_dbc(xue);
+    xue->open = 1;
+
+    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 xue the xue to flush
+ * @param trb the ring containing the TRBs to transfer
+ * @param wrk the work ring containing data to be flushed
+ */
+static void xue_flush(struct xue *xue, struct xue_trb_ring *trb,
+                      struct xue_work_ring *wrk)
+{
+    struct xue_dbc_reg *reg = xue->dbc_reg;
+    uint32_t db = (reg->db & 0xFFFF00FF) | (trb->db << 8);
+
+    if ( xue->open && !(reg->ctrl & (1UL << XUE_CTRL_DCE)) )
+    {
+        if ( !xue_init_dbc(xue) )
+            return;
+
+        xue_init_work_ring(xue, &xue->dbc_owork);
+        xue_enable_dbc(xue);
+    }
+
+    xue_pop_events(xue);
+
+    if ( !(reg->ctrl & (1UL << XUE_CTRL_DCR)) )
+    {
+        xue_error("DbC not configured");
+        return;
+    }
+
+    if ( reg->ctrl & (1UL << XUE_CTRL_DRC) )
+    {
+        reg->ctrl |= (1UL << XUE_CTRL_DRC);
+        reg->portsc |= (1UL << XUE_PSC_PED);
+        wmb();
+    }
+
+    if ( xue_trb_ring_full(trb) )
+        return;
+
+    if ( wrk->enq == wrk->deq )
+        return;
+    else if ( wrk->enq > wrk->deq )
+    {
+        xue_push_trb(xue, trb, wrk->dma + wrk->deq, wrk->enq - wrk->deq);
+        wrk->deq = wrk->enq;
+    }
+    else
+    {
+        xue_push_trb(xue, trb, wrk->dma + wrk->deq,
+                     XUE_WORK_RING_CAP - wrk->deq);
+        wrk->deq = 0;
+        if ( wrk->enq > 0 && !xue_trb_ring_full(trb) )
+        {
+            xue_push_trb(xue, trb, wrk->dma, wrk->enq);
+            wrk->deq = wrk->enq;
+        }
+    }
+
+    wmb();
+    reg->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 xue the xue to write to
+ * @param c the character to write
+ * @return the number of bytes written
+ */
+static int64_t xue_putc(struct xue *xue, char c)
+{
+    if ( !xue_push_work(xue, &xue->dbc_owork, &c, 1) )
+        return 0;
+
+    if ( c == '\n' )
+        xue_flush(xue, &xue->dbc_oring, &xue->dbc_owork);
+
+    return 1;
+}
+
+struct xue_uart {
+    struct xue xue;
+    struct timer timer;
+    spinlock_t *lock;
+};
+
+static struct xue_uart xue_uart;
+
+static void cf_check xue_uart_poll(void *data)
+{
+    struct serial_port *port = data;
+    struct xue_uart *uart = port->uart;
+    struct xue *xue = &uart->xue;
+    unsigned long flags = 0;
+
+    if ( spin_trylock_irqsave(&port->tx_lock, flags) )
+    {
+        xue_flush(xue, &xue->dbc_oring, &xue->dbc_owork);
+        spin_unlock_irqrestore(&port->tx_lock, flags);
+    }
+
+    serial_tx_interrupt(port, guest_cpu_user_regs());
+    set_timer(&uart->timer, NOW() + MICROSECS(XUE_POLL_INTERVAL));
+}
+
+static void __init cf_check xue_uart_init_preirq(struct serial_port *port)
+{
+    struct xue_uart *uart = port->uart;
+    uart->lock = &port->tx_lock;
+}
+
+static void __init cf_check xue_uart_init_postirq(struct serial_port *port)
+{
+    struct xue_uart *uart = port->uart;
+
+    serial_async_transmit(port);
+    init_timer(&uart->timer, xue_uart_poll, port, 0);
+    set_timer(&uart->timer, NOW() + MILLISECS(1));
+}
+
+static int cf_check xue_uart_tx_ready(struct serial_port *port)
+{
+    struct xue_uart *uart = port->uart;
+    struct xue *xue = &uart->xue;
+
+    return XUE_WORK_RING_CAP - xue_work_ring_size(&xue->dbc_owork);
+}
+
+static void cf_check xue_uart_putc(struct serial_port *port, char c)
+{
+    struct xue_uart *uart = port->uart;
+    xue_putc(&uart->xue, c);
+}
+
+static void cf_check xue_uart_flush(struct serial_port *port)
+{
+    s_time_t goal;
+    struct xue_uart *uart = port->uart;
+    struct xue *xue = &uart->xue;
+
+    xue_flush(xue, &xue->dbc_oring, &xue->dbc_owork);
+
+    goal = NOW() + MICROSECS(XUE_POLL_INTERVAL);
+    if ( uart->timer.expires > goal )
+        set_timer(&uart->timer, goal);
+}
+
+static struct uart_driver xue_uart_driver = {
+    .init_preirq = xue_uart_init_preirq,
+    .init_postirq = xue_uart_init_postirq,
+    .tx_ready = xue_uart_tx_ready,
+    .putc = xue_uart_putc,
+    .flush = xue_uart_flush,
+};
+
+static struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
+static struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
+static struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
+static struct xue_erst_segment erst __aligned(64);
+static struct xue_dbc_ctx ctx __aligned(64);
+static uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
+static char str_buf[XUE_PAGE_SIZE] __aligned(64);
+static char __initdata opt_dbgp[30];
+
+string_param("dbgp", opt_dbgp);
+
+void __init xue_uart_init(void)
+{
+    struct xue_uart *uart = &xue_uart;
+    struct xue *xue = &uart->xue;
+
+    if ( strncmp(opt_dbgp, "xue", 3) )
+        return;
+
+    memset(xue, 0, sizeof(*xue));
+
+    xue->dbc_ctx = &ctx;
+    xue->dbc_erst = &erst;
+    xue->dbc_ering.trb = evt_trb;
+    xue->dbc_oring.trb = out_trb;
+    xue->dbc_iring.trb = in_trb;
+    xue->dbc_owork.buf = wrk_buf;
+    xue->dbc_str = str_buf;
+
+    if ( xue_open(xue) )
+        serial_register_uart(SERHND_DBGP, &xue_uart_driver, &xue_uart);
+}
+
+#ifdef XUE_DEBUG
+static void xue_dump(struct xue *xue)
+{
+    struct xue_dbc_reg *r = xue->dbc_reg;
+
+    xue_debug("XUE DUMP:\n");
+    xue_debug("    ctrl: 0x%x stat: 0x%x psc: 0x%x\n", r->ctrl, r->st,
+              r->portsc);
+    xue_debug("    id: 0x%x, db: 0x%x\n", r->id, r->db);
+    xue_debug("    erstsz: %u, erstba: 0x%lx\n", r->erstsz, r->erstba);
+    xue_debug("    erdp: 0x%lx, cp: 0x%lx\n", r->erdp, r->cp);
+    xue_debug("    ddi1: 0x%x, ddi2: 0x%x\n", r->ddi1, r->ddi2);
+    xue_debug("    erstba == virt_to_dma(erst): %d\n",
+              r->erstba == virt_to_maddr(xue->dbc_erst));
+    xue_debug("    erdp == virt_to_dma(erst[0].base): %d\n",
+              r->erdp == xue->dbc_erst[0].base);
+    xue_debug("    cp == virt_to_dma(ctx): %d\n",
+              r->cp == virt_to_maddr(xue->dbc_ctx));
+}
+
+static void xue_uart_dump(void)
+{
+    struct xue_uart *uart = &xue_uart;
+    struct xue *xue = &uart->xue;
+
+    xue_dump(xue);
+}
+#endif
diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h
index 6548f0b0a9cf..a737c526a181 100644
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -171,6 +171,9 @@ struct ns16550_defaults {
 };
 void ns16550_init(int index, struct ns16550_defaults *defaults);
 void ehci_dbgp_init(void);
+#ifdef CONFIG_HAS_XHCI
+void xue_uart_init(void);
+#endif
 
 void arm_uart_init(void);
 
-- 
git-series 0.9.1

[PATCH v2 2/9] xue: reset XHCI ports when initializing dbc

[Marek Marczykowski-Górecki]

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 v2:
- use uint32_t instead of u32
- code style
---
 xen/drivers/char/xue.c | 70 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 70 insertions(+)

diff --git a/xen/drivers/char/xue.c b/xen/drivers/char/xue.c
index 234b07b563bb..2cbbaea11fa0 100644
--- a/xen/drivers/char/xue.c
+++ b/xen/drivers/char/xue.c
@@ -63,6 +63,10 @@
     ((1UL << XUE_PSC_CSC) | (1UL << XUE_PSC_PRC) | (1UL << XUE_PSC_PLC) |      \
      (1UL << XUE_PSC_CEC))
 
+#define XUE_XHC_EXT_PORT_MAJOR(x)  (((x) >> 24) & 0xff)
+#define PORT_RESET                 (1 << 4)
+#define PORT_CONNECT               (1 << 0)
+
 #define xue_debug(...) printk("xue debug: " __VA_ARGS__)
 #define xue_alert(...) printk("xue alert: " __VA_ARGS__)
 #define xue_error(...) printk("xue error: " __VA_ARGS__)
@@ -590,6 +594,68 @@ static void xue_init_strings(struct xue *xue, uint32_t *info)
     info[8] = (4 << 24) | (30 << 16) | (8 << 8) | 6;
 }
 
+static void xue_do_reset_debug_port(struct xue *xue,
+                                    unsigned int id, unsigned int count)
+{
+    uint32_t *ops_reg;
+    uint32_t *portsc;
+    uint32_t val, cap_length;
+    int i;
+
+    cap_length = (*(uint32_t*)xue->xhc_mmio) & 0xff;
+    ops_reg = xue->xhc_mmio + cap_length;
+
+    id--;
+    for ( i = id; i < (id + count); i++ )
+    {
+        portsc = ops_reg + 0x100 + i * 0x4;
+        val = *portsc;
+        if ( !(val & PORT_CONNECT) )
+            *portsc = val | PORT_RESET;
+    }
+}
+
+static void xue_reset_debug_port(struct xue *xue)
+{
+    uint32_t val, port_offset, port_count;
+    uint32_t *xcap;
+    uint32_t next;
+    uint32_t id;
+    uint8_t *mmio = (uint8_t *)xue->xhc_mmio;
+    uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
+    const uint32_t PROTOCOL_ID = 0x2;
+
+    /**
+     * Paranoid check against a zero value. The spec mandates that
+     * at least one "supported protocol" capability must be implemented,
+     * so this should always be false.
+     */
+    if ( (*hccp1 & 0xFFFF0000) == 0 )
+        return;
+
+    xcap = (uint32_t *)(mmio + (((*hccp1 & 0xFFFF0000) >> 16) << 2));
+    next = (*xcap & 0xFF00) >> 8;
+    id = *xcap & 0xFF;
+
+    /* Look for "supported protocol" capability, major revision 3 */
+    for ( ; next; xcap += next, id = *xcap & 0xFF, next = (*xcap & 0xFF00) >> 8)
+    {
+        if ( id != PROTOCOL_ID && next )
+            continue;
+
+        if ( XUE_XHC_EXT_PORT_MAJOR(*xcap) != 0x3 )
+            continue;
+
+        /* extract ports offset and count from the capability structure */
+        val = *(xcap + 2);
+        port_offset = val & 0xff;
+        port_count = (val >> 8) & 0xff;
+
+        /* and reset them all */
+        xue_do_reset_debug_port(xue, port_offset, port_count);
+    }
+}
+
 static void xue_enable_dbc(struct xue *xue)
 {
     struct xue_dbc_reg *reg = xue->dbc_reg;
@@ -601,6 +667,10 @@ static void xue_enable_dbc(struct xue *xue)
     while ( (reg->ctrl & (1UL << XUE_CTRL_DCE)) == 0 )
         xue_sys_pause();
 
+    /* reset ports on initial open, to force re-enumerating by the host */
+    if ( !xue->open )
+        xue_reset_debug_port(xue);
+
     wmb();
     reg->portsc |= (1UL << XUE_PSC_PED);
     wmb();
-- 
git-series 0.9.1

[PATCH v2 3/9] xue: add support for selecting specific xhci

[Marek Marczykowski-Górecki]

Handle parameters similar to dbgp=ehci.

Implement this by not resettting xhc_cf8 again in xue_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>
---
Changes in v2:
 - unsigned int xhc_num
 - code style
---
 docs/misc/xen-command-line.pandoc |  2 +-
 xen/drivers/char/xue.c            | 53 ++++++++++++++++++++++++--------
 2 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index f9fa857bd84e..1ab92bf7b50a 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> ]`
-> `= xue`
+> `= xue[ <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/xue.c b/xen/drivers/char/xue.c
index 2cbbaea11fa0..9d48068a5fba 100644
--- a/xen/drivers/char/xue.c
+++ b/xen/drivers/char/xue.c
@@ -205,6 +205,7 @@ struct xue {
     void *xhc_mmio;
 
     int open;
+    unsigned int xhc_num; /* look for n-th xhc */
 };
 
 static void xue_sys_pause(void)
@@ -238,24 +239,35 @@ static bool __init xue_init_xhc(struct xue *xue)
     uint64_t bar1;
     uint64_t devfn;
 
-    /*
-     * 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 ( xue->sbdf.sbdf == 0 )
     {
-        pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
-        uint32_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) == XUE_XHC_CLASSC )
+            pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
+            uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
+
+            if ( hdr == 0 || hdr == 0x80 )
             {
-                xue->sbdf = sbdf;
-                break;
+                if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == XUE_XHC_CLASSC )
+                {
+                    if ( xue->xhc_num-- )
+                        continue;
+                    xue->sbdf = sbdf;
+                    break;
+                }
             }
         }
     }
+    else
+    {
+        /* Verify if selected device is really xHC */
+        if ( (pci_conf_read32(xue->sbdf, PCI_CLASS_REVISION) >> 8) != XUE_XHC_CLASSC )
+            xue->sbdf.sbdf = 0;
+    }
 
     if ( !xue->sbdf.sbdf )
     {
@@ -955,12 +967,29 @@ void __init xue_uart_init(void)
 {
     struct xue_uart *uart = &xue_uart;
     struct xue *xue = &uart->xue;
+    const char *e;
 
     if ( strncmp(opt_dbgp, "xue", 3) )
         return;
 
     memset(xue, 0, sizeof(*xue));
 
+    if ( isdigit(opt_dbgp[3]) || !opt_dbgp[3] )
+    {
+        if ( opt_dbgp[3] )
+            xue->xhc_num = simple_strtoul(opt_dbgp + 3, &e, 10);
+    }
+    else if ( strncmp(opt_dbgp + 3, "@pci", 4) == 0 )
+    {
+        unsigned int bus, slot, func;
+
+        e = parse_pci(opt_dbgp + 7, NULL, &bus, &slot, &func);
+        if ( !e || *e )
+            return;
+
+        xue->sbdf = PCI_SBDF(0, bus, slot, func);
+    }
+
     xue->dbc_ctx = &ctx;
     xue->dbc_erst = &erst;
     xue->dbc_ering.trb = evt_trb;
-- 
git-series 0.9.1

[PATCH v2 4/9] console: support multiple serial console simultaneously

[Marek Marczykowski-Górecki]

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

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

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/char/console.c | 58 ++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 13 deletions(-)

diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
index f9937c5134c0..44b703296487 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 4
+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 */
@@ -395,9 +397,17 @@ static unsigned int serial_rx_cons, serial_rx_prod;
 
 static void (*serial_steal_fn)(const char *, size_t nr) = early_puts;
 
+/* Redirect any console output to *fn*, if *handle* is configured as a console. */
 int console_steal(int handle, void (*fn)(const char *, size_t nr))
 {
-    if ( (handle == -1) || (handle != sercon_handle) )
+    int i;
+
+    if ( handle == -1 )
+        return 0;
+    for ( i = 0; i < nr_sercon_handle; i++ )
+        if ( handle == sercon_handle[i] )
+            break;
+    if ( nr_sercon_handle && i == nr_sercon_handle )
         return 0;
 
     if ( serial_steal_fn != NULL )
@@ -415,10 +425,13 @@ void console_giveback(int id)
 
 void console_serial_puts(const char *s, size_t nr)
 {
+    int i;
+
     if ( serial_steal_fn != NULL )
         serial_steal_fn(s, nr);
     else
-        serial_puts(sercon_handle, s, nr);
+        for ( i = 0; i < nr_sercon_handle; i++ )
+            serial_puts(sercon_handle[i], s, nr);
 
     /* Copy all serial output into PV console */
     pv_console_puts(s, nr);
@@ -956,7 +969,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,7 +990,8 @@ void __init console_init_preirq(void)
             continue;
         else if ( (sh = serial_parse_handle(p)) >= 0 )
         {
-            sercon_handle = sh;
+            if ( nr_sercon_handle < MAX_SERCONS )
+                sercon_handle[nr_sercon_handle++] = sh;
             serial_steal_fn = NULL;
         }
         else
@@ -996,7 +1010,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 +1029,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 +1137,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 +1171,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 +1322,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(sercon_handle[0], suspend_steal_fn);
     serial_suspend();
     return 0;
 }
-- 
git-series 0.9.1

[PATCH v2 5/9] IOMMU: add common API for device reserved memory

[Marek Marczykowski-Górecki]

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

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

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/passthrough/iommu.c | 40 ++++++++++++++++++++++++++++++++++-
 xen/include/xen/iommu.h         | 11 +++++++++-
 2 files changed, 51 insertions(+)

diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 75df3aa8ddaa..257691ad19ef 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -651,6 +651,46 @@ 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 {
+    xen_pfn_t start;
+    xen_ulong_t nr;
+    u32 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(xen_pfn_t start, xen_ulong_t nr, u32 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..7640f40d77b5 100644
--- a/xen/include/xen/iommu.h
+++ b/xen/include/xen/iommu.h
@@ -297,6 +297,17 @@ 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(xen_pfn_t start, xen_ulong_t nr, u32 sbdf);
+/*
+ * To be called by specific IOMMU driver during initialization,
+ * to fetch ranges registered with iommu_add_extra_reserved_device_memory().
+ */
+extern int iommu_get_extra_reserved_device_memory(iommu_grdm_t *func, void *ctxt);
+
 #include <asm/iommu.h>
 
 #ifndef iommu_call
-- 
git-series 0.9.1

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

[Marek Marczykowski-Górecki]

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

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 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..661a182b08d9 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -861,111 +861,148 @@ 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,
+                                    u32 *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,
+                                    u32 *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 )
+    {
+        printk(XENLOG_ERR VTDPREFIX
+               "Invalid RMRR Range "ERMRRU_FMT"\n",
+               ERMRRU_ARG);
+        return 0;
+    }
+
+    if ( (end_pfn - base_pfn) >= MAX_USER_RMRR_PAGES )
     {
-        base = user_rmrrs[i].base_pfn;
-        end = user_rmrrs[i].end_pfn;
+        printk(XENLOG_ERR VTDPREFIX
+               "RMRR range "ERMRRU_FMT" exceeds "\
+               __stringify(MAX_USER_RMRR_PAGES)" pages\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 +1047,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;
-- 
git-series 0.9.1

[PATCH v2 7/9] IOMMU/AMD: wire common device reserved memory API

[Marek Marczykowski-Górecki]

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

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/passthrough/amd/iommu_acpi.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c
index ac6835225bae..2a4896c05442 100644
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -1078,6 +1078,20 @@ static inline bool_t is_ivmd_block(u8 type)
             type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
 }
 
+static int __init cf_check add_one_extra_ivmd(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
+{
+    struct acpi_ivrs_memory ivmd;
+
+    ivmd.start_address = start << PAGE_SHIFT;
+    ivmd.memory_length = nr << PAGE_SHIFT;
+    ivmd.header.flags = ACPI_IVMD_UNITY |
+                        ACPI_IVMD_READ | ACPI_IVMD_WRITE;
+    ivmd.header.length = sizeof(ivmd);
+    ivmd.header.device_id = id;
+    ivmd.header.type = ACPI_IVRS_TYPE_MEMORY_ONE;
+    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 +1135,8 @@ static int __init cf_check parse_ivrs_table(struct acpi_table_header *table)
         AMD_IOMMU_DEBUG("IVMD: %u command line provided entries\n", nr_ivmd);
     for ( i = 0; !error && i < nr_ivmd; ++i )
         error = parse_ivmd_block(user_ivmds + i);
+    if ( !error )
+        error = iommu_get_extra_reserved_device_memory(add_one_extra_ivmd, NULL);
 
     /* Each IO-APIC must have been mentioned in the table. */
     for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
-- 
git-series 0.9.1

[PATCH v2 8/9] xue: mark DMA buffers as reserved for the device

[Marek Marczykowski-Górecki]

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

Using this API for DbC pages requires raising MAX_USER_RMRR_PAGES.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/char/xue.c             | 43 ++++++++++++++++++++-----------
 xen/drivers/passthrough/vtd/dmar.c |  2 +-
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/xen/drivers/char/xue.c b/xen/drivers/char/xue.c
index 9d48068a5fba..a6c49bb43e97 100644
--- a/xen/drivers/char/xue.c
+++ b/xen/drivers/char/xue.c
@@ -26,6 +26,7 @@
 #include <xen/serial.h>
 #include <xen/timer.h>
 #include <xen/param.h>
+#include <xen/iommu.h>
 #include <asm/fixmap.h>
 #include <asm/io.h>
 #include <xen/mm.h>
@@ -952,13 +953,21 @@ static struct uart_driver xue_uart_driver = {
     .flush = xue_uart_flush,
 };
 
-static struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
-static struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
-static struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
-static struct xue_erst_segment erst __aligned(64);
-static struct xue_dbc_ctx ctx __aligned(64);
-static uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
-static char str_buf[XUE_PAGE_SIZE] __aligned(64);
+struct xue_dma_bufs {
+    struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
+    struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
+    struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
+    struct xue_erst_segment erst __aligned(64);
+    struct xue_dbc_ctx ctx __aligned(64);
+    uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
+    char str_buf[XUE_PAGE_SIZE] __aligned(64);
+    /*
+     * Don't place anything else on this page - it will be
+     * DMA-reachable by the USB controller.
+     */
+    char _pad[0] __aligned(XUE_PAGE_SIZE);
+};
+static struct xue_dma_bufs xue_dma_bufs __aligned(XUE_PAGE_SIZE);
 static char __initdata opt_dbgp[30];
 
 string_param("dbgp", opt_dbgp);
@@ -990,16 +999,22 @@ void __init xue_uart_init(void)
         xue->sbdf = PCI_SBDF(0, bus, slot, func);
     }
 
-    xue->dbc_ctx = &ctx;
-    xue->dbc_erst = &erst;
-    xue->dbc_ering.trb = evt_trb;
-    xue->dbc_oring.trb = out_trb;
-    xue->dbc_iring.trb = in_trb;
-    xue->dbc_owork.buf = wrk_buf;
-    xue->dbc_str = str_buf;
+    xue->dbc_ctx = &xue_dma_bufs.ctx;
+    xue->dbc_erst = &xue_dma_bufs.erst;
+    xue->dbc_ering.trb = xue_dma_bufs.evt_trb;
+    xue->dbc_oring.trb = xue_dma_bufs.out_trb;
+    xue->dbc_iring.trb = xue_dma_bufs.in_trb;
+    xue->dbc_owork.buf = xue_dma_bufs.wrk_buf;
+    xue->dbc_str = xue_dma_bufs.str_buf;
 
     if ( xue_open(xue) )
+    {
+        iommu_add_extra_reserved_device_memory(
+                PFN_DOWN(virt_to_maddr(&xue_dma_bufs)),
+                PFN_UP(sizeof(xue_dma_bufs)),
+                uart->xue.sbdf.sbdf);
         serial_register_uart(SERHND_DBGP, &xue_uart_driver, &xue_uart);
+    }
 }
 
 #ifdef XUE_DEBUG
diff --git a/xen/drivers/passthrough/vtd/dmar.c b/xen/drivers/passthrough/vtd/dmar.c
index 661a182b08d9..2caa3e9ad1b0 100644
--- a/xen/drivers/passthrough/vtd/dmar.c
+++ b/xen/drivers/passthrough/vtd/dmar.c
@@ -845,7 +845,7 @@ out:
     return ret;
 }
 
-#define MAX_USER_RMRR_PAGES 16
+#define MAX_USER_RMRR_PAGES 64
 #define MAX_USER_RMRR 10
 
 /* RMRR units derived from command line rmrr option. */
-- 
git-series 0.9.1

[PATCH v2 9/9] xue: allow driving the rest of XHCI by a domain while Xen uses DbC

[Marek Marczykowski-Górecki]

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

To avoid Linux messing with the DbC, mark this MMIO area as read-only.

Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
 xen/drivers/char/xue.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/xen/drivers/char/xue.c b/xen/drivers/char/xue.c
index a6c49bb43e97..3461e51e746a 100644
--- a/xen/drivers/char/xue.c
+++ b/xen/drivers/char/xue.c
@@ -27,6 +27,7 @@
 #include <xen/timer.h>
 #include <xen/param.h>
 #include <xen/iommu.h>
+#include <xen/rangeset.h>
 #include <asm/fixmap.h>
 #include <asm/io.h>
 #include <xen/mm.h>
@@ -807,6 +808,7 @@ static void xue_flush(struct xue *xue, struct xue_trb_ring *trb,
 {
     struct xue_dbc_reg *reg = xue->dbc_reg;
     uint32_t db = (reg->db & 0xFFFF00FF) | (trb->db << 8);
+    uint32_t cmd;
 
     if ( xue->open && !(reg->ctrl & (1UL << XUE_CTRL_DCE)) )
     {
@@ -817,6 +819,16 @@ static void xue_flush(struct xue *xue, struct xue_trb_ring *trb,
         xue_enable_dbc(xue);
     }
 
+    /* Re-enable bus mastering, if dom0 (or other) disabled it in the meantime. */
+    cmd = pci_conf_read16(xue->sbdf, PCI_COMMAND);
+#define XUE_XHCI_CMD_REQUIRED (PCI_COMMAND_MEMORY|PCI_COMMAND_MASTER)
+    if ( (cmd & XUE_XHCI_CMD_REQUIRED) != XUE_XHCI_CMD_REQUIRED )
+    {
+        cmd |= XUE_XHCI_CMD_REQUIRED;
+        pci_conf_write16(xue->sbdf, PCI_COMMAND, cmd);
+    }
+#undef XUE_XHCI_CMD_REQUIRED
+
     xue_pop_events(xue);
 
     if ( !(reg->ctrl & (1UL << XUE_CTRL_DCR)) )
@@ -916,6 +928,13 @@ static void __init cf_check xue_uart_init_postirq(struct serial_port *port)
     serial_async_transmit(port);
     init_timer(&uart->timer, xue_uart_poll, port, 0);
     set_timer(&uart->timer, NOW() + MILLISECS(1));
+
+#ifdef CONFIG_X86
+    if ( rangeset_add_range(mmio_ro_ranges,
+                PFN_DOWN(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset),
+                PFN_UP(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset + sizeof(*uart->xue.dbc_reg)) - 1) )
+        printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
+#endif
 }
 
 static int cf_check xue_uart_tx_ready(struct serial_port *port)
-- 
git-series 0.9.1

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki]

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

On Wed, Jul 06, 2022 at 05:32:06PM +0200, Marek Marczykowski-Górecki wrote:
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index e5f7b1d8eb8a..d12b2205dafc 100644
> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -74,3 +74,12 @@ config HAS_EHCI
>  	help
>  	  This selects the USB based EHCI debug port to be used as a UART. If
>  	  you have an x86 based system with USB, say Y.
> +
> +config HAS_XHCI
> +	bool "XHCI DbC UART driver"
> +	depends on X86
> +	help
> +	  This selects the USB based XHCI debug capability to be used as a UART.
> +	  Enabling this option makes Xen use extra ~2MB memory, even if XHCI UART

My math sucks here... 58 pages is 232KiB.

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

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

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> --- 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> ]`
> +> `= xue`
>  
>  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 `xue` for XHCI debug capability.
> +Xue driver will wait indefinitely for the debug host to connect - make sure the
> +cable is connected.

Especially without it being clear what "xue" stands for, I wonder
whether "xhci" would be the better (more commonly known) token to
use here.

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -946,6 +946,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      ns16550.irq     = 3;
>      ns16550_init(1, &ns16550);
>      ehci_dbgp_init();
> +#ifdef CONFIG_HAS_XHCI
> +    xue_uart_init();
> +#endif

Can you make an empty inline stub to avoid the #ifdef here?

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -74,3 +74,12 @@ config HAS_EHCI
>  	help
>  	  This selects the USB based EHCI debug port to be used as a UART. If
>  	  you have an x86 based system with USB, say Y.
> +
> +config HAS_XHCI
> +	bool "XHCI DbC UART driver"

I'm afraid I consider most of the other options here wrong in
starting with HAS_: Such named options should have no prompt, and
be exclusively engaged by "select". Hence I'd like to ask to drop
the HAS_ here.

> +	depends on X86
> +	help
> +	  This selects the USB based XHCI debug capability to be used as a UART.

s/used/usable/?

> --- /dev/null
> +++ b/xen/drivers/char/xue.c
> @@ -0,0 +1,933 @@
> +/*
> + * drivers/char/xue.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/types.h>
> +#include <asm/string.h>
> +#include <asm/system.h>
> +#include <xen/serial.h>
> +#include <xen/timer.h>
> +#include <xen/param.h>
> +#include <asm/fixmap.h>
> +#include <asm/io.h>
> +#include <xen/mm.h>

Please sort xen/ before asm/ and alphabetically within each group.

> +/* uncomment to have xue_uart_dump() debug function */
> +/* #define XUE_DEBUG 1 */
> +
> +#define XUE_POLL_INTERVAL 100 /* us */
> +
> +#define XUE_PAGE_SIZE 4096ULL

I think I had asked before - why this odd suffix?

> +static void xue_sys_pause(void)
> +{
> +    asm volatile("pause" ::: "memory");
> +}

I wonder whether the open-coded inline assembly is really needed
here: Can't you use cpu_relax()? If not, style nit: Several blanks
missing.

> +static bool __init xue_init_xhc(struct xue *xue)
> +{
> +    uint32_t bar0;
> +    uint64_t bar1;
> +    uint64_t devfn;
> +
> +    /*
> +     * 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);
> +        uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> +
> +        if ( hdr == 0 || hdr == 0x80 )
> +        {
> +            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == XUE_XHC_CLASSC )
> +            {
> +                xue->sbdf = sbdf;
> +                break;
> +            }
> +        }
> +    }
> +
> +    if ( !xue->sbdf.sbdf )
> +    {
> +        xue_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(xue->sbdf, PCI_BASE_ADDRESS_0);
> +    bar1 = pci_conf_read32(xue->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;
> +
> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
> +    xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) & 0xFFFFFFF0) + 1;
> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, bar0);

Why is a 64-bit BAR required when you size only the low 32 bits?
Also you need to disable memory decoding around this (and
somewhere you also need to explicitly enable it, assuming here
you would afterwards restore the original value of the command
register). Further you're still open-coding
PCI_BASE_ADDRESS_MEM_MASK here.

> +/**
> + * 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 xue_dbc_reg *xue_find_dbc(struct xue *xue)
> +{
> +    uint32_t *xcap;
> +    uint32_t next;
> +    uint32_t id;
> +    uint8_t *mmio = (uint8_t *)xue->xhc_mmio;
> +    uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
> +    const uint32_t DBC_ID = 0xA;
> +
> +    /**
> +     * Paranoid check against a zero value. The spec mandates that
> +     * at least one "supported protocol" capability must be implemented,
> +     * so this should always be false.
> +     */
> +    if ( (*hccp1 & 0xFFFF0000) == 0 )
> +        return NULL;
> +
> +    xcap = (uint32_t *)(mmio + (((*hccp1 & 0xFFFF0000) >> 16) << 2));

Why not either

    xcap = (uint32_t *)(mmio + ((*hccp1 >> 16) << 2));

or

    xcap = (uint32_t *)(mmio + ((*hccp1 & 0xFFFF0000) >> 14));

?

> +    next = (*xcap & 0xFF00) >> 8;
> +    id = *xcap & 0xFF;
> +
> +    /**
> +     * Table 7-1 states that 'next' is relative to
> +     * the current value of xcap and is a dword offset.
> +     */
> +    while ( id != DBC_ID && next ) {

Nit: Brace placement.

> +        xcap += next;
> +        id = *xcap & 0xFF;
> +        next = (*xcap & 0xFF00) >> 8;
> +    }

Is this loop guaranteed to terminate? See drivers/pci/pci.c where
circular chains are being dealt with in a similar situation.

> +/* Initialize the DbC info with USB string descriptor addresses */
> +static void xue_init_strings(struct xue *xue, uint32_t *info)
> +{
> +    uint64_t *sda;
> +
> +    /* clang-format off */

What's this?

> +    const char strings[] = {

static?

> +        6,  3, 9, 0, 4, 0,
> +        8,  3, 'A', 0, 'I', 0, 'S', 0,
> +        30, 3, 'X', 0, 'u', 0, 'e', 0, ' ', 0,
> +               'D', 0, 'b', 0, 'C', 0, ' ', 0,
> +               'D', 0, 'e', 0, 'v', 0, 'i', 0, 'c', 0, 'e', 0,
> +        4, 3, '0', 0
> +    };
> +    /* clang-format on */
> +
> +    memcpy(xue->dbc_str, strings, sizeof(strings));

Can't you simply assign to xue->dbc_str? I don't see this being used
elsewhere, so it might even be possible to omit the field altogether
(and with it the str_buf static variable consuming an entire page).

> +    sda = (uint64_t *)&info[0];
> +    sda[0] = virt_to_maddr(xue->dbc_str);
> +    sda[1] = sda[0] + 6;
> +    sda[2] = sda[0] + 6 + 8;
> +    sda[3] = sda[0] + 6 + 8 + 30;
> +    info[8] = (4 << 24) | (30 << 16) | (8 << 8) | 6;

Wow, magic numbers. And, apparently, some used several times.

Jan

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> 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=xue' 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:
> - 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 2MB 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>

Btw - iirc this email address has already been bouncing for me when
replying to v1 patches. Interestingly enough you did Cc the cover
letter to Connor Davis <connojdavis@gmail.com> (which I'm using in
replacement for the other address in this reply). And I can only
assume that the address did bounce for you as well when sending
both v1 and v2 ...

Jan

Re: [PATCH v2 2/9] xue: reset XHCI ports when initializing dbc

[Jan Beulich]

On 06.07.2022 17:32, 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>

Of course it would be nice to get an actual R-b from someone knowledgable
(likely going to apply to subsequent patches as well).

Jan

Re: [PATCH v2 3/9] xue: add support for selecting specific xhci

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> Handle parameters similar to dbgp=ehci.
> 
> Implement this by not resettting xhc_cf8 again in xue_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>
preferably with two adjustments:

> @@ -238,24 +239,35 @@ static bool __init xue_init_xhc(struct xue *xue)
>      uint64_t bar1;
>      uint64_t devfn;
>  
> -    /*
> -     * 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 ( xue->sbdf.sbdf == 0 )
>      {
> -        pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> -        uint32_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) == XUE_XHC_CLASSC )
> +            pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> +            uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);

Already in the original code: Why uint32_t? If anything, uint8_t, but
according to ./CODING_STYLE unsigned int might be even better.

> @@ -955,12 +967,29 @@ void __init xue_uart_init(void)
>  {
>      struct xue_uart *uart = &xue_uart;
>      struct xue *xue = &uart->xue;
> +    const char *e;
>  
>      if ( strncmp(opt_dbgp, "xue", 3) )
>          return;
>  
>      memset(xue, 0, sizeof(*xue));
>  
> +    if ( isdigit(opt_dbgp[3]) || !opt_dbgp[3] )

I don't think you need the right side here, nor ...

> +    {
> +        if ( opt_dbgp[3] )

... this inner conditional.

Jan

> +            xue->xhc_num = simple_strtoul(opt_dbgp + 3, &e, 10);
> +    }
> +    else if ( strncmp(opt_dbgp + 3, "@pci", 4) == 0 )
> +    {
> +        unsigned int bus, slot, func;
> +
> +        e = parse_pci(opt_dbgp + 7, NULL, &bus, &slot, &func);
> +        if ( !e || *e )
> +            return;
> +
> +        xue->sbdf = PCI_SBDF(0, bus, slot, func);
> +    }
> +
>      xue->dbc_ctx = &ctx;
>      xue->dbc_erst = &erst;
>      xue->dbc_ering.trb = evt_trb;

Re: [PATCH v2 4/9] console: support multiple serial console simultaneously

[Jan Beulich]

On 06.07.2022 17:32, 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 (4) is arbitrary,
> inspired by the number of SERHND_IDX values.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  xen/drivers/char/console.c | 58 ++++++++++++++++++++++++++++++---------
>  1 file changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> index f9937c5134c0..44b703296487 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 4

Might this want to be a Kconfig setting?

> +static int __read_mostly sercon_handle[MAX_SERCONS];
> +static int __read_mostly nr_sercon_handle = 0;

I would have wanted to ask for __ro_after_init here, but Arm still
hasn't enabled support for it.

> @@ -395,9 +397,17 @@ static unsigned int serial_rx_cons, serial_rx_prod;
>  
>  static void (*serial_steal_fn)(const char *, size_t nr) = early_puts;
>  
> +/* Redirect any console output to *fn*, if *handle* is configured as a console. */
>  int console_steal(int handle, void (*fn)(const char *, size_t nr))
>  {
> -    if ( (handle == -1) || (handle != sercon_handle) )
> +    int i;

unsigned int please (also elsewhere).

> +    if ( handle == -1 )
> +        return 0;
> +    for ( i = 0; i < nr_sercon_handle; i++ )
> +        if ( handle == sercon_handle[i] )
> +            break;
> +    if ( nr_sercon_handle && i == nr_sercon_handle )
>          return 0;

No need for the left side of the &&, I think. I guess it's actively
wrong to be there.

>      if ( serial_steal_fn != NULL )

I guess you then also want to make serial_steal_fn an array, and no
longer return constant 1 from the function.

> @@ -977,7 +990,8 @@ void __init console_init_preirq(void)
>              continue;
>          else if ( (sh = serial_parse_handle(p)) >= 0 )
>          {
> -            sercon_handle = sh;
> +            if ( nr_sercon_handle < MAX_SERCONS )
> +                sercon_handle[nr_sercon_handle++] = sh;

else <log a message>

> @@ -1291,7 +1322,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(sercon_handle[0], suspend_steal_fn);
>      serial_suspend();
>      return 0;
>  }

The commit message gives no explanation why only the first handle
would want/need dealing with here.

One overall remark: Especially with sync_console latency is going to
be yet worse with all output being done sequentially. The help text
for "console=" will want to mention this, up and until this would be
parallelized.

Jan

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> +struct xue {
> +    struct xue_dbc_reg *dbc_reg;
> +    struct xue_dbc_ctx *dbc_ctx;
> +    struct xue_erst_segment *dbc_erst;
> +    struct xue_trb_ring dbc_ering;
> +    struct xue_trb_ring dbc_oring;
> +    struct xue_trb_ring dbc_iring;
> +    struct xue_work_ring dbc_owork;
> +    char *dbc_str;
> +
> +    pci_sbdf_t sbdf;
> +    uint64_t xhc_mmio_phys;
> +    uint64_t xhc_mmio_size;
> +    uint64_t xhc_dbc_offset;

One more observation: None of these four field look to be needed.
They're all used only in a single function, so could be local
variables there (and xhc_dbc_offset is only ever written, so
could be dropped altogether).

Jan

Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory

[Jan Beulich]

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

I take it that I'll understand the purpose of this when making it to
later patches.

> --- a/xen/drivers/passthrough/iommu.c
> +++ b/xen/drivers/passthrough/iommu.c
> @@ -651,6 +651,46 @@ 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 {
> +    xen_pfn_t start;

Why not unsigned long?

> +    xen_ulong_t nr;

Why not unsigned int or unsigned long?

> +    u32 sbdf;

uint32_t please.

All these type related remarks apply elsewhere as well; others
below also apply wherever else applicable.

> +};
> +static unsigned int __initdata nr_extra_reserved_ranges;
> +static struct extra_reserved_range __initdata extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];

Overly long line.

> +int iommu_add_extra_reserved_device_memory(xen_pfn_t start, xen_ulong_t nr, u32 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;
> +}

Blank line please before final return statement.

Jan

Re: [PATCH v2 7/9] IOMMU/AMD: wire common device reserved memory API

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -1078,6 +1078,20 @@ static inline bool_t is_ivmd_block(u8 type)
>              type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
>  }
>  
> +static int __init cf_check add_one_extra_ivmd(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
> +{
> +    struct acpi_ivrs_memory ivmd;
> +
> +    ivmd.start_address = start << PAGE_SHIFT;
> +    ivmd.memory_length = nr << PAGE_SHIFT;

Aren't these at risk of truncation on 32-bit architectures? We have
suitable wrappers for such conversions, avoiding such issues.

> +    ivmd.header.flags = ACPI_IVMD_UNITY |
> +                        ACPI_IVMD_READ | ACPI_IVMD_WRITE;
> +    ivmd.header.length = sizeof(ivmd);
> +    ivmd.header.device_id = id;
> +    ivmd.header.type = ACPI_IVRS_TYPE_MEMORY_ONE;

Please make these the variable's initializer.

Jan

Re: [PATCH v2 8/9] xue: mark DMA buffers as reserved for the device

[Jan Beulich]

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

I don't think this device should be allowed to be assigned to any
DomU. Just like the EHCI driver, at some point in the series you
will want to call pci_hide_device() (you may already do, and I may
merely have missed it).

> Using this API for DbC pages requires raising MAX_USER_RMRR_PAGES.

I disagree here: This is merely an effect of you re-using the pre-
existing functionality there with too little customization. I think
the respective check shouldn't be applied for internal uses.

> @@ -952,13 +953,21 @@ static struct uart_driver xue_uart_driver = {
>      .flush = xue_uart_flush,
>  };
>  
> -static struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> -static struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> -static struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> -static struct xue_erst_segment erst __aligned(64);
> -static struct xue_dbc_ctx ctx __aligned(64);
> -static uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
> -static char str_buf[XUE_PAGE_SIZE] __aligned(64);
> +struct xue_dma_bufs {
> +    struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +    struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +    struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +    struct xue_erst_segment erst __aligned(64);
> +    struct xue_dbc_ctx ctx __aligned(64);
> +    uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +    char str_buf[XUE_PAGE_SIZE] __aligned(64);

Please arrange data such that the amount of unused (padding) space
is minimal. If I'm not mistaken the page-size-aligned fields are
also an even multiple of pages in size, in which case they all want
to go ahead of the 64-byte aligned but sub-page-size fields (as
per an earlier comment str_buf[] likely can go away anyway, being
the one field which is a page in size but having smaller alignment).

> +    /*
> +     * Don't place anything else on this page - it will be
> +     * DMA-reachable by the USB controller.
> +     */
> +    char _pad[0] __aligned(XUE_PAGE_SIZE);

I don't think this is needed, due to sizeof() being required to be
a multiple of alignof().

> +};
> +static struct xue_dma_bufs xue_dma_bufs __aligned(XUE_PAGE_SIZE);

I don't think the alignment here is needed, as the struct will
already have suitable alignment (derived from the biggest field
alignment value). Instead please consider putting this in
.bss.page_aligned.

> @@ -990,16 +999,22 @@ void __init xue_uart_init(void)
>          xue->sbdf = PCI_SBDF(0, bus, slot, func);
>      }
>  
> -    xue->dbc_ctx = &ctx;
> -    xue->dbc_erst = &erst;
> -    xue->dbc_ering.trb = evt_trb;
> -    xue->dbc_oring.trb = out_trb;
> -    xue->dbc_iring.trb = in_trb;
> -    xue->dbc_owork.buf = wrk_buf;
> -    xue->dbc_str = str_buf;
> +    xue->dbc_ctx = &xue_dma_bufs.ctx;
> +    xue->dbc_erst = &xue_dma_bufs.erst;
> +    xue->dbc_ering.trb = xue_dma_bufs.evt_trb;
> +    xue->dbc_oring.trb = xue_dma_bufs.out_trb;
> +    xue->dbc_iring.trb = xue_dma_bufs.in_trb;
> +    xue->dbc_owork.buf = xue_dma_bufs.wrk_buf;
> +    xue->dbc_str = xue_dma_bufs.str_buf;
>  
>      if ( xue_open(xue) )
> +    {
> +        iommu_add_extra_reserved_device_memory(
> +                PFN_DOWN(virt_to_maddr(&xue_dma_bufs)),

virt_to_pfn()?

Jan

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> +static int xue_init_dbc(struct xue *xue)
> +{
> +    uint64_t erdp = 0;
> +    uint64_t out = 0;
> +    uint64_t in = 0;
> +    uint64_t mbs = 0;
> +    struct xue_dbc_reg *reg = xue_find_dbc(xue);
> +
> +    if ( !reg )
> +        return 0;
> +
> +    xue->dbc_reg = reg;
> +    xue_disable_dbc(xue);
> +
> +    xue_trb_ring_init(xue, &xue->dbc_ering, 0, XUE_DB_INVAL);
> +    xue_trb_ring_init(xue, &xue->dbc_oring, 1, XUE_DB_OUT);
> +    xue_trb_ring_init(xue, &xue->dbc_iring, 1, XUE_DB_IN);
> +
> +    erdp = virt_to_maddr(xue->dbc_ering.trb);
> +    if ( !erdp )
> +        return 0;
> +
> +    memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> +    xue->dbc_erst->base = erdp;
> +    xue->dbc_erst->size = XUE_TRB_RING_CAP;
> +
> +    mbs = (reg->ctrl & 0xFF0000) >> 16;
> +    out = virt_to_maddr(xue->dbc_oring.trb);
> +    in = virt_to_maddr(xue->dbc_iring.trb);
> +
> +    memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> +    xue_init_strings(xue, xue->dbc_ctx->info);
> +    xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> +    xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> +
> +    reg->erstsz = 1;
> +    reg->erstba = virt_to_maddr(xue->dbc_erst);
> +    reg->erdp = erdp;
> +    reg->cp = virt_to_maddr(xue->dbc_ctx);

The only place this field is read looks to be xue_dump().

> +static struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +static struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +static struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +static struct xue_erst_segment erst __aligned(64);
> +static struct xue_dbc_ctx ctx __aligned(64);
> +static uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
> +static char str_buf[XUE_PAGE_SIZE] __aligned(64);

While I think I can see the point of the page-size alignment, can you
please clarify the need for the three instances of 64-byte alignment?

Jan

Re: [PATCH v2 9/9] xue: allow driving the rest of XHCI by a domain while Xen uses DbC

[Jan Beulich]

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

Isn't there a risk that intermediately data was lost?

> To avoid Linux messing with the DbC, mark this MMIO area as read-only.

In principle this would want to happen quite a bit earlier in the
series. I'm okay with it being kept here as long as it is made
very obvious to and easily noticeable by committers that this series
should only be committed all in one go.

Also along with this is where I'd see the pci_hide_device() go.

> @@ -817,6 +819,16 @@ static void xue_flush(struct xue *xue, struct xue_trb_ring *trb,
>          xue_enable_dbc(xue);
>      }
>  
> +    /* Re-enable bus mastering, if dom0 (or other) disabled it in the meantime. */
> +    cmd = pci_conf_read16(xue->sbdf, PCI_COMMAND);
> +#define XUE_XHCI_CMD_REQUIRED (PCI_COMMAND_MEMORY|PCI_COMMAND_MASTER)
> +    if ( (cmd & XUE_XHCI_CMD_REQUIRED) != XUE_XHCI_CMD_REQUIRED )
> +    {
> +        cmd |= XUE_XHCI_CMD_REQUIRED;
> +        pci_conf_write16(xue->sbdf, PCI_COMMAND, cmd);
> +    }
> +#undef XUE_XHCI_CMD_REQUIRED
> +
>      xue_pop_events(xue);
>  
>      if ( !(reg->ctrl & (1UL << XUE_CTRL_DCR)) )
> @@ -916,6 +928,13 @@ static void __init cf_check xue_uart_init_postirq(struct serial_port *port)
>      serial_async_transmit(port);
>      init_timer(&uart->timer, xue_uart_poll, port, 0);
>      set_timer(&uart->timer, NOW() + MILLISECS(1));
> +
> +#ifdef CONFIG_X86
> +    if ( rangeset_add_range(mmio_ro_ranges,
> +                PFN_DOWN(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset),
> +                PFN_UP(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset + sizeof(*uart->xue.dbc_reg)) - 1) )
> +        printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n");
> +#endif
>  }

According to my counting there are three overly long lines in these
two hunks.

Jan

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 08:05:28AM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +struct xue {
> > +    struct xue_dbc_reg *dbc_reg;
> > +    struct xue_dbc_ctx *dbc_ctx;
> > +    struct xue_erst_segment *dbc_erst;
> > +    struct xue_trb_ring dbc_ering;
> > +    struct xue_trb_ring dbc_oring;
> > +    struct xue_trb_ring dbc_iring;
> > +    struct xue_work_ring dbc_owork;
> > +    char *dbc_str;
> > +
> > +    pci_sbdf_t sbdf;
> > +    uint64_t xhc_mmio_phys;
> > +    uint64_t xhc_mmio_size;
> > +    uint64_t xhc_dbc_offset;
> 
> One more observation: None of these four field look to be needed.
> They're all used only in a single function, so could be local
> variables there (and xhc_dbc_offset is only ever written, so
> could be dropped altogether).

While xhc_mmio_size indeed isn't used outside of this function,
xhc_mmio_phys and xhc_dbc_offset are in later patches.

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

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

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +static int xue_init_dbc(struct xue *xue)
> > +{
> > +    uint64_t erdp = 0;
> > +    uint64_t out = 0;
> > +    uint64_t in = 0;
> > +    uint64_t mbs = 0;
> > +    struct xue_dbc_reg *reg = xue_find_dbc(xue);
> > +
> > +    if ( !reg )
> > +        return 0;
> > +
> > +    xue->dbc_reg = reg;
> > +    xue_disable_dbc(xue);
> > +
> > +    xue_trb_ring_init(xue, &xue->dbc_ering, 0, XUE_DB_INVAL);
> > +    xue_trb_ring_init(xue, &xue->dbc_oring, 1, XUE_DB_OUT);
> > +    xue_trb_ring_init(xue, &xue->dbc_iring, 1, XUE_DB_IN);
> > +
> > +    erdp = virt_to_maddr(xue->dbc_ering.trb);
> > +    if ( !erdp )
> > +        return 0;
> > +
> > +    memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> > +    xue->dbc_erst->base = erdp;
> > +    xue->dbc_erst->size = XUE_TRB_RING_CAP;
> > +
> > +    mbs = (reg->ctrl & 0xFF0000) >> 16;
> > +    out = virt_to_maddr(xue->dbc_oring.trb);
> > +    in = virt_to_maddr(xue->dbc_iring.trb);
> > +
> > +    memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> > +    xue_init_strings(xue, xue->dbc_ctx->info);
> > +    xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> > +    xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> > +
> > +    reg->erstsz = 1;
> > +    reg->erstba = virt_to_maddr(xue->dbc_erst);
> > +    reg->erdp = erdp;
> > +    reg->cp = virt_to_maddr(xue->dbc_ctx);
> 
> The only place this field is read looks to be xue_dump().
> 
> > +static struct xue_trb evt_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static struct xue_trb out_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static struct xue_trb in_trb[XUE_TRB_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static struct xue_erst_segment erst __aligned(64);
> > +static struct xue_dbc_ctx ctx __aligned(64);
> > +static uint8_t wrk_buf[XUE_WORK_RING_CAP] __aligned(XUE_PAGE_SIZE);
> > +static char str_buf[XUE_PAGE_SIZE] __aligned(64);
> 
> While I think I can see the point of the page-size alignment, can you
> please clarify the need for the three instances of 64-byte alignment?

(Guessing why original author of this code did it this way) At least
ERSTBA (5.5.2.3.2 section of the spec) is required to be 64-byte aligned
by the xHCI spec. Interestingly the DbC version of this register
(DCERSTBA, section 7.6.8.3.2) requires just 16-byte alignment.
ctx seems to require just 16-byte alignment too, and str_buf (in
practice) requires just 2-byte alignment.
I'll try to reduce those alignments and see if that still works...

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

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

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki]

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

On Tue, Jul 12, 2022 at 05:59:51PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > --- 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> ]`
> > +> `= xue`
> >  
> >  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 `xue` for XHCI debug capability.
> > +Xue driver will wait indefinitely for the debug host to connect - make sure the
> > +cable is connected.
> 
> Especially without it being clear what "xue" stands for, I wonder
> whether "xhci" would be the better (more commonly known) token to
> use here.

Sure, I can change that. I modify this code heavily anyway, so there is
little point in keeping it similar to the original xue driver.

> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -946,6 +946,9 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> >      ns16550.irq     = 3;
> >      ns16550_init(1, &ns16550);
> >      ehci_dbgp_init();
> > +#ifdef CONFIG_HAS_XHCI
> > +    xue_uart_init();
> > +#endif
> 
> Can you make an empty inline stub to avoid the #ifdef here?

Ok.

> > --- a/xen/drivers/char/Kconfig
> > +++ b/xen/drivers/char/Kconfig
> > @@ -74,3 +74,12 @@ config HAS_EHCI
> >  	help
> >  	  This selects the USB based EHCI debug port to be used as a UART. If
> >  	  you have an x86 based system with USB, say Y.
> > +
> > +config HAS_XHCI
> > +	bool "XHCI DbC UART driver"
> 
> I'm afraid I consider most of the other options here wrong in
> starting with HAS_: Such named options should have no prompt, and
> be exclusively engaged by "select". Hence I'd like to ask to drop
> the HAS_ here.

Ok.

> > +	depends on X86
> > +	help
> > +	  This selects the USB based XHCI debug capability to be used as a UART.
> 
> s/used/usable/?

Yes.

> > --- /dev/null
> > +++ b/xen/drivers/char/xue.c
> > @@ -0,0 +1,933 @@
> > +/*
> > + * drivers/char/xue.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/types.h>
> > +#include <asm/string.h>
> > +#include <asm/system.h>
> > +#include <xen/serial.h>
> > +#include <xen/timer.h>
> > +#include <xen/param.h>
> > +#include <asm/fixmap.h>
> > +#include <asm/io.h>
> > +#include <xen/mm.h>
> 
> Please sort xen/ before asm/ and alphabetically within each group.

Ok.

> > +/* uncomment to have xue_uart_dump() debug function */
> > +/* #define XUE_DEBUG 1 */
> > +
> > +#define XUE_POLL_INTERVAL 100 /* us */
> > +
> > +#define XUE_PAGE_SIZE 4096ULL
> 
> I think I had asked before - why this odd suffix?
> 
> > +static void xue_sys_pause(void)
> > +{
> > +    asm volatile("pause" ::: "memory");
> > +}
> 
> I wonder whether the open-coded inline assembly is really needed
> here: Can't you use cpu_relax()? If not, style nit: Several blanks
> missing.

Probably I can.

> > +static bool __init xue_init_xhc(struct xue *xue)
> > +{
> > +    uint32_t bar0;
> > +    uint64_t bar1;
> > +    uint64_t devfn;
> > +
> > +    /*
> > +     * 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);
> > +        uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> > +
> > +        if ( hdr == 0 || hdr == 0x80 )
> > +        {
> > +            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == XUE_XHC_CLASSC )
> > +            {
> > +                xue->sbdf = sbdf;
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    if ( !xue->sbdf.sbdf )
> > +    {
> > +        xue_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(xue->sbdf, PCI_BASE_ADDRESS_0);
> > +    bar1 = pci_conf_read32(xue->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;
> > +
> > +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
> > +    xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) & 0xFFFFFFF0) + 1;
> > +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, bar0);
> 
> Why is a 64-bit BAR required when you size only the low 32 bits?

xHCI spec says the first BAR is required to be 64-bit, so I'm checking
this assumption to handle just this one case. But then, the size is 64K
in practice (and xue_sys_map_xhc() checks for that), so just 32 bits are
enough. Anyway, I can add sizing the whole thing, for consistency.

> Also you need to disable memory decoding around this (and
> somewhere you also need to explicitly enable it, assuming here
> you would afterwards restore the original value of the command
> register). 

Actually, this is a good place to enable memory decoding.

> Further you're still open-coding
> PCI_BASE_ADDRESS_MEM_MASK here.
> 
> > +/**
> > + * 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 xue_dbc_reg *xue_find_dbc(struct xue *xue)
> > +{
> > +    uint32_t *xcap;
> > +    uint32_t next;
> > +    uint32_t id;
> > +    uint8_t *mmio = (uint8_t *)xue->xhc_mmio;
> > +    uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
> > +    const uint32_t DBC_ID = 0xA;
> > +
> > +    /**
> > +     * Paranoid check against a zero value. The spec mandates that
> > +     * at least one "supported protocol" capability must be implemented,
> > +     * so this should always be false.
> > +     */
> > +    if ( (*hccp1 & 0xFFFF0000) == 0 )
> > +        return NULL;
> > +
> > +    xcap = (uint32_t *)(mmio + (((*hccp1 & 0xFFFF0000) >> 16) << 2));
> 
> Why not either
> 
>     xcap = (uint32_t *)(mmio + ((*hccp1 >> 16) << 2));
> 
> or
> 
>     xcap = (uint32_t *)(mmio + ((*hccp1 & 0xFFFF0000) >> 14));
> 
> ?

Ok.

> > +    next = (*xcap & 0xFF00) >> 8;
> > +    id = *xcap & 0xFF;
> > +
> > +    /**
> > +     * Table 7-1 states that 'next' is relative to
> > +     * the current value of xcap and is a dword offset.
> > +     */
> > +    while ( id != DBC_ID && next ) {
> 
> Nit: Brace placement.
> 
> > +        xcap += next;
> > +        id = *xcap & 0xFF;
> > +        next = (*xcap & 0xFF00) >> 8;
> > +    }
> 
> Is this loop guaranteed to terminate? See drivers/pci/pci.c where
> circular chains are being dealt with in a similar situation.

Proper device shouldn't have circular chains here, but yes, adding
protection against this is a good idea.

> > +/* Initialize the DbC info with USB string descriptor addresses */
> > +static void xue_init_strings(struct xue *xue, uint32_t *info)
> > +{
> > +    uint64_t *sda;
> > +
> > +    /* clang-format off */
> 
> What's this?
> 
> > +    const char strings[] = {
> 
> static?
> 
> > +        6,  3, 9, 0, 4, 0,
> > +        8,  3, 'A', 0, 'I', 0, 'S', 0,
> > +        30, 3, 'X', 0, 'u', 0, 'e', 0, ' ', 0,
> > +               'D', 0, 'b', 0, 'C', 0, ' ', 0,
> > +               'D', 0, 'e', 0, 'v', 0, 'i', 0, 'c', 0, 'e', 0,
> > +        4, 3, '0', 0
> > +    };
> > +    /* clang-format on */
> > +
> > +    memcpy(xue->dbc_str, strings, sizeof(strings));
> 
> Can't you simply assign to xue->dbc_str? I don't see this being used
> elsewhere, so it might even be possible to omit the field altogether
> (and with it the str_buf static variable consuming an entire page).

That is an option, but honestly (as you note below), there is a bit too
much magic here.

> > +    sda = (uint64_t *)&info[0];
> > +    sda[0] = virt_to_maddr(xue->dbc_str);
> > +    sda[1] = sda[0] + 6;
> > +    sda[2] = sda[0] + 6 + 8;
> > +    sda[3] = sda[0] + 6 + 8 + 30;
> > +    info[8] = (4 << 24) | (30 << 16) | (8 << 8) | 6;
> 
> Wow, magic numbers. And, apparently, some used several times.

I think I can make this whole string table init a bit clearer (at a
negligible higher runtime cost).

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

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

Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > --- a/xen/drivers/passthrough/iommu.c
> > +++ b/xen/drivers/passthrough/iommu.c
> > @@ -651,6 +651,46 @@ 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 {
> > +    xen_pfn_t start;
> 
> Why not unsigned long?

Isn't this the correct type for the page number?

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

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

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich]

On 18.07.2022 12:45, Marek Marczykowski-Górecki wrote:
> On Tue, Jul 12, 2022 at 05:59:51PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +static bool __init xue_init_xhc(struct xue *xue)
>>> +{
>>> +    uint32_t bar0;
>>> +    uint64_t bar1;
>>> +    uint64_t devfn;
>>> +
>>> +    /*
>>> +     * 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);
>>> +        uint32_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
>>> +
>>> +        if ( hdr == 0 || hdr == 0x80 )
>>> +        {
>>> +            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == XUE_XHC_CLASSC )
>>> +            {
>>> +                xue->sbdf = sbdf;
>>> +                break;
>>> +            }
>>> +        }
>>> +    }
>>> +
>>> +    if ( !xue->sbdf.sbdf )
>>> +    {
>>> +        xue_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(xue->sbdf, PCI_BASE_ADDRESS_0);
>>> +    bar1 = pci_conf_read32(xue->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;
>>> +
>>> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
>>> +    xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) & 0xFFFFFFF0) + 1;
>>> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, bar0);
>>
>> Why is a 64-bit BAR required when you size only the low 32 bits?
> 
> xHCI spec says the first BAR is required to be 64-bit, so I'm checking
> this assumption to handle just this one case. But then, the size is 64K
> in practice (and xue_sys_map_xhc() checks for that), so just 32 bits are
> enough. Anyway, I can add sizing the whole thing, for consistency.
> 
>> Also you need to disable memory decoding around this (and
>> somewhere you also need to explicitly enable it, assuming here
>> you would afterwards restore the original value of the command
>> register). 
> 
> Actually, this is a good place to enable memory decoding.

It might seem so, I agree, but then upon encountering a later error
you'll need more precautions so you would able to restore the command
register to its original value. I think it's easier / clearer when
you keep command register save/restore to within functions.

Jan

Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +};
> > +static unsigned int __initdata nr_extra_reserved_ranges;
> > +static struct extra_reserved_range __initdata extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
> 
> Overly long line.

I can't find what coding style dictate in such case. Should the second
line be indented (and how much)?

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

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

Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory

[Jan Beulich]

On 18.07.2022 12:53, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/drivers/passthrough/iommu.c
>>> +++ b/xen/drivers/passthrough/iommu.c
>>> @@ -651,6 +651,46 @@ 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 {
>>> +    xen_pfn_t start;
>>
>> Why not unsigned long?
> 
> Isn't this the correct type for the page number?

In the public interface - yes. But internally we (almost) universally
use unsigned long. xen_pfn_t is inefficient to use on Arm32 (and then
presumably also other future 32-bit counterparts of 64-bit
architectures, inheriting the choices made for Arm and differing from
what we have on x86).

Jan

Re: [PATCH v2 5/9] IOMMU: add common API for device reserved memory

[Jan Beulich]

On 18.07.2022 13:03, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 12:17:53PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +};
>>> +static unsigned int __initdata nr_extra_reserved_ranges;
>>> +static struct extra_reserved_range __initdata extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];
>>
>> Overly long line.
> 
> I can't find what coding style dictate in such case. Should the second
> line be indented (and how much)?

I'd recommend

static struct extra_reserved_range __initdata
   extra_reserved_ranges[MAX_EXTRA_RESERVED_RANGES];

Jan

Re: [PATCH v2 7/9] IOMMU/AMD: wire common device reserved memory API

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 12:22:36PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> > +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> > @@ -1078,6 +1078,20 @@ static inline bool_t is_ivmd_block(u8 type)
> >              type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
> >  }
> >  
> > +static int __init cf_check add_one_extra_ivmd(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
> > +{
> > +    struct acpi_ivrs_memory ivmd;
> > +
> > +    ivmd.start_address = start << PAGE_SHIFT;
> > +    ivmd.memory_length = nr << PAGE_SHIFT;
> 
> Aren't these at risk of truncation on 32-bit architectures? We have
> suitable wrappers for such conversions, avoiding such issues.

Well, this (and the vtd equivalent) is x86-only, so it's always 64-bit.
Anyway, what are those wrappers?

> > +    ivmd.header.flags = ACPI_IVMD_UNITY |
> > +                        ACPI_IVMD_READ | ACPI_IVMD_WRITE;
> > +    ivmd.header.length = sizeof(ivmd);
> > +    ivmd.header.device_id = id;
> > +    ivmd.header.type = ACPI_IVRS_TYPE_MEMORY_ONE;
> 
> Please make these the variable's initializer.
> 
> Jan

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

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

Re: [PATCH v2 7/9] IOMMU/AMD: wire common device reserved memory API

[Jan Beulich]

On 18.07.2022 13:35, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 12:22:36PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>> @@ -1078,6 +1078,20 @@ static inline bool_t is_ivmd_block(u8 type)
>>>              type == ACPI_IVRS_TYPE_MEMORY_IOMMU);
>>>  }
>>>  
>>> +static int __init cf_check add_one_extra_ivmd(xen_pfn_t start, xen_ulong_t nr, u32 id, void *ctxt)
>>> +{
>>> +    struct acpi_ivrs_memory ivmd;
>>> +
>>> +    ivmd.start_address = start << PAGE_SHIFT;
>>> +    ivmd.memory_length = nr << PAGE_SHIFT;
>>
>> Aren't these at risk of truncation on 32-bit architectures? We have
>> suitable wrappers for such conversions, avoiding such issues.
> 
> Well, this (and the vtd equivalent) is x86-only, so it's always 64-bit.

Nevertheless writing things cleanly everywhere will reduce the risk of
somebody cloning this code on the assumption that it's correct.

> Anyway, what are those wrappers?

pfn_to_paddr()

Jan

Re: [PATCH v2 4/9] console: support multiple serial console simultaneously

[Marek Marczykowski-Górecki]

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

On Wed, Jul 13, 2022 at 11:39:07AM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, 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 (4) is arbitrary,
> > inspired by the number of SERHND_IDX values.
> > 
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> > ---
> >  xen/drivers/char/console.c | 58 ++++++++++++++++++++++++++++++---------
> >  1 file changed, 45 insertions(+), 13 deletions(-)
> > 
> > diff --git a/xen/drivers/char/console.c b/xen/drivers/char/console.c
> > index f9937c5134c0..44b703296487 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 4
> 
> Might this want to be a Kconfig setting?

Is that going to be useful for anybody (who isn't modifying the code
anyway, for example to add yet another console driver)?

> > +static int __read_mostly sercon_handle[MAX_SERCONS];
> > +static int __read_mostly nr_sercon_handle = 0;
> 
> I would have wanted to ask for __ro_after_init here, but Arm still
> hasn't enabled support for it.
> 
> > @@ -395,9 +397,17 @@ static unsigned int serial_rx_cons, serial_rx_prod;
> >  
> >  static void (*serial_steal_fn)(const char *, size_t nr) = early_puts;
> >  
> > +/* Redirect any console output to *fn*, if *handle* is configured as a console. */
> >  int console_steal(int handle, void (*fn)(const char *, size_t nr))
> >  {
> > -    if ( (handle == -1) || (handle != sercon_handle) )
> > +    int i;
> 
> unsigned int please (also elsewhere).
> 
> > +    if ( handle == -1 )
> > +        return 0;
> > +    for ( i = 0; i < nr_sercon_handle; i++ )
> > +        if ( handle == sercon_handle[i] )
> > +            break;
> > +    if ( nr_sercon_handle && i == nr_sercon_handle )
> >          return 0;
> 
> No need for the left side of the &&, I think. I guess it's actively
> wrong to be there.
> 
> >      if ( serial_steal_fn != NULL )
> 
> I guess you then also want to make serial_steal_fn an array, and no
> longer return constant 1 from the function.
> 
> > @@ -977,7 +990,8 @@ void __init console_init_preirq(void)
> >              continue;
> >          else if ( (sh = serial_parse_handle(p)) >= 0 )
> >          {
> > -            sercon_handle = sh;
> > +            if ( nr_sercon_handle < MAX_SERCONS )
> > +                sercon_handle[nr_sercon_handle++] = sh;
> 
> else <log a message>
> 
> > @@ -1291,7 +1322,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(sercon_handle[0], suspend_steal_fn);
> >      serial_suspend();
> >      return 0;
> >  }
> 
> The commit message gives no explanation why only the first handle
> would want/need dealing with here.

Sure, I can add an explanation. I'm adding this comment to console_steal():
/* Redirect any console output to *fn*, if *handle* is configured as a console. */

So, calling console_steal() is about all serial consoles, not just a
specific one. The use case for this "if" part is gdbstub, which wants
to redirect serial output only if that serial was configured as both
console and gdb. Having proper per-console stealing is doable, but IMO
not worth it (it would require also avoiding duplicated output in case
of multiple serial consoles, and probably few more corner cases).

> One overall remark: Especially with sync_console latency is going to
> be yet worse with all output being done sequentially. The help text
> for "console=" will want to mention this, up and until this would be
> parallelized.

I don't think it was parallelized anywhere. All the relevant functions
(__putstr especially) write to various console types sequentially. The
difference is that previously only the last "serial" console was used,
all the other were silently ignored. So, it was "parallel" with all
_zero other_ serial consoles, but not other console types.
Anyway, both help text and boot warning for sync_console already warn
about it. Do you want me to include relation to number of configured
console explicitly?

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

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

Re: [PATCH v2 9/9] xue: allow driving the rest of XHCI by a domain while Xen uses DbC

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 02:06:07PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > That's possible, because the capability was designed specifically to
> > allow separate driver handle it, in parallel to unmodified xhci driver
> > (separate set of registers, pretending the port is "disconnected" for
> > the main xhci driver etc). It works with Linux dom0, although requires
> > an awful hack - re-enabling bus mastering behind dom0's backs.
> > Linux driver does similar thing - see
> > drivers/usb/early/xhci-dbc.c:xdbc_handle_events().
> 
> Isn't there a risk that intermediately data was lost?

Yes, there is such risk (although minimal in practice - it happens just
once during dom0 boot). You can avoid it by instructing dom0 to not use
that USB controller.
Having this capability is really helpful (compared with the alternative
of using the whole USB controller by either Xen or Linux), as many
(most) systems have only a single USB controller.

> > To avoid Linux messing with the DbC, mark this MMIO area as read-only.
> 
> In principle this would want to happen quite a bit earlier in the
> series. I'm okay with it being kept here as long as it is made
> very obvious to and easily noticeable by committers that this series
> should only be committed all in one go.
> 
> Also along with this is where I'd see the pci_hide_device() go.

Earlier version of the patch set had pci_ro_device() before this patch.
I can add pci_ro_device() in the initial patch, and drop it in this one.

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

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

Re: [PATCH v2 8/9] xue: mark DMA buffers as reserved for the device

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 01:51:06PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > The important part is to include those buffers in IOMMU page table
> > relevant for the USB controller. Otherwise, DbC will stop working as
> > soon as IOMMU is enabled, regardless of to which domain device assigned
> > (be it xen or dom0).
> > If the device is passed through to dom0 or other domain (see later
> > patches), that domain will effectively have access to those buffers too.
> > It does give such domain yet another way to DoS the system (as is the
> > case when having PCI device assigned already), but also possibly steal
> > the console ring content. Thus, such domain should be a trusted one.
> > In any case, prevent anything else being placed on those pages by adding
> > artificial padding.
> 
> I don't think this device should be allowed to be assigned to any
> DomU. Just like the EHCI driver, at some point in the series you
> will want to call pci_hide_device() (you may already do, and I may
> merely have missed it).

There is the major difference compared to the EHCI driver: the XHCI
hardware interface was designed for debug capability to be driven by
separate driver (see description of patch 9). The device reset is the
only action that needs some coordination and this patch series follows
what Linux does (re-enable DbC part, when it detects the XHCI reset).
Having this capability is especially important when one has only a
single USB controller (many, if not most recent Intel platforms) and has
some important devices on USB (for example system disk, or the only ethernet
controller - I have both of those cases in my test lab...).

Anyway, this patch is necessary even if no domain would use the device.
As Andrew explained in response to the cover letter of the RFC series,
Xen doesn't have IOMMU context for devices used by Xen exclusively. So,
with the current model, it would be pci_ro_device() + dom0's IOMMU
context.

> > Using this API for DbC pages requires raising MAX_USER_RMRR_PAGES.
> 
> I disagree here: This is merely an effect of you re-using the pre-
> existing functionality there with too little customization. I think
> the respective check shouldn't be applied for internal uses.

Ok, I'll move the check.

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

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

Re: [PATCH v2 4/9] console: support multiple serial console simultaneously

[Jan Beulich]

On 18.07.2022 14:48, Marek Marczykowski-Górecki wrote:
> On Wed, Jul 13, 2022 at 11:39:07AM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> --- 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 4
>>
>> Might this want to be a Kconfig setting?
> 
> Is that going to be useful for anybody (who isn't modifying the code
> anyway, for example to add yet another console driver)?

If allowing multiple serial consoles is deemed useful, then making
their maximum count build-time configurable is quite likely useful.
People may not want to allow multiple of them, for example.

>>> @@ -1291,7 +1322,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(sercon_handle[0], suspend_steal_fn);
>>>      serial_suspend();
>>>      return 0;
>>>  }
>>
>> The commit message gives no explanation why only the first handle
>> would want/need dealing with here.
> 
> Sure, I can add an explanation. I'm adding this comment to console_steal():
> /* Redirect any console output to *fn*, if *handle* is configured as a console. */
> 
> So, calling console_steal() is about all serial consoles, not just a
> specific one. The use case for this "if" part is gdbstub, which wants
> to redirect serial output only if that serial was configured as both
> console and gdb. Having proper per-console stealing is doable, but IMO
> not worth it (it would require also avoiding duplicated output in case
> of multiple serial consoles, and probably few more corner cases).

And what if the one handle you pass on isn't the one matching the
console the gdbstub is using? While I understand that per-console
stealing may have some sharp edges, I don't currently see how we can
get away here without handling things per-console.

>> One overall remark: Especially with sync_console latency is going to
>> be yet worse with all output being done sequentially. The help text
>> for "console=" will want to mention this, up and until this would be
>> parallelized.
> 
> I don't think it was parallelized anywhere. All the relevant functions
> (__putstr especially) write to various console types sequentially. The
> difference is that previously only the last "serial" console was used,
> all the other were silently ignored. So, it was "parallel" with all
> _zero other_ serial consoles, but not other console types.

Parallelizing vga and serial likely wasn't deemed very useful, as
vga has negligible latency compared to a (slow) serial line (albeit
I leave aside software scrolling here, which indeed is slow). There
are also no commands involved in vga output which may require waiting
for their completion - it's all simple MMIO writes (and hence the
slowness of scrolling could only be dealt with by involving a 2nd
CPU, as the one doing the scrolling can't at the same time do output
to another device; nevertheless some of the latency could be
compensated by doing output in suitable order). This is quite
different when it comes to multiple serial consoles.

> Anyway, both help text and boot warning for sync_console already warn
> about it. Do you want me to include relation to number of configured
> console explicitly?

I think it should be made explicit, yes.

Jan

Re: [PATCH v2 9/9] xue: allow driving the rest of XHCI by a domain while Xen uses DbC

[Jan Beulich]

On 18.07.2022 14:54, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 02:06:07PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> That's possible, because the capability was designed specifically to
>>> allow separate driver handle it, in parallel to unmodified xhci driver
>>> (separate set of registers, pretending the port is "disconnected" for
>>> the main xhci driver etc). It works with Linux dom0, although requires
>>> an awful hack - re-enabling bus mastering behind dom0's backs.
>>> Linux driver does similar thing - see
>>> drivers/usb/early/xhci-dbc.c:xdbc_handle_events().
>>
>> Isn't there a risk that intermediately data was lost?
> 
> Yes, there is such risk (although minimal in practice - it happens just
> once during dom0 boot). You can avoid it by instructing dom0 to not use
> that USB controller.
> Having this capability is really helpful (compared with the alternative
> of using the whole USB controller by either Xen or Linux), as many
> (most) systems have only a single USB controller.

No question about the usefulness. But this aspect wants spelling out,
and it is one of the arguments against allowing use of the device by
other than hwdom.

>>> To avoid Linux messing with the DbC, mark this MMIO area as read-only.
>>
>> In principle this would want to happen quite a bit earlier in the
>> series. I'm okay with it being kept here as long as it is made
>> very obvious to and easily noticeable by committers that this series
>> should only be committed all in one go.
>>
>> Also along with this is where I'd see the pci_hide_device() go.
> 
> Earlier version of the patch set had pci_ro_device() before this patch.
> I can add pci_ro_device() in the initial patch, and drop it in this one.

Having pci_ro_device() up to here sounds reasonable, but then you still
want to flip to using pci_hide_device() rather than just dropping the
call.

Jan

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +static int xue_init_dbc(struct xue *xue)
> > +{
> > +    uint64_t erdp = 0;
> > +    uint64_t out = 0;
> > +    uint64_t in = 0;
> > +    uint64_t mbs = 0;
> > +    struct xue_dbc_reg *reg = xue_find_dbc(xue);
> > +
> > +    if ( !reg )
> > +        return 0;
> > +
> > +    xue->dbc_reg = reg;
> > +    xue_disable_dbc(xue);
> > +
> > +    xue_trb_ring_init(xue, &xue->dbc_ering, 0, XUE_DB_INVAL);
> > +    xue_trb_ring_init(xue, &xue->dbc_oring, 1, XUE_DB_OUT);
> > +    xue_trb_ring_init(xue, &xue->dbc_iring, 1, XUE_DB_IN);
> > +
> > +    erdp = virt_to_maddr(xue->dbc_ering.trb);
> > +    if ( !erdp )
> > +        return 0;
> > +
> > +    memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> > +    xue->dbc_erst->base = erdp;
> > +    xue->dbc_erst->size = XUE_TRB_RING_CAP;
> > +
> > +    mbs = (reg->ctrl & 0xFF0000) >> 16;
> > +    out = virt_to_maddr(xue->dbc_oring.trb);
> > +    in = virt_to_maddr(xue->dbc_iring.trb);
> > +
> > +    memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> > +    xue_init_strings(xue, xue->dbc_ctx->info);
> > +    xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> > +    xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> > +
> > +    reg->erstsz = 1;
> > +    reg->erstba = virt_to_maddr(xue->dbc_erst);
> > +    reg->erdp = erdp;
> > +    reg->cp = virt_to_maddr(xue->dbc_ctx);
> 
> The only place this field is read looks to be xue_dump().

No, reg is MMIO, all those assignments are actually configuring the
device.

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

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

Re: [PATCH v2 8/9] xue: mark DMA buffers as reserved for the device

[Marek Marczykowski-Górecki]

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

On Thu, Jul 14, 2022 at 01:51:06PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +    /*
> > +     * Don't place anything else on this page - it will be
> > +     * DMA-reachable by the USB controller.
> > +     */
> > +    char _pad[0] __aligned(XUE_PAGE_SIZE);
> 
> I don't think this is needed, due to sizeof() being required to be
> a multiple of alignof().

I'd prefer to be explicit about this, because if some future change
breaks this property (makes alignment smaller than a page size), the
result will be pretty bad.

> > +};
> > +static struct xue_dma_bufs xue_dma_bufs __aligned(XUE_PAGE_SIZE);
> 
> I don't think the alignment here is needed, as the struct will
> already have suitable alignment (derived from the biggest field
> alignment value). Instead please consider putting this in
> .bss.page_aligned.

Ok.

> > @@ -990,16 +999,22 @@ void __init xue_uart_init(void)
> >          xue->sbdf = PCI_SBDF(0, bus, slot, func);
> >      }
> >  
> > -    xue->dbc_ctx = &ctx;
> > -    xue->dbc_erst = &erst;
> > -    xue->dbc_ering.trb = evt_trb;
> > -    xue->dbc_oring.trb = out_trb;
> > -    xue->dbc_iring.trb = in_trb;
> > -    xue->dbc_owork.buf = wrk_buf;
> > -    xue->dbc_str = str_buf;
> > +    xue->dbc_ctx = &xue_dma_bufs.ctx;
> > +    xue->dbc_erst = &xue_dma_bufs.erst;
> > +    xue->dbc_ering.trb = xue_dma_bufs.evt_trb;
> > +    xue->dbc_oring.trb = xue_dma_bufs.out_trb;
> > +    xue->dbc_iring.trb = xue_dma_bufs.in_trb;
> > +    xue->dbc_owork.buf = xue_dma_bufs.wrk_buf;
> > +    xue->dbc_str = xue_dma_bufs.str_buf;
> >  
> >      if ( xue_open(xue) )
> > +    {
> > +        iommu_add_extra_reserved_device_memory(
> > +                PFN_DOWN(virt_to_maddr(&xue_dma_bufs)),
> 
> virt_to_pfn()?

Doesn't exist. Did you mean virt_to_mfn()?

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

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

Re: [PATCH v2 1/9] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich]

On 20.07.2022 22:12, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +static int xue_init_dbc(struct xue *xue)
>>> +{
>>> +    uint64_t erdp = 0;
>>> +    uint64_t out = 0;
>>> +    uint64_t in = 0;
>>> +    uint64_t mbs = 0;
>>> +    struct xue_dbc_reg *reg = xue_find_dbc(xue);
>>> +
>>> +    if ( !reg )
>>> +        return 0;
>>> +
>>> +    xue->dbc_reg = reg;
>>> +    xue_disable_dbc(xue);
>>> +
>>> +    xue_trb_ring_init(xue, &xue->dbc_ering, 0, XUE_DB_INVAL);
>>> +    xue_trb_ring_init(xue, &xue->dbc_oring, 1, XUE_DB_OUT);
>>> +    xue_trb_ring_init(xue, &xue->dbc_iring, 1, XUE_DB_IN);
>>> +
>>> +    erdp = virt_to_maddr(xue->dbc_ering.trb);
>>> +    if ( !erdp )
>>> +        return 0;
>>> +
>>> +    memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
>>> +    xue->dbc_erst->base = erdp;
>>> +    xue->dbc_erst->size = XUE_TRB_RING_CAP;
>>> +
>>> +    mbs = (reg->ctrl & 0xFF0000) >> 16;
>>> +    out = virt_to_maddr(xue->dbc_oring.trb);
>>> +    in = virt_to_maddr(xue->dbc_iring.trb);
>>> +
>>> +    memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
>>> +    xue_init_strings(xue, xue->dbc_ctx->info);
>>> +    xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
>>> +    xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
>>> +
>>> +    reg->erstsz = 1;
>>> +    reg->erstba = virt_to_maddr(xue->dbc_erst);
>>> +    reg->erdp = erdp;
>>> +    reg->cp = virt_to_maddr(xue->dbc_ctx);
>>
>> The only place this field is read looks to be xue_dump().
> 
> No, reg is MMIO, all those assignments are actually configuring the
> device.

Well, then the pointer would preferably be marked __iomem and the
writes should be carried out via writel() and friends.

Jan

Re: [PATCH v2 8/9] xue: mark DMA buffers as reserved for the device

[Jan Beulich]

On 20.07.2022 22:17, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 01:51:06PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +    /*
>>> +     * Don't place anything else on this page - it will be
>>> +     * DMA-reachable by the USB controller.
>>> +     */
>>> +    char _pad[0] __aligned(XUE_PAGE_SIZE);
>>
>> I don't think this is needed, due to sizeof() being required to be
>> a multiple of alignof().
> 
> I'd prefer to be explicit about this, because if some future change
> breaks this property (makes alignment smaller than a page size), the
> result will be pretty bad.

I don't mind you leaving the comment; anyone making adjustments there
ought to be checking the effects of what they're doing.

>>> @@ -990,16 +999,22 @@ void __init xue_uart_init(void)
>>>          xue->sbdf = PCI_SBDF(0, bus, slot, func);
>>>      }
>>>  
>>> -    xue->dbc_ctx = &ctx;
>>> -    xue->dbc_erst = &erst;
>>> -    xue->dbc_ering.trb = evt_trb;
>>> -    xue->dbc_oring.trb = out_trb;
>>> -    xue->dbc_iring.trb = in_trb;
>>> -    xue->dbc_owork.buf = wrk_buf;
>>> -    xue->dbc_str = str_buf;
>>> +    xue->dbc_ctx = &xue_dma_bufs.ctx;
>>> +    xue->dbc_erst = &xue_dma_bufs.erst;
>>> +    xue->dbc_ering.trb = xue_dma_bufs.evt_trb;
>>> +    xue->dbc_oring.trb = xue_dma_bufs.out_trb;
>>> +    xue->dbc_iring.trb = xue_dma_bufs.in_trb;
>>> +    xue->dbc_owork.buf = xue_dma_bufs.wrk_buf;
>>> +    xue->dbc_str = xue_dma_bufs.str_buf;
>>>  
>>>      if ( xue_open(xue) )
>>> +    {
>>> +        iommu_add_extra_reserved_device_memory(
>>> +                PFN_DOWN(virt_to_maddr(&xue_dma_bufs)),
>>
>> virt_to_pfn()?
> 
> Doesn't exist. Did you mean virt_to_mfn()?

Oh, sorry, never mind then. virt_to_mfn() would be good to use if
the function took mfn_t, but as long as it doesn't what you have
is as good.

Jan