Re: [PATCH] drivers/char: Add USB3 debug capability driver

[Connor Davis <connojdavis@xxxxxxxxx>]

On 5/17/21 3:27 AM, Jan Beulich wrote:
> On 12.05.2021 02:12, Connor Davis wrote:
> > Add support for the xHCI debug capability (DbC). The DbC provides
> > a SuperSpeed serial link between a debug target running Xen and a
> > debug host. To use it you will need a USB3 A/A debug cable plugged into
> > a root port on the target machine. Recent kernels report the existence
> > of the DbC capability at
> >
> >    /sys/kernel/debug/usb/xhci/<seg>:<bus>:<slot>.<func>/reg-ext-dbc:00
> >
> > The host machine should have the usb_debug.ko module on the system
> > (the cable can be plugged into any port on the host side). After the
> > host usb_debug enumerates the DbC, it will create a /dev/ttyUSB<n> file
> > that can be used with things like minicom.
> >
> > To use the DbC as a console, pass `console=dbgp dbgp=xhci`
> > on the xen command line. This will select the first host controller
> > found that implements the DbC. Other variations to 'dbgp=' are accepted,
> > please see xen-command-line.pandoc for more. Remote GDB is also supported
> > with `gdb=dbgp dbgp=xhci`. Note that to see output and/or provide input
> > after dom0 starts, DMA remapping of the host controller must be
> > disabled.
> >
> > Signed-off-by: Connor Davis <connojdavis@xxxxxxxxx>
> > ---
> >   MAINTAINERS                       |    6 +
> >   docs/misc/xen-command-line.pandoc |   19 +-
> >   xen/arch/x86/Kconfig              |    1 -
> >   xen/arch/x86/setup.c              |    1 +
> >   xen/drivers/char/Kconfig          |   15 +
> >   xen/drivers/char/Makefile         |    1 +
> >   xen/drivers/char/xhci-dbc.c       | 1122 +++++++++++++++++++++++++++++
> >   xen/drivers/char/xhci-dbc.h       |  621 ++++++++++++++++
> What is the reason for needing the separate header? Isn't / can't all
> logic (be) contained within xhci-dbc.c? If this is because you clone
> code from elsewhere (as the initial comment in the files seems to
> suggest), it might be a good idea for the description to say so.
> Depending on the origin and possible plans to keep out clone in sync
> down the road, undoing this separation as well as correction of certain
> style issues (which I'm not going to try to spot consistently just yet)
> may then not want requesting. Otoh the files look to have been converted
> to Xen style, so direct syncing may not be a goal.
No reason other than cosmetic, separating "header-ish" things from source.

I can put it all in one .c if that is preferred

> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -714,9 +714,26 @@ Available alternatives, with their meaning, are:
> >   ### dbgp
> >   > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]`
> >   
> > -Specify the USB controller to use, either by instance number (when going
> > +Specify the EHCI USB controller to use, either by instance number (when going
> >   over the PCI busses sequentially) or by PCI device (must be on segment 0).
> >   
> > +If you have a system with an xHCI USB controller that supports the Debug
> > +Capability (DbC), you can use
> > +
> > +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
> > +
> > +To use this, you need a USB3 A/A debugging cable plugged into a SuperSpeed
> > +root port on the target machine. Recent kernels expose the existence of the
> > +DbC at /sys/kernel/debug/usb/xhci/<seg>:<bus>:<slot>.<func>/reg-ext-dbc:00.
> > +Note that to see output and process input after dom0 is started, you need to
> > +ensure that the host controller's DMA is not remapped (e.g. with
> > +dom0-iommu=passthrough).
> This is a relatively bad limitation imo - people would better not get
> used to using passthrough mode, and debugging other IOMMU modes (for
> Dom0) may then be impossible at all.
Why is turning on passthrough mode to debug something bad? Sure

it shouldn't be done in a production deployment, but I don't see the

issue if it is used in a debug environment.

> > --- a/xen/arch/x86/Kconfig
> > +++ b/xen/arch/x86/Kconfig
> > @@ -11,7 +11,6 @@ config X86
> >         select HAS_ALTERNATIVE
> >         select HAS_COMPAT
> >         select HAS_CPUFREQ
> > -       select HAS_EHCI
> Why?
>
> > --- a/xen/drivers/char/Kconfig
> > +++ b/xen/drivers/char/Kconfig
> > @@ -63,6 +63,21 @@ config HAS_SCIF
> >   config HAS_EHCI
> >         bool
> >         depends on X86
> > +        default y if !HAS_XHCI_DBC
> Again, way? The two drivers shouldn't be exclusive of one another.
They both implement the dbgp_op hypercall, so they can't both

be built, otherwise the build fails (as the code stands now with this

patch that is)

> Also please note the mixture of indentation you introduce.
Thanks, will fix
> >         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_DBC
> > +       bool "xHCI Debug Capability driver"
> A setting name HAS_* wouldn't normally have a prompt.
Understood
> > +       depends on X86 && HAS_PCI
> > +       help
> > +         This selects the xHCI Debug Capabilty to be used as a UART.
> > +
> > +config XHCI_FIXMAP_PAGES
> > +        int "Number of fixmap entries to allocate for the xHC"
> > +       depends on HAS_XHCI_DBC
> > +        default 16
> > +        help
> > +          This should equal the size (in 4K pages) of the first 64-bit
> > +          BAR of the host controller in which the DbC is being used.
> Again - please use consistent (in itself as well as with the rest of
> the file) indentation.
>
> Jan

Thanks,

Connor