Re: [PATCH v1 01/10] drivers/char: Add support for Xue USB3 debugger

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.06.2022 16:30, Marek Marczykowski-Górecki wrote:
> From: Connor Davis <davisc@xxxxxxxxxxxx>
> 
> [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.

Which suggests that the command line doc also wants updating here.

> --- a/xen/drivers/char/Kconfig
> +++ b/xen/drivers/char/Kconfig
> @@ -74,3 +74,10 @@ 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
> +     depends on X86

This isn't very useful when there's no prompt, and only "select" would
enable an option. Even if HAS_EHCI has it (for whatever reason), I'd
prefer if we could avoid proliferation of the oddity.

> +     help
> +      This selects the USB based XHCI debug capability to be used as a UART. 
> If
> +      you have an x86 based system with USB3, say Y.

Indentation looks wrong here - the HAS_EHCI one in context shows how
it's expected to be.

> --- /dev/null
> +++ b/xen/drivers/char/xue.c
> @@ -0,0 +1,957 @@
> +/*
> + * drivers/char/xue.c
> + *
> + * Xen port for the xue debugger

Since even here it's not spelled out - may I ask what "xue" actually
stands for (assumng it's an acronym)?

> + * 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>
> +
> +#define XUE_POLL_INTERVAL 100 /* us */
> +
> +#define XUE_PAGE_SIZE 4096ULL
> +
> +/* Supported xHC PCI configurations */
> +#define XUE_XHC_CLASSC 0xC0330ULL

While I'm not meaning to fully review the code in this file (for lack
of knowledge on xhci), the two ULL suffixes above strike me as odd.
Is there a specific reason these can't just be U?

> +/* 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 */
> +#pragma pack(push, 1)

Why? There's no mis-aligned field ...

> +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;
> +};
> +#pragma pack(pop)

... anywhere in these structures, afaict.

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

Nit: In Xen we don't normally use __asm, but asm.

> +static void xue_flush_range(struct xue *xue, void *ptr, uint32_t bytes)
> +{
> +    uint32_t i;
> +
> +    const uint32_t clshft = 6;
> +    const uint32_t clsize = (1UL << clshft);
> +    const uint32_t clmask = clsize - 1;
> +
> +    uint32_t lines = (bytes >> clshft);
> +    lines += (bytes & clmask) != 0;
> +
> +    for ( i = 0; i < lines; i++ )
> +        clflush((void *)((uint64_t)ptr + (i * clsize)));
> +}

Please drop this function (which doesn't even use its first parameter)
and use cache_flush() instead. (At the example of this though, please
see ./CODING_STYLE for the use of fixed-width types. I understand
there are many places where their use is appropriate in a drivers, but
none of the uses above look to fall in this class.)

> +static int xue_init_xhc(struct xue *xue)

Looks like this function returns boolean, and hence wants to have bool
return type. Also looks like this can be __init. Both remarks similarly
apply to xue_open() (the only caller of this function).

> +{
> +    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++ ) {

Nit: Xen style wants the brace on its own line, like you have almost
everywhere else.

> +        uint32_t dev = (devfn & 0xF8) >> 3;
> +        uint32_t fun = devfn & 0x07;
> +        pci_sbdf_t sbdf = PCI_SBDF(0, dev, fun);

This is at best an abuse, as per

#define PCI_SBDF(s, b, d...) PCI_(SBDF, count_args(s, b, ##d))(s, b, ##d)

But really I think this generates the wrong coordinates. What you can
fold is devfn, i.e. PCI_SBDF(0, 0, devfn). (If you don't want to use
that form, then please avoid open-coding PCI_SLOT() and PCI_FUNC().)

> +        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 0;
> +    }
> +
> +    /* ...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);

What if there are multiple?

> +    /* IO BARs not allowed; BAR must be 64-bit */
> +    if ( (bar0 & 0x1) != 0 || ((bar0 & 0x6) >> 1) != 2 )

Please don't open-code constants from xen/pci.h.

> +        return 0;
> +
> +    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;

Same here and ...

> +    pci_conf_write32(xue->sbdf, PCI_BASE_ADDRESS_0, bar0);
> +
> +    xue->xhc_mmio_phys = (bar0 & 0xFFFFFFF0) | (bar1 << 32);

... here.

> +static struct uart_driver xue_uart_driver = {
> +    .init_preirq = xue_uart_init_preirq,
> +    .init_postirq = xue_uart_init_postirq,
> +    .endboot = NULL,
> +    .suspend = NULL,
> +    .resume = NULL,
> +    .tx_ready = xue_uart_tx_ready,
> +    .putc = xue_uart_putc,
> +    .flush = xue_uart_flush,
> +    .getc = NULL
> +};

Please omit the NULL initializers.

> +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;

Especially the page-sized entities want allocating dynamically here, as
they won't be needed without the command line option requesting the use
of this driver.

> +    xue_open(xue);

No check of return value?

> +    serial_register_uart(SERHND_DBGP, &xue_uart_driver, &xue_uart);
> +}
> +
> +void xue_uart_dump(void)
> +{
> +    struct xue_uart *uart = &xue_uart;
> +    struct xue *xue = &uart->xue;
> +
> +    xue_dump(xue);
> +}

This function looks to be unused (and lacks a declaration).

Jan