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

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

On Wed, Jun 15, 2022 at 04:25:54PM +0200, Jan Beulich wrote:
> On 07.06.2022 16:30, Marek Marczykowski-Górecki wrote:
> > From: Connor Davis <davisc@xxxxxxxxxxxx>
> > --- /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)?

Honestly, I don't know. That would be a question to Connor.

> > +/* 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?

I don't think so (that's just how it was in xue.h). Will adjust. The
same response applies to many other remarks.

> > +    /* ...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?

You mean two 32-bit BARs? I check for that below (refusing to use them).
Anyway, I don't think that's a thing in USB 3.0 controllers.


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

(...)

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

Are you okay with changing this only in patch 9, where I restructure those
buffers anyway?

> > +    xue_open(xue);
> 
> No check of return value?

Good catch, will adjust.

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

It is unused, same as xue_dump(), but is extremely useful when
debugging. Should I put it behind something like #ifdef XUE_DEBUG,
accompanied with a comment about its purpose?

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

Attachment: signature.asc
Description: PGP signature