Re: [PATCH v3 01/10] drivers/char: Add support for USB3 DbC debugger

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

On Thu, Aug 04, 2022 at 02:57:49PM +0200, Jan Beulich wrote:
> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> > +/* Defines the size in bytes of TRB rings as 2^DBC_TRB_RING_ORDER * 4096 */
> > +#ifndef DBC_TRB_RING_ORDER
> > +#define DBC_TRB_RING_ORDER 4
> > +#endif
> > +#define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE * (1 << DBC_TRB_RING_ORDER))
> 
> I have to admit that I'm always puzzled when seeing such - why not
> 
> #define DBC_TRB_RING_CAP (DBC_TRB_PER_PAGE << DBC_TRB_RING_ORDER)
> 
> ?

I think the former is a bit more clear what's the actual size, but the
end result is the same, I can change that.

> > +struct dbc {
> > +    struct dbc_reg __iomem *dbc_reg;
> > +    struct xhci_dbc_ctx *dbc_ctx;
> > +    struct xhci_erst_segment *dbc_erst;
> > +    struct xhci_trb_ring dbc_ering;
> > +    struct xhci_trb_ring dbc_oring;
> > +    struct xhci_trb_ring dbc_iring;
> > +    struct dbc_work_ring dbc_owork;
> > +    struct xhci_string_descriptor *dbc_str;
> 
> I'm afraid I still don't see why the static page this field is being
> initialized with is necessary. Can't you have a static variable (of
> some struct type) all pre-filled at build time, which you then apply
> virt_to_maddr() to in order to fill the respective dbc_ctx fields?

I need to keep this structure somewhere DMA-reachable for the device (as
in - included in appropriate IOMMU context). Patch 8/10 is doing it. And
also, patch 8/10 is putting it together with other DMA-reachable
structures (not a separate page on its own). If I'd make it a separate
static variable (not part of that later struct), I'd need to reserve the
whole page for it - to guarantee no unrelated data lives on the same
(DMA-reachable) page.

As for statically initializing it, if would require the whole
(multi-page DMA-reachable) thing living in .data, not .bss, so a bigger
binary (not a huge concern due to compression, but still). But more
importantly, I don't know how to do it in a readable way, and you have
complained about readability of initializer of this structure in v2.

> That struct will be quite a bit less than a page's worth in size.

See above - it cannot share page with unrelated Xen data.

> If you build the file with -fshort-wchar, you may even be able to
> use easy to read string literals for the initializer.

I can try, but I'm not exactly sure how to make readable UTF-16
literals...

> > +static void *dbc_sys_map_xhc(uint64_t phys, size_t size)
> > +{
> > +    size_t i;
> > +
> > +    if ( size != MAX_XHCI_PAGES * DBC_PAGE_SIZE )
> > +        return NULL;
> > +
> > +    for ( i = FIX_XHCI_END; i >= FIX_XHCI_BEGIN; i-- )
> > +    {
> > +        set_fixmap_nocache(i, phys);
> > +        phys += DBC_PAGE_SIZE;
> 
> While there may be an assumption of DBC_PAGE_SIZE == PAGE_SIZE, the
> constant used here clearly needs to be PAGE_SIZE, as that's the unit
> set_fixmap_nocache() deals with.

Ok.

> > +static bool __init dbc_init_xhc(struct dbc *dbc)
> > +{
> > +    uint32_t bar0;
> > +    uint64_t bar1;
> > +    uint64_t bar_size;
> > +    uint64_t devfn;
> > +    uint16_t cmd;
> > +    size_t xhc_mmio_size;
> > +
> > +    /*
> > +     * Search PCI bus 0 for the xHC. All the host controllers supported so 
> > far
> > +     * are part of the chipset and are on bus 0.
> > +     */
> > +    for ( devfn = 0; devfn < 256; devfn++ )
> > +    {
> > +        pci_sbdf_t sbdf = PCI_SBDF(0, 0, devfn);
> > +        uint8_t hdr = pci_conf_read8(sbdf, PCI_HEADER_TYPE);
> > +
> > +        if ( hdr == 0 || hdr == 0x80 )
> > +        {
> > +            if ( (pci_conf_read32(sbdf, PCI_CLASS_REVISION) >> 8) == 
> > DBC_XHC_CLASSC )
> > +            {
> > +                dbc->sbdf = sbdf;
> > +                break;
> > +            }
> > +        }
> > +    }
> > +
> > +    if ( !dbc->sbdf.sbdf )
> > +    {
> > +        dbc_error("Compatible xHC not found on bus 0\n");
> > +        return false;
> > +    }
> > +
> > +    /* ...we found it, so parse the BAR and map the registers */
> > +    bar0 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> > +    bar1 = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1);
> > +
> > +    /* IO BARs not allowed; BAR must be 64-bit */
> > +    if ( (bar0 & PCI_BASE_ADDRESS_SPACE) != PCI_BASE_ADDRESS_SPACE_MEMORY 
> > ||
> > +         (bar0 & PCI_BASE_ADDRESS_MEM_TYPE_MASK) != 
> > PCI_BASE_ADDRESS_MEM_TYPE_64 )
> > +        return false;
> > +
> > +    cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND);
> > +    pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd & ~PCI_COMMAND_MEMORY);
> > +
> > +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, 0xFFFFFFFF);
> > +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, 0xFFFFFFFF);
> > +    bar_size = pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_0);
> > +    bar_size |= (uint64_t)pci_conf_read32(dbc->sbdf, PCI_BASE_ADDRESS_1) 
> > << 32;
> > +    xhc_mmio_size = ~(bar_size & PCI_BASE_ADDRESS_MEM_MASK) + 1;
> > +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_0, bar0);
> > +    pci_conf_write32(dbc->sbdf, PCI_BASE_ADDRESS_1, bar1);
> > +
> > +    pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd);
> > +
> > +    dbc->xhc_mmio_phys = (bar0 & PCI_BASE_ADDRESS_MEM_MASK) | (bar1 << 32);
> > +    dbc->xhc_mmio = dbc_sys_map_xhc(dbc->xhc_mmio_phys, xhc_mmio_size);
> 
> Before actually using the address to map the MMIO you will want to make
> somewhat sure firmware did initialize it: The EHCI driver checks for
> all zeroes or all ones in the writable bits.

Ok.

> 
> > +/**
> > + * The first register of the debug capability is found by traversing the
> > + * host controller's capability list (xcap) until a capability
> > + * with ID = 0xA is found. The xHCI capability list begins at address
> > + * mmio + (HCCPARAMS1[31:16] << 2).
> > + */
> > +static struct dbc_reg __iomem *xhci_find_dbc(struct dbc *dbc)
> > +{
> > +    uint32_t *xcap;
> 
> const?
> 
> > +    uint32_t xcap_val;
> > +    uint32_t next;
> > +    uint32_t id = 0;
> > +    uint8_t *mmio = (uint8_t *)dbc->xhc_mmio;
> 
> Can't this be const void * (and probably wants to also use __iomem),
> avoiding the cast here, ...
> 
> > +    uint32_t *hccp1 = (uint32_t *)(mmio + 0x10);
> 
> ... here, ...
> 
> > +    const uint32_t DBC_ID = 0xA;
> > +    int ttl = 48;
> > +
> > +    xcap = (uint32_t *)dbc->xhc_mmio;
> 
> ... and here (if actually using the local variable you've got).

Ok.

> > +/*
> > + * Note that if IN transfer support is added, then this
> > + * will need to be changed; it assumes an OUT transfer ring only
> > + */
> 
> Hmm, is this comment telling me that this driver is an output-only one?

At this point, yes. Input support is added in patch 10/10.

> Or is it simply that input doesn't use this code path?
> 
> > +static void dbc_init_string_single(struct xhci_string_descriptor *string,
> > +                                   char *ascii_str,
> 
> If this function has to survive, then const please here and ...
> 
> > +                                   uint64_t *str_ptr,
> > +                                   uint8_t *str_size_ptr)
> > +{
> > +    size_t i, len = strlen(ascii_str);
> > +
> > +    string->size = offsetof(typeof(*string), string) + len * 2;
> > +    string->type = XHCI_DT_STRING;
> > +    /* ASCII to UTF16 conversion */
> > +    for (i = 0; i < len; i++)
> 
> ... this missing blanks added here.

Ok.

> > +static struct xhci_trb evt_trb[DBC_TRB_RING_CAP];
> > +static struct xhci_trb out_trb[DBC_TRB_RING_CAP];
> > +static struct xhci_trb in_trb[DBC_TRB_RING_CAP];
> > +static struct xhci_erst_segment erst __aligned(64);
> > +static struct xhci_dbc_ctx ctx __aligned(64);
> > +static uint8_t out_wrk_buf[DBC_WORK_RING_CAP] __aligned(DBC_PAGE_SIZE);
> > +static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
> > +static char __initdata opt_dbgp[30];
> > +
> > +string_param("dbgp", opt_dbgp);
> 
> This duplicates what ehci-dbgp.c already has. I guess we can live with
> it for now and de-duplicate later, but it's still a little odd. In any
> even please move the blank line up be a line, so that string_param()
> and its referenced array are kept together.
> 
> > +void __init xhci_dbc_uart_init(void)
> > +{
> > +    struct dbc_uart *uart = &dbc_uart;
> > +    struct dbc *dbc = &uart->dbc;
> > +
> > +    if ( strncmp(opt_dbgp, "xhci", 4) )
> > +        return;
> > +
> > +    memset(dbc, 0, sizeof(*dbc));
> 
> Why? dbc_uart is a static variable, and hence already zero-initialized.

Ok.

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

Attachment: signature.asc
Description: PGP signature