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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

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: signature.asc
Description: PGP signature