| Message ID | f9c4f0df804ed406dea0e480614a033d5bd434c6.1662124370.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On 02.09.2022 15:17, Marek Marczykowski-Górecki wrote: > This allows configuring EHCI and XHCI consoles separately, > simultaneously. > > This changes string_param() to custom_param() in both ehci and xhci > drivers. Both drivers parse only values applicable to them. > > While at it, drop unnecessary memset() of a static variable. Are you sure of this? What if there are two "dbgp=xhci,..." options on the command line, the latter intended to override the earlier but malformed. Then ->enabled would be left set from parsing the first instance, afaict. > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -409,7 +409,7 @@ The following are examples of correct specifications: > Specify the size of the console ring buffer. > > ### console > -> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | none ]` > +> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | xhci | none ]` Personally I consider "dbc" more in line with "dbgp", but I'm okay with "xhci". We may want to allow for "ehci" then as an alias of "dbgp", though (in a separate, later patch). > --- a/xen/drivers/char/ehci-dbgp.c > +++ b/xen/drivers/char/ehci-dbgp.c > @@ -1464,7 +1464,18 @@ static struct uart_driver __read_mostly ehci_dbgp_driver = { > static struct ehci_dbgp ehci_dbgp = { .state = dbgp_unsafe, .phys_port = 1 }; > > static char __initdata opt_dbgp[30]; > -string_param("dbgp", opt_dbgp); > + > +static int __init parse_ehci_dbgp(const char *opt) > +{ > + if ( strncmp(opt, "ehci", 4) ) > + return 0; > + > + strlcpy(opt_dbgp, opt, sizeof(opt_dbgp)); > + > + return 0; > +} > + > +custom_param("dbgp", parse_ehci_dbgp); We commonly don't put a blank line between the function and this construct. (Same again further down then.) > --- a/xen/drivers/char/xhci-dbc.c > +++ b/xen/drivers/char/xhci-dbc.c > @@ -245,6 +245,7 @@ struct dbc { > uint64_t xhc_dbc_offset; > void __iomem *xhc_mmio; > > + bool enable; /* whether dbgp=xhci was set at all */ In dbc_init_xhc() there's an assumption that the "sbdf" field is always non-zero. Do you really need this separate flag then? Jan
On Tue, Sep 06, 2022 at 05:07:27PM +0200, Jan Beulich wrote: > On 02.09.2022 15:17, Marek Marczykowski-Górecki wrote: > > This allows configuring EHCI and XHCI consoles separately, > > simultaneously. > > > > This changes string_param() to custom_param() in both ehci and xhci > > drivers. Both drivers parse only values applicable to them. > > > > While at it, drop unnecessary memset() of a static variable. > > Are you sure of this? What if there are two "dbgp=xhci,..." options > on the command line, the latter intended to override the earlier but > malformed. Then ->enabled would be left set from parsing the first > instance, afaict. Right. > > --- a/docs/misc/xen-command-line.pandoc > > +++ b/docs/misc/xen-command-line.pandoc > > @@ -409,7 +409,7 @@ The following are examples of correct specifications: > > Specify the size of the console ring buffer. > > > > ### console > > -> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | none ]` > > +> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | xhci | none ]` > > Personally I consider "dbc" more in line with "dbgp", but I'm okay > with "xhci". We may want to allow for "ehci" then as an alias of > "dbgp", though (in a separate, later patch). I've changed "dbc" to "xhci", as "dbc" isn't really surfaced to the user anywhere else. As in - it requires some deeper knowledge to draw a connection between console=dbc and dbgp=xhci. And yes, when going this way, "ehci" alias would make sense. > > > --- a/xen/drivers/char/ehci-dbgp.c > > +++ b/xen/drivers/char/ehci-dbgp.c > > @@ -1464,7 +1464,18 @@ static struct uart_driver __read_mostly ehci_dbgp_driver = { > > static struct ehci_dbgp ehci_dbgp = { .state = dbgp_unsafe, .phys_port = 1 }; > > > > static char __initdata opt_dbgp[30]; > > -string_param("dbgp", opt_dbgp); > > + > > +static int __init parse_ehci_dbgp(const char *opt) > > +{ > > + if ( strncmp(opt, "ehci", 4) ) > > + return 0; > > + > > + strlcpy(opt_dbgp, opt, sizeof(opt_dbgp)); > > + > > + return 0; > > +} > > + > > +custom_param("dbgp", parse_ehci_dbgp); > > We commonly don't put a blank line between the function and this > construct. (Same again further down then.) > > > --- a/xen/drivers/char/xhci-dbc.c > > +++ b/xen/drivers/char/xhci-dbc.c > > @@ -245,6 +245,7 @@ struct dbc { > > uint64_t xhc_dbc_offset; > > void __iomem *xhc_mmio; > > > > + bool enable; /* whether dbgp=xhci was set at all */ > > In dbc_init_xhc() there's an assumption that the "sbdf" field is > always non-zero. Do you really need this separate flag then? Not really, sbdf == 0 means "find Nth xhci", where N=xhc_num+1 (and xhc_num can be zero too). See the "if" at the very top of dbc_init_xhc().
On 06.09.2022 17:46, Marek Marczykowski-Górecki wrote: > On Tue, Sep 06, 2022 at 05:07:27PM +0200, Jan Beulich wrote: >> On 02.09.2022 15:17, Marek Marczykowski-Górecki wrote: >>> --- a/xen/drivers/char/xhci-dbc.c >>> +++ b/xen/drivers/char/xhci-dbc.c >>> @@ -245,6 +245,7 @@ struct dbc { >>> uint64_t xhc_dbc_offset; >>> void __iomem *xhc_mmio; >>> >>> + bool enable; /* whether dbgp=xhci was set at all */ >> >> In dbc_init_xhc() there's an assumption that the "sbdf" field is >> always non-zero. Do you really need this separate flag then? > > Not really, sbdf == 0 means "find Nth xhci", where N=xhc_num+1 (and > xhc_num can be zero too). See the "if" at the very top of > dbc_init_xhc(). Oh, I see. I'm sorry for mis-reading that code. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 9a79385a3712..c8b07042f58e 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -409,7 +409,7 @@ The following are examples of correct specifications: Specify the size of the console ring buffer. ### console -> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | none ]` +> `= List of [ vga | com1[H,L] | com2[H,L] | pv | dbgp | xhci | none ]` > Default: `console=com1,vga` @@ -428,7 +428,9 @@ cleared. This allows a single port to be shared by two subsystems `pv` indicates that Xen should use Xen's PV console. This option is only available when used together with `pv-in-pvh`. -`dbgp` indicates that Xen should use a USB debug port. +`dbgp` indicates that Xen should use a USB2 debug port. + +`xhci` indicates that Xen should use a USB3 debug port. `none` indicates that Xen should not use a console. This option only makes sense on its own. diff --git a/xen/drivers/char/ehci-dbgp.c b/xen/drivers/char/ehci-dbgp.c index 92c588ec0aa3..b1794ed64c7b 100644 --- a/xen/drivers/char/ehci-dbgp.c +++ b/xen/drivers/char/ehci-dbgp.c @@ -1464,7 +1464,18 @@ static struct uart_driver __read_mostly ehci_dbgp_driver = { static struct ehci_dbgp ehci_dbgp = { .state = dbgp_unsafe, .phys_port = 1 }; static char __initdata opt_dbgp[30]; -string_param("dbgp", opt_dbgp); + +static int __init parse_ehci_dbgp(const char *opt) +{ + if ( strncmp(opt, "ehci", 4) ) + return 0; + + strlcpy(opt_dbgp, opt, sizeof(opt_dbgp)); + + return 0; +} + +custom_param("dbgp", parse_ehci_dbgp); void __init ehci_dbgp_init(void) { @@ -1472,7 +1483,7 @@ void __init ehci_dbgp_init(void) u32 debug_port, offset, bar_val; const char *e; - if ( strncmp(opt_dbgp, "ehci", 4) ) + if ( !opt_dbgp[0] ) return; if ( isdigit(opt_dbgp[4]) || !opt_dbgp[4] ) diff --git a/xen/drivers/char/serial.c b/xen/drivers/char/serial.c index 47899222cef8..9d9445039232 100644 --- a/xen/drivers/char/serial.c +++ b/xen/drivers/char/serial.c @@ -311,6 +311,12 @@ int __init serial_parse_handle(const char *conf) goto common; } + if ( !strncmp(conf, "xhci", 4) && (!conf[4] || conf[4] == ',') ) + { + handle = SERHND_XHCI; + goto common; + } + if ( !strncmp(conf, "dtuart", 6) ) { handle = SERHND_DTUART; diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index ca7d4a62139e..8da76282259a 100644 --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -245,6 +245,7 @@ struct dbc { uint64_t xhc_dbc_offset; void __iomem *xhc_mmio; + bool enable; /* whether dbgp=xhci was set at all */ bool open; unsigned int xhc_num; /* look for n-th xhc */ }; @@ -1058,20 +1059,14 @@ static struct xhci_dbc_ctx ctx __aligned(16); static uint8_t out_wrk_buf[DBC_WORK_RING_CAP]; static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT]; -static char __initdata opt_dbgp[30]; - -string_param("dbgp", opt_dbgp); - -void __init xhci_dbc_uart_init(void) +static int __init xhci_parse_dbgp(const char *opt_dbgp) { struct dbc_uart *uart = &dbc_uart; struct dbc *dbc = &uart->dbc; const char *e; if ( strncmp(opt_dbgp, "xhci", 4) ) - return; - - memset(dbc, 0, sizeof(*dbc)); + return 0; if ( isdigit(opt_dbgp[4]) ) { @@ -1087,12 +1082,27 @@ void __init xhci_dbc_uart_init(void) printk(XENLOG_ERR "Invalid dbgp= PCI device spec: '%s'\n", opt_dbgp + 8); - return; + return -EINVAL; } dbc->sbdf = PCI_SBDF(0, bus, slot, func); } + dbc->enable = true; + + return 0; +} + +custom_param("dbgp", xhci_parse_dbgp); + +void __init xhci_dbc_uart_init(void) +{ + struct dbc_uart *uart = &dbc_uart; + struct dbc *dbc = &uart->dbc; + + if ( !dbc->enable ) + return; + dbc->dbc_ctx = &ctx; dbc->dbc_erst = &erst; dbc->dbc_ering.trb = evt_trb; @@ -1102,7 +1112,7 @@ void __init xhci_dbc_uart_init(void) dbc->dbc_str = str_buf; if ( dbc_open(dbc) ) - serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart); + serial_register_uart(SERHND_XHCI, &dbc_uart_driver, &dbc_uart); } #ifdef DBC_DEBUG diff --git a/xen/include/xen/serial.h b/xen/include/xen/serial.h index 4cd4ae5e6f1c..f0aff7ea7661 100644 --- a/xen/include/xen/serial.h +++ b/xen/include/xen/serial.h @@ -91,10 +91,11 @@ struct uart_driver { }; /* 'Serial handles' are composed from the following fields. */ -#define SERHND_IDX (3<<0) /* COM1, COM2, DBGP, DTUART? */ +#define SERHND_IDX (3<<0) /* COM1, COM2, DBGP, XHCI, DTUART? */ # define SERHND_COM1 (0<<0) # define SERHND_COM2 (1<<0) # define SERHND_DBGP (2<<0) +# define SERHND_XHCI (3<<0) # define SERHND_DTUART (0<<0) /* Steal SERHND_COM1 value */ #define SERHND_HI (1<<2) /* Mux/demux each transferred char by MSB. */ #define SERHND_LO (1<<3) /* Ditto, except that the MSB is cleared. */
This allows configuring EHCI and XHCI consoles separately, simultaneously. This changes string_param() to custom_param() in both ehci and xhci drivers. Both drivers parse only values applicable to them. While at it, drop unnecessary memset() of a static variable. Suggested-by: Jan Beulich <jbeulich@suse.com> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v6: - keep dbgp=xhci, but use custom_param() to parse multiple values separately - modify ehci-dbgp to use custom_param() too - change console=dbc to console=xhci, as 'dbc' doesn't appear in any other option anymore - update comment in serial.h new in v5 --- docs/misc/xen-command-line.pandoc | 6 ++++-- xen/drivers/char/ehci-dbgp.c | 15 +++++++++++++-- xen/drivers/char/serial.c | 6 ++++++ xen/drivers/char/xhci-dbc.c | 30 ++++++++++++++++++++---------- xen/include/xen/serial.h | 3 ++- 5 files changed, 45 insertions(+), 15 deletions(-)