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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

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

Also please note the mixture of indentation you introduce.

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

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