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

[Jan Beulich]

On 20.07.2022 22:12, Marek Marczykowski-Górecki wrote:
> On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +static int xue_init_dbc(struct xue *xue)
>>> +{
>>> +uint64_t erdp = 0;
>>> +uint64_t out = 0;
>>> +uint64_t in = 0;
>>> +uint64_t mbs = 0;
>>> +struct xue_dbc_reg *reg = xue_find_dbc(xue);
>>> +
>>> +if ( !reg )
>>> +return 0;
>>> +
>>> +xue->dbc_reg = reg;
>>> +xue_disable_dbc(xue);
>>> +
>>> +xue_trb_ring_init(xue, >dbc_ering, 0, XUE_DB_INVAL);
>>> +xue_trb_ring_init(xue, >dbc_oring, 1, XUE_DB_OUT);
>>> +xue_trb_ring_init(xue, >dbc_iring, 1, XUE_DB_IN);
>>> +
>>> +erdp = virt_to_maddr(xue->dbc_ering.trb);
>>> +if ( !erdp )
>>> +return 0;
>>> +
>>> +memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
>>> +xue->dbc_erst->base = erdp;
>>> +xue->dbc_erst->size = XUE_TRB_RING_CAP;
>>> +
>>> +mbs = (reg->ctrl & 0xFF) >> 16;
>>> +out = virt_to_maddr(xue->dbc_oring.trb);
>>> +in = virt_to_maddr(xue->dbc_iring.trb);
>>> +
>>> +memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
>>> +xue_init_strings(xue, xue->dbc_ctx->info);
>>> +xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
>>> +xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
>>> +
>>> +reg->erstsz = 1;
>>> +reg->erstba = virt_to_maddr(xue->dbc_erst);
>>> +reg->erdp = erdp;
>>> +reg->cp = virt_to_maddr(xue->dbc_ctx);
>>
>> The only place this field is read looks to be xue_dump().
> 
> No, reg is MMIO, all those assignments are actually configuring the
> device.

Well, then the pointer would preferably be marked __iomem and the
writes should be carried out via writel() and friends.

Jan

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

[Marek Marczykowski-Górecki]

On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +static int xue_init_dbc(struct xue *xue)
> > +{
> > +uint64_t erdp = 0;
> > +uint64_t out = 0;
> > +uint64_t in = 0;
> > +uint64_t mbs = 0;
> > +struct xue_dbc_reg *reg = xue_find_dbc(xue);
> > +
> > +if ( !reg )
> > +return 0;
> > +
> > +xue->dbc_reg = reg;
> > +xue_disable_dbc(xue);
> > +
> > +xue_trb_ring_init(xue, >dbc_ering, 0, XUE_DB_INVAL);
> > +xue_trb_ring_init(xue, >dbc_oring, 1, XUE_DB_OUT);
> > +xue_trb_ring_init(xue, >dbc_iring, 1, XUE_DB_IN);
> > +
> > +erdp = virt_to_maddr(xue->dbc_ering.trb);
> > +if ( !erdp )
> > +return 0;
> > +
> > +memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> > +xue->dbc_erst->base = erdp;
> > +xue->dbc_erst->size = XUE_TRB_RING_CAP;
> > +
> > +mbs = (reg->ctrl & 0xFF) >> 16;
> > +out = virt_to_maddr(xue->dbc_oring.trb);
> > +in = virt_to_maddr(xue->dbc_iring.trb);
> > +
> > +memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> > +xue_init_strings(xue, xue->dbc_ctx->info);
> > +xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> > +xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> > +
> > +reg->erstsz = 1;
> > +reg->erstba = virt_to_maddr(xue->dbc_erst);
> > +reg->erdp = erdp;
> > +reg->cp = virt_to_maddr(xue->dbc_ctx);
> 
> The only place this field is read looks to be xue_dump().

No, reg is MMIO, all those assignments are actually configuring the
device.

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


signature.asc
Description: PGP signature

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

[Jan Beulich]

On 18.07.2022 12:45, Marek Marczykowski-Górecki wrote:
> On Tue, Jul 12, 2022 at 05:59:51PM +0200, Jan Beulich wrote:
>> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
>>> +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, 0x);
>>> +xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) 
>>> & 0xFFF0) + 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.

It might seem so, I agree, but then upon encountering a later error
you'll need more precautions so you would able to restore the command
register to its original value. I think it's easier / clearer when
you keep command register save/restore to within functions.

Jan

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

[Marek Marczykowski-Górecki]

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[  | @pci:. ]`
> > +> `= 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, );
> >  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 .
> > + *
> > + * Copyright (c) 2019 Assured Information Security.
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> 
> 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 

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

[Marek Marczykowski-Górecki]

On Thu, Jul 14, 2022 at 01:58:25PM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +static int xue_init_dbc(struct xue *xue)
> > +{
> > +uint64_t erdp = 0;
> > +uint64_t out = 0;
> > +uint64_t in = 0;
> > +uint64_t mbs = 0;
> > +struct xue_dbc_reg *reg = xue_find_dbc(xue);
> > +
> > +if ( !reg )
> > +return 0;
> > +
> > +xue->dbc_reg = reg;
> > +xue_disable_dbc(xue);
> > +
> > +xue_trb_ring_init(xue, >dbc_ering, 0, XUE_DB_INVAL);
> > +xue_trb_ring_init(xue, >dbc_oring, 1, XUE_DB_OUT);
> > +xue_trb_ring_init(xue, >dbc_iring, 1, XUE_DB_IN);
> > +
> > +erdp = virt_to_maddr(xue->dbc_ering.trb);
> > +if ( !erdp )
> > +return 0;
> > +
> > +memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> > +xue->dbc_erst->base = erdp;
> > +xue->dbc_erst->size = XUE_TRB_RING_CAP;
> > +
> > +mbs = (reg->ctrl & 0xFF) >> 16;
> > +out = virt_to_maddr(xue->dbc_oring.trb);
> > +in = virt_to_maddr(xue->dbc_iring.trb);
> > +
> > +memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> > +xue_init_strings(xue, xue->dbc_ctx->info);
> > +xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> > +xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> > +
> > +reg->erstsz = 1;
> > +reg->erstba = virt_to_maddr(xue->dbc_erst);
> > +reg->erdp = erdp;
> > +reg->cp = virt_to_maddr(xue->dbc_ctx);
> 
> The only place this field is read looks to be xue_dump().
> 
> > +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);
> 
> While I think I can see the point of the page-size alignment, can you
> please clarify the need for the three instances of 64-byte alignment?

(Guessing why original author of this code did it this way) At least
ERSTBA (5.5.2.3.2 section of the spec) is required to be 64-byte aligned
by the xHCI spec. Interestingly the DbC version of this register
(DCERSTBA, section 7.6.8.3.2) requires just 16-byte alignment.
ctx seems to require just 16-byte alignment too, and str_buf (in
practice) requires just 2-byte alignment.
I'll try to reduce those alignments and see if that still works...

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


signature.asc
Description: PGP signature

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

[Marek Marczykowski-Górecki]

On Thu, Jul 14, 2022 at 08:05:28AM +0200, Jan Beulich wrote:
> On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> > +struct xue {
> > +struct xue_dbc_reg *dbc_reg;
> > +struct xue_dbc_ctx *dbc_ctx;
> > +struct xue_erst_segment *dbc_erst;
> > +struct xue_trb_ring dbc_ering;
> > +struct xue_trb_ring dbc_oring;
> > +struct xue_trb_ring dbc_iring;
> > +struct xue_work_ring dbc_owork;
> > +char *dbc_str;
> > +
> > +pci_sbdf_t sbdf;
> > +uint64_t xhc_mmio_phys;
> > +uint64_t xhc_mmio_size;
> > +uint64_t xhc_dbc_offset;
> 
> One more observation: None of these four field look to be needed.
> They're all used only in a single function, so could be local
> variables there (and xhc_dbc_offset is only ever written, so
> could be dropped altogether).

While xhc_mmio_size indeed isn't used outside of this function,
xhc_mmio_phys and xhc_dbc_offset are in later patches.

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


signature.asc
Description: PGP signature

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

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> +static int xue_init_dbc(struct xue *xue)
> +{
> +uint64_t erdp = 0;
> +uint64_t out = 0;
> +uint64_t in = 0;
> +uint64_t mbs = 0;
> +struct xue_dbc_reg *reg = xue_find_dbc(xue);
> +
> +if ( !reg )
> +return 0;
> +
> +xue->dbc_reg = reg;
> +xue_disable_dbc(xue);
> +
> +xue_trb_ring_init(xue, >dbc_ering, 0, XUE_DB_INVAL);
> +xue_trb_ring_init(xue, >dbc_oring, 1, XUE_DB_OUT);
> +xue_trb_ring_init(xue, >dbc_iring, 1, XUE_DB_IN);
> +
> +erdp = virt_to_maddr(xue->dbc_ering.trb);
> +if ( !erdp )
> +return 0;
> +
> +memset(xue->dbc_erst, 0, sizeof(*xue->dbc_erst));
> +xue->dbc_erst->base = erdp;
> +xue->dbc_erst->size = XUE_TRB_RING_CAP;
> +
> +mbs = (reg->ctrl & 0xFF) >> 16;
> +out = virt_to_maddr(xue->dbc_oring.trb);
> +in = virt_to_maddr(xue->dbc_iring.trb);
> +
> +memset(xue->dbc_ctx, 0, sizeof(*xue->dbc_ctx));
> +xue_init_strings(xue, xue->dbc_ctx->info);
> +xue_init_ep(xue->dbc_ctx->ep_out, mbs, xue_ep_bulk_out, out);
> +xue_init_ep(xue->dbc_ctx->ep_in, mbs, xue_ep_bulk_in, in);
> +
> +reg->erstsz = 1;
> +reg->erstba = virt_to_maddr(xue->dbc_erst);
> +reg->erdp = erdp;
> +reg->cp = virt_to_maddr(xue->dbc_ctx);

The only place this field is read looks to be xue_dump().

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

While I think I can see the point of the page-size alignment, can you
please clarify the need for the three instances of 64-byte alignment?

Jan

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

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> +struct xue {
> +struct xue_dbc_reg *dbc_reg;
> +struct xue_dbc_ctx *dbc_ctx;
> +struct xue_erst_segment *dbc_erst;
> +struct xue_trb_ring dbc_ering;
> +struct xue_trb_ring dbc_oring;
> +struct xue_trb_ring dbc_iring;
> +struct xue_work_ring dbc_owork;
> +char *dbc_str;
> +
> +pci_sbdf_t sbdf;
> +uint64_t xhc_mmio_phys;
> +uint64_t xhc_mmio_size;
> +uint64_t xhc_dbc_offset;

One more observation: None of these four field look to be needed.
They're all used only in a single function, so could be local
variables there (and xhc_dbc_offset is only ever written, so
could be dropped altogether).

Jan

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

[Jan Beulich]

On 06.07.2022 17:32, Marek Marczykowski-Górecki wrote:
> From: Connor Davis 
> 
> [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.
> 
> [Marek]
> The Xue driver is taken from https://github.com/connojd/xue and heavily
> refactored to fit into Xen code base. Major changes include:
> - drop support for non-Xen systems
> - drop xue_ops abstraction
> - use Xen's native helper functions for PCI access
> - move all the code to xue.c, drop "inline"
> - build for x86 only
> - annotate functions with cf_check
> - adjust for Xen's code style
> 
> At this stage, only the first xHCI is considered. Later patch adds
> support for choosing specific one.
> The driver is initiallized before memory allocator works, so all the
> transfer buffers (about 2MB of them) are allocated statically and will
> use memory even if XUE console is not selected. The driver can be
> disabled build time to reclaim this memory.
> 
> Signed-off-by: Connor Davis 

Btw - iirc this email address has already been bouncing for me when
replying to v1 patches. Interestingly enough you did Cc the cover
letter to Connor Davis  (which I'm using in
replacement for the other address in this reply). And I can only
assume that the address did bounce for you as well when sending
both v1 and v2 ...

Jan

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

[Jan Beulich]

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[  | @pci:. ]`
> +> `= 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, );
>  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 .
> + *
> + * Copyright (c) 2019 Assured Information Security.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 

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, 0x);
> +xue->xhc_mmio_size = ~(pci_conf_read32(xue->sbdf, PCI_BASE_ADDRESS_0) & 
> 0xFFF0) + 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 

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

[Marek Marczykowski-Górecki]

On Wed, Jul 06, 2022 at 05:32:06PM +0200, Marek Marczykowski-Górecki wrote:
> diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
> index e5f7b1d8eb8a..d12b2205dafc 100644
> --- 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"
> + depends on X86
> + help
> +   This selects the USB based XHCI debug capability to be used as a UART.
> +   Enabling this option makes Xen use extra ~2MB memory, even if XHCI 
> UART

My math sucks here... 58 pages is 232KiB.

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


signature.asc
Description: PGP signature

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

[Marek Marczykowski-Górecki]

From: Connor Davis 

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

[Marek]
The Xue driver is taken from https://github.com/connojd/xue and heavily
refactored to fit into Xen code base. Major changes include:
- drop support for non-Xen systems
- drop xue_ops abstraction
- use Xen's native helper functions for PCI access
- move all the code to xue.c, drop "inline"
- build for x86 only
- annotate functions with cf_check
- adjust for Xen's code style

At this stage, only the first xHCI is considered. Later patch adds
support for choosing specific one.
The driver is initiallized before memory allocator works, so all the
transfer buffers (about 2MB of them) are allocated statically and will
use memory even if XUE console is not selected. The driver can be
disabled build time to reclaim this memory.

Signed-off-by: Connor Davis 
Signed-off-by: Marek Marczykowski-Górecki 
---
Changes in v2:
- drop #pragma pack
- fix indentation in Kconfig
- minor style fixes
- use cache_flush()
- mark init functions as __init, and return bool
- fix PCI_SBDF usage, use constants from pci_regs.h
- add command line docs
- allow disabling the driver in menuconfig, to reclaim 2MB allocated
  memory
- guard unused debug functions with #ifdef XUE_DEBUG
---
 docs/misc/xen-command-line.pandoc |   5 +-
 xen/arch/x86/include/asm/fixmap.h |   4 +-
 xen/arch/x86/setup.c  |   3 +-
 xen/drivers/char/Kconfig  |   9 +-
 xen/drivers/char/Makefile |   1 +-
 xen/drivers/char/xue.c| 933 +++-
 xen/include/xen/serial.h  |   3 +-
 7 files changed, 958 insertions(+)
 create mode 100644 xen/drivers/char/xue.c

diff --git a/docs/misc/xen-command-line.pandoc 
b/docs/misc/xen-command-line.pandoc
index a92b7d228cae..f9fa857bd84e 100644
--- 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[  | @pci:. ]`
+> `= 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.
+
 ### debug_stack_lines
 > `= `
 
diff --git a/xen/arch/x86/include/asm/fixmap.h 
b/xen/arch/x86/include/asm/fixmap.h
index 20746afd0a2a..bc39ffe896b1 100644
--- a/xen/arch/x86/include/asm/fixmap.h
+++ b/xen/arch/x86/include/asm/fixmap.h
@@ -25,6 +25,8 @@
 #include 
 #include 
 
+#define MAX_XHCI_PAGES 16
+
 /*
  * Here we define all the compile-time 'special' virtual
  * addresses. The point is to have a constant address at
@@ -43,6 +45,8 @@ enum fixed_addresses {
 FIX_COM_BEGIN,
 FIX_COM_END,
 FIX_EHCI_DBGP,
+FIX_XHCI_BEGIN,
+FIX_XHCI_END = FIX_XHCI_BEGIN + MAX_XHCI_PAGES - 1,
 #ifdef CONFIG_XEN_GUEST
 FIX_PV_CONSOLE,
 FIX_XEN_SHARED_INFO,
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 53a73010e029..58a0723a7501 100644
--- 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, );
 ehci_dbgp_init();
+#ifdef CONFIG_HAS_XHCI
+xue_uart_init();
+#endif
 console_init_preirq();
 
 if ( pvh_boot )
diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig
index e5f7b1d8eb8a..d12b2205dafc 100644
--- 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"
+   depends on X86
+   help
+ This selects the USB based XHCI debug capability to be used as a UART.
+ Enabling this option makes Xen use extra ~2MB memory, even if XHCI 
UART
+ (XUE) is not selected.
+ If you have an x86 based system with USB3, say Y.
diff --git a/xen/drivers/char/Makefile b/xen/drivers/char/Makefile
index 14e67cf072d7..bda1e44d3f39 100644
--- a/xen/drivers/char/Makefile
+++ b/xen/drivers/char/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_HAS_MVEBU) += mvebu-uart.o
 obj-$(CONFIG_HAS_OMAP) += omap-uart.o
 obj-$(CONFIG_HAS_SCIF) += scif-uart.o
 obj-$(CONFIG_HAS_EHCI) += ehci-dbgp.o
+obj-$(CONFIG_HAS_XHCI) += xue.o
 obj-$(CONFIG_HAS_IMX_LPUART) += imx-lpuart.o
 obj-$(CONFIG_ARM) += arm-uart.o
 obj-y += serial.o
diff --git