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

[Jan Beulich <jbeulich@xxxxxxxx>]

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