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

[Connor Davis <connojdavis@gmail.com>]

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@gmail.com>
>> ---
>>   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