| Message ID | 403daba6911a3d40e4774b46ba555f6d76b3c249.1660354597.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On 13.08.2022 03:39, Marek Marczykowski-Górecki wrote: > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -724,7 +724,7 @@ Available alternatives, with their meaning, are: > > ### dbgp > > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]` > -> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]` > +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=<bool>|hwdom]` > > 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). > @@ -732,6 +732,19 @@ over the PCI busses sequentially) or by PCI device (must be on segment 0). > Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability. > XHCI driver will wait indefinitely for the debug host to connect - make > sure the cable is connected. > +The `share` option for xhci controls who else can use the controller: > +* `no`: use the controller exclusively for console, even hardware domain > + (dom0) cannot use it > +* `hwdom`: hardware domain may use the controller too, ports not used for debug > + console will be available for normal devices; this is the default > +* `yes`: the controller can be assigned to any domain; it is not safe to assign > + the controller to untrusted domain > + > +Choosing `share=hwdom` (the default) or `share=no` allows a domain to reset the DYM "... or `share=yes` ..." here? > --- a/xen/drivers/char/xhci-dbc.c > +++ b/xen/drivers/char/xhci-dbc.c > @@ -23,6 +23,7 @@ > #include <xen/iommu.h> > #include <xen/mm.h> > #include <xen/param.h> > +#include <xen/rangeset.h> > #include <xen/serial.h> > #include <xen/timer.h> > #include <xen/types.h> > @@ -232,6 +233,14 @@ struct dbc_work_ring { > uint64_t dma; > }; > > +enum xhci_share { > + XHCI_SHARE_NONE = 0, > +#ifdef CONFIG_XHCI_SHARE > + XHCI_SHARE_HWDOM, > + XHCI_SHARE_ANY > +#endif > +}; Hmm, this suggests that Dom0 cannot use the controller without the Kconfig enabled, which I hope is not the case. But I notice that patch 1, which was committed already, still uses pci_ro_device() rather than pci_hide_device() (like ehci-dbgp.c does). > @@ -1128,10 +1181,34 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) > init_timer(&uart->timer, dbc_uart_poll, port, 0); > set_timer(&uart->timer, NOW() + MILLISECS(1)); > > - if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) > - printk(XENLOG_WARNING > - "Failed to mark read-only %pp used for XHCI console\n", > - &uart->dbc.sbdf); > + switch ( uart->dbc.share ) > + { > + case XHCI_SHARE_NONE: > + if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) > + printk(XENLOG_WARNING > + "Failed to mark read-only %pp used for XHCI console\n", > + &uart->dbc.sbdf); > + break; > +#ifdef CONFIG_XHCI_SHARE > + case XHCI_SHARE_HWDOM: > + if ( pci_hide_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) > + printk(XENLOG_WARNING > + "Failed to hide %pp used for XHCI console\n", > + &uart->dbc.sbdf); > + break; > + case XHCI_SHARE_ANY: > + /* Do not hide. */ > + break; > +#endif > + } > +#ifdef CONFIG_X86 > + if ( rangeset_add_range(mmio_ro_ranges, > + PFN_DOWN(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset), > + PFN_UP(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset + > + sizeof(*uart->dbc.dbc_reg)) - 1) ) > + printk(XENLOG_INFO > + "Error while adding MMIO range of device to mmio_ro_ranges\n"); > +#endif I did comment on this last part before. There very minimum that I'd expect to appear here is a comment as to the issue with other elements living on the same page which a domain's driver may actually find a need to write to. As said before - as soon as such a report would surface, we'd likely need to add write emulation support for the leading/traling parts of the page(s) that Xen doesn't use itself. > @@ -1202,13 +1279,18 @@ void __init xhci_dbc_uart_init(void) > { > struct dbc_uart *uart = &dbc_uart; > struct dbc *dbc = &uart->dbc; > - const char *e; > + const char *e, *opt; > > if ( strncmp(opt_dbgp, "xhci", 4) ) > return; > > memset(dbc, 0, sizeof(*dbc)); > > +#ifdef CONFIG_XHCI_SHARE > + dbc->share = XHCI_SHARE_HWDOM; > +#endif I think it would be best if the default value was "0"; I can see though that "NONE" being zero also makse sense, if the enum was to be used in boolean context (which afaics it currently isn't). > + e = &opt_dbgp[4]; > if ( isdigit(opt_dbgp[4]) ) > { > dbc->xhc_num = simple_strtoul(opt_dbgp + 4, &e, 10); > @@ -1218,7 +1300,7 @@ void __init xhci_dbc_uart_init(void) > unsigned int bus, slot, func; > > e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func); > - if ( !e || *e ) > + if ( !e || (*e && *e != ',') ) > { > printk(XENLOG_ERR > "Invalid dbgp= PCI device spec: '%s'\n", > @@ -1228,6 +1310,41 @@ void __init xhci_dbc_uart_init(void) > > dbc->sbdf = PCI_SBDF(0, bus, slot, func); > } > + opt = e; > + > +#ifdef CONFIG_XHCI_SHARE > + /* other options */ > + while ( *opt == ',' ) > + { > + opt++; > + e = strchr(opt, ','); > + if ( !e ) > + e = strchr(opt, '\0'); > + > + if ( !strncmp(opt, "share=", 6) ) > + { > + int val = parse_bool(opt + 6, e); > + if ( val == -1 && !cmdline_strcmp(opt + 6, "hwdom") ) Nit: Blank line please between declaration(s) and statement(s). Any reason you're using parse_bool() and not parse_boolean() here? That would save you the open-coded strncmp() afaict. Finally a remark seeing the opt_dbgp use here and the identically named option in ehci-dbgp.c, taken together with your multiple- serial-consoles patch: Since the two option consumers are now different, they can't sensibly coexist anymore. There were issues already before - it doesn't seem to be possible this way to run EHCI and XHCI based consoles in parallel. (An exceptional case would be if <integer> for both was intended to be same number.) IOW I think one of the options needs renaming; it was a mistake of mine to not have pointed this out before committing patch 1. Following the name of the source file as well as e.g. the title here - maybe "dbc="? Jan
On Wed, Aug 17, 2022 at 05:08:35PM +0200, Jan Beulich wrote: > On 13.08.2022 03:39, Marek Marczykowski-Górecki wrote: > > --- a/docs/misc/xen-command-line.pandoc > > +++ b/docs/misc/xen-command-line.pandoc > > @@ -724,7 +724,7 @@ Available alternatives, with their meaning, are: > > > > ### dbgp > > > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]` > > -> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]` > > +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=<bool>|hwdom]` > > > > 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). > > @@ -732,6 +732,19 @@ over the PCI busses sequentially) or by PCI device (must be on segment 0). > > Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability. > > XHCI driver will wait indefinitely for the debug host to connect - make > > sure the cable is connected. > > +The `share` option for xhci controls who else can use the controller: > > +* `no`: use the controller exclusively for console, even hardware domain > > + (dom0) cannot use it > > +* `hwdom`: hardware domain may use the controller too, ports not used for debug > > + console will be available for normal devices; this is the default > > +* `yes`: the controller can be assigned to any domain; it is not safe to assign > > + the controller to untrusted domain > > + > > +Choosing `share=hwdom` (the default) or `share=no` allows a domain to reset the > > DYM "... or `share=yes` ..." here? Yes. > > --- a/xen/drivers/char/xhci-dbc.c > > +++ b/xen/drivers/char/xhci-dbc.c > > @@ -23,6 +23,7 @@ > > #include <xen/iommu.h> > > #include <xen/mm.h> > > #include <xen/param.h> > > +#include <xen/rangeset.h> > > #include <xen/serial.h> > > #include <xen/timer.h> > > #include <xen/types.h> > > @@ -232,6 +233,14 @@ struct dbc_work_ring { > > uint64_t dma; > > }; > > > > +enum xhci_share { > > + XHCI_SHARE_NONE = 0, > > +#ifdef CONFIG_XHCI_SHARE > > + XHCI_SHARE_HWDOM, > > + XHCI_SHARE_ANY > > +#endif > > +}; > > Hmm, this suggests that Dom0 cannot use the controller without the Kconfig > enabled, which I hope is not the case. It is the case, because you requested reset quirk to be behind "experimental" tag in kconfig. This quirk is (currently) necessary even if just dom0 uses the controller. I'm happy to include the quirk by default, but I got impression you wouldn't accept it. And I'd rather avoid marking the whole driver as "experimental" (which hides it unless you select UNSUPPORTED) just because of a small code necessary to share it with dom0. > But I notice that patch 1, which > was committed already, still uses pci_ro_device() rather than > pci_hide_device() (like ehci-dbgp.c does). > > > @@ -1128,10 +1181,34 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) > > init_timer(&uart->timer, dbc_uart_poll, port, 0); > > set_timer(&uart->timer, NOW() + MILLISECS(1)); > > > > - if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) > > - printk(XENLOG_WARNING > > - "Failed to mark read-only %pp used for XHCI console\n", > > - &uart->dbc.sbdf); > > + switch ( uart->dbc.share ) > > + { > > + case XHCI_SHARE_NONE: > > + if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) > > + printk(XENLOG_WARNING > > + "Failed to mark read-only %pp used for XHCI console\n", > > + &uart->dbc.sbdf); > > + break; > > +#ifdef CONFIG_XHCI_SHARE > > + case XHCI_SHARE_HWDOM: > > + if ( pci_hide_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) > > + printk(XENLOG_WARNING > > + "Failed to hide %pp used for XHCI console\n", > > + &uart->dbc.sbdf); > > + break; > > + case XHCI_SHARE_ANY: > > + /* Do not hide. */ > > + break; > > +#endif > > + } > > +#ifdef CONFIG_X86 > > + if ( rangeset_add_range(mmio_ro_ranges, > > + PFN_DOWN(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset), > > + PFN_UP(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset + > > + sizeof(*uart->dbc.dbc_reg)) - 1) ) > > + printk(XENLOG_INFO > > + "Error while adding MMIO range of device to mmio_ro_ranges\n"); > > +#endif > > I did comment on this last part before. There very minimum that I'd expect > to appear here is a comment as to the issue with other elements living on > the same page which a domain's driver may actually find a need to write to. > As said before - as soon as such a report would surface, we'd likely need > to add write emulation support for the leading/traling parts of the page(s) > that Xen doesn't use itself. I did included paragraph in the commit message: | In any case, to avoid Linux messing with the DbC, mark this MMIO area as | read-only. This might cause issues for Linux's driver (if it tries to | write something on the same page - like anoter xcap), but makes Xen's | use safe. In practice, as of Linux 5.18, it seems to work without | issues. Do you want this as a code comment too? > > @@ -1202,13 +1279,18 @@ void __init xhci_dbc_uart_init(void) > > { > > struct dbc_uart *uart = &dbc_uart; > > struct dbc *dbc = &uart->dbc; > > - const char *e; > > + const char *e, *opt; > > > > if ( strncmp(opt_dbgp, "xhci", 4) ) > > return; > > > > memset(dbc, 0, sizeof(*dbc)); > > > > +#ifdef CONFIG_XHCI_SHARE > > + dbc->share = XHCI_SHARE_HWDOM; > > +#endif > > I think it would be best if the default value was "0"; I can see though > that "NONE" being zero also makse sense, if the enum was to be used in > boolean context (which afaics it currently isn't). > > > + e = &opt_dbgp[4]; > > if ( isdigit(opt_dbgp[4]) ) > > { > > dbc->xhc_num = simple_strtoul(opt_dbgp + 4, &e, 10); > > @@ -1218,7 +1300,7 @@ void __init xhci_dbc_uart_init(void) > > unsigned int bus, slot, func; > > > > e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func); > > - if ( !e || *e ) > > + if ( !e || (*e && *e != ',') ) > > { > > printk(XENLOG_ERR > > "Invalid dbgp= PCI device spec: '%s'\n", > > @@ -1228,6 +1310,41 @@ void __init xhci_dbc_uart_init(void) > > > > dbc->sbdf = PCI_SBDF(0, bus, slot, func); > > } > > + opt = e; > > + > > +#ifdef CONFIG_XHCI_SHARE > > + /* other options */ > > + while ( *opt == ',' ) > > + { > > + opt++; > > + e = strchr(opt, ','); > > + if ( !e ) > > + e = strchr(opt, '\0'); > > + > > + if ( !strncmp(opt, "share=", 6) ) > > + { > > + int val = parse_bool(opt + 6, e); > > + if ( val == -1 && !cmdline_strcmp(opt + 6, "hwdom") ) > > Nit: Blank line please between declaration(s) and statement(s). > > Any reason you're using parse_bool() and not parse_boolean() here? > That would save you the open-coded strncmp() afaict. I can probably use parse_boolean() too, but then handling "hwdom" variant would be a bit weird. I could skip 'share=hwdom' parsing at all, since that's default if the kconfig option is enabled, but I'm not sure if that's a good idea. > Finally a remark seeing the opt_dbgp use here and the identically > named option in ehci-dbgp.c, taken together with your multiple- > serial-consoles patch: Since the two option consumers are now > different, they can't sensibly coexist anymore. There were issues > already before - it doesn't seem to be possible this way to run > EHCI and XHCI based consoles in parallel. (An exceptional case > would be if <integer> for both was intended to be same number.) > IOW I think one of the options needs renaming; it was a mistake of > mine to not have pointed this out before committing patch 1. > Following the name of the source file as well as e.g. the title > here - maybe "dbc="? Yes, I can rename the option here. That requires also registering new SERHND_* and inventing new value for console= parameter (implemented in serial_parse_handle()). "dbc" there too?
On 17.08.2022 17:27, Marek Marczykowski-Górecki wrote: > On Wed, Aug 17, 2022 at 05:08:35PM +0200, Jan Beulich wrote: >> On 13.08.2022 03:39, Marek Marczykowski-Górecki wrote: >>> --- a/xen/drivers/char/xhci-dbc.c >>> +++ b/xen/drivers/char/xhci-dbc.c >>> @@ -23,6 +23,7 @@ >>> #include <xen/iommu.h> >>> #include <xen/mm.h> >>> #include <xen/param.h> >>> +#include <xen/rangeset.h> >>> #include <xen/serial.h> >>> #include <xen/timer.h> >>> #include <xen/types.h> >>> @@ -232,6 +233,14 @@ struct dbc_work_ring { >>> uint64_t dma; >>> }; >>> >>> +enum xhci_share { >>> + XHCI_SHARE_NONE = 0, >>> +#ifdef CONFIG_XHCI_SHARE >>> + XHCI_SHARE_HWDOM, >>> + XHCI_SHARE_ANY >>> +#endif >>> +}; >> >> Hmm, this suggests that Dom0 cannot use the controller without the Kconfig >> enabled, which I hope is not the case. > > It is the case, because you requested reset quirk to be behind > "experimental" tag in kconfig. This quirk is (currently) necessary even > if just dom0 uses the controller. > I'm happy to include the quirk by default, but I got impression you > wouldn't accept it. And I'd rather avoid marking the whole driver as > "experimental" (which hides it unless you select UNSUPPORTED) just > because of a small code necessary to share it with dom0. Hmm, well, I'm not happy about that quirk (and I did point out how it's done for EHCI), but I agree we don't want to "hide" the entire drivers, and I continue to think Dom0 should, by default, be able to use the device (to the extent possible). So I guess I have no choice but to accept the use of this quirk by default. >>> @@ -1128,10 +1181,34 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) >>> init_timer(&uart->timer, dbc_uart_poll, port, 0); >>> set_timer(&uart->timer, NOW() + MILLISECS(1)); >>> >>> - if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) >>> - printk(XENLOG_WARNING >>> - "Failed to mark read-only %pp used for XHCI console\n", >>> - &uart->dbc.sbdf); >>> + switch ( uart->dbc.share ) >>> + { >>> + case XHCI_SHARE_NONE: >>> + if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) >>> + printk(XENLOG_WARNING >>> + "Failed to mark read-only %pp used for XHCI console\n", >>> + &uart->dbc.sbdf); >>> + break; >>> +#ifdef CONFIG_XHCI_SHARE >>> + case XHCI_SHARE_HWDOM: >>> + if ( pci_hide_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) >>> + printk(XENLOG_WARNING >>> + "Failed to hide %pp used for XHCI console\n", >>> + &uart->dbc.sbdf); >>> + break; >>> + case XHCI_SHARE_ANY: >>> + /* Do not hide. */ >>> + break; >>> +#endif >>> + } >>> +#ifdef CONFIG_X86 >>> + if ( rangeset_add_range(mmio_ro_ranges, >>> + PFN_DOWN(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset), >>> + PFN_UP(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset + >>> + sizeof(*uart->dbc.dbc_reg)) - 1) ) >>> + printk(XENLOG_INFO >>> + "Error while adding MMIO range of device to mmio_ro_ranges\n"); >>> +#endif >> >> I did comment on this last part before. There very minimum that I'd expect >> to appear here is a comment as to the issue with other elements living on >> the same page which a domain's driver may actually find a need to write to. >> As said before - as soon as such a report would surface, we'd likely need >> to add write emulation support for the leading/traling parts of the page(s) >> that Xen doesn't use itself. > > I did included paragraph in the commit message: > | In any case, to avoid Linux messing with the DbC, mark this MMIO area as > | read-only. This might cause issues for Linux's driver (if it tries to > | write something on the same page - like anoter xcap), but makes Xen's > | use safe. In practice, as of Linux 5.18, it seems to work without > | issues. > > Do you want this as a code comment too? A shorter form thereof perhaps, but yes, absolutely. Getting at that information shouldn't require locating the commit. >>> @@ -1228,6 +1310,41 @@ void __init xhci_dbc_uart_init(void) >>> >>> dbc->sbdf = PCI_SBDF(0, bus, slot, func); >>> } >>> + opt = e; >>> + >>> +#ifdef CONFIG_XHCI_SHARE >>> + /* other options */ >>> + while ( *opt == ',' ) >>> + { >>> + opt++; >>> + e = strchr(opt, ','); >>> + if ( !e ) >>> + e = strchr(opt, '\0'); >>> + >>> + if ( !strncmp(opt, "share=", 6) ) >>> + { >>> + int val = parse_bool(opt + 6, e); >>> + if ( val == -1 && !cmdline_strcmp(opt + 6, "hwdom") ) >> >> Nit: Blank line please between declaration(s) and statement(s). >> >> Any reason you're using parse_bool() and not parse_boolean() here? >> That would save you the open-coded strncmp() afaict. > > I can probably use parse_boolean() too, but then handling "hwdom" > variant would be a bit weird. I could skip 'share=hwdom' parsing at all, > since that's default if the kconfig option is enabled, but I'm not sure > if that's a good idea. May I suggest that you take a look at xen/arch/x86/spec-ctrl.c's uses of parse_boolean()? Maybe you consider some of those "weird" as well, but it's not like these have been around forever and are now deemed "bad". >> Finally a remark seeing the opt_dbgp use here and the identically >> named option in ehci-dbgp.c, taken together with your multiple- >> serial-consoles patch: Since the two option consumers are now >> different, they can't sensibly coexist anymore. There were issues >> already before - it doesn't seem to be possible this way to run >> EHCI and XHCI based consoles in parallel. (An exceptional case >> would be if <integer> for both was intended to be same number.) >> IOW I think one of the options needs renaming; it was a mistake of >> mine to not have pointed this out before committing patch 1. >> Following the name of the source file as well as e.g. the title >> here - maybe "dbc="? > > Yes, I can rename the option here. That requires also registering new > SERHND_* and inventing new value for console= parameter (implemented in > serial_parse_handle()). "dbc" there too? Probably best to be halfway consistent with the naming, yes. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 64441a2679b8..c618f93a8fd5 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -724,7 +724,7 @@ Available alternatives, with their meaning, are: ### dbgp > `= ehci[ <integer> | @pci<bus>:<slot>.<func> ]` -> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]` +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,share=<bool>|hwdom]` 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). @@ -732,6 +732,19 @@ over the PCI busses sequentially) or by PCI device (must be on segment 0). Use `ehci` for EHCI debug port, use `xhci` for XHCI debug capability. XHCI driver will wait indefinitely for the debug host to connect - make sure the cable is connected. +The `share` option for xhci controls who else can use the controller: +* `no`: use the controller exclusively for console, even hardware domain + (dom0) cannot use it +* `hwdom`: hardware domain may use the controller too, ports not used for debug + console will be available for normal devices; this is the default +* `yes`: the controller can be assigned to any domain; it is not safe to assign + the controller to untrusted domain + +Choosing `share=hwdom` (the default) or `share=no` allows a domain to reset the +controller, which may cause small portion of the console output to be lost. +The `share` option is available only if CONFIG_XHCI_SHARE is enabled. + +The `share=yes` configuration is not security supported. ### debug_stack_lines > `= <integer>` diff --git a/xen/drivers/char/Kconfig b/xen/drivers/char/Kconfig index 7b5ff0c414ec..a04c28fbff78 100644 --- a/xen/drivers/char/Kconfig +++ b/xen/drivers/char/Kconfig @@ -104,3 +104,14 @@ config XHCI Enabling this option makes Xen use extra ~230KiB memory, even if XHCI UART is not selected. If you have an x86 based system with USB3, say Y. + +config XHCI_SHARE + bool "Allow sharing XHCI when DbC UART is active (EXPERIMENTAL)" + depends on XHCI && UNSUPPORTED + help + This enables sharing XHCI with another domain (hardware domain, or another + domU) while Xen uses XHCI DbC as a console. When this option is + enabled, domain can perform controller reset, which may cause small portion + of console output to be lost. + + Sharing XHCI with non-dom0 is not security supported. diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index a94a48a48989..0ac418280d19 100644 --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -23,6 +23,7 @@ #include <xen/iommu.h> #include <xen/mm.h> #include <xen/param.h> +#include <xen/rangeset.h> #include <xen/serial.h> #include <xen/timer.h> #include <xen/types.h> @@ -232,6 +233,14 @@ struct dbc_work_ring { uint64_t dma; }; +enum xhci_share { + XHCI_SHARE_NONE = 0, +#ifdef CONFIG_XHCI_SHARE + XHCI_SHARE_HWDOM, + XHCI_SHARE_ANY +#endif +}; + struct dbc { struct dbc_reg __iomem *dbc_reg; struct xhci_dbc_ctx *dbc_ctx; @@ -249,6 +258,7 @@ struct dbc { void __iomem *xhc_mmio; bool open; + enum xhci_share share; unsigned int xhc_num; /* look for n-th xhc */ }; @@ -951,13 +961,56 @@ static bool __init dbc_open(struct dbc *dbc) } /* - * Ensure DbC is still running, handle events, and possibly re-enable if cable - * was re-plugged. Returns true if DbC is operational. + * Ensure DbC is still running, handle events, and possibly + * re-enable/re-configure if cable was re-plugged or controller was reset. + * Returns true if DbC is operational. */ static bool dbc_ensure_running(struct dbc *dbc) { struct dbc_reg *reg = dbc->dbc_reg; uint32_t ctrl; + uint16_t cmd; + + if ( dbc->share != XHCI_SHARE_NONE ) + { + /* + * Re-enable memory decoding and later bus mastering, if dom0 (or + * other) disabled it in the meantime. + */ + cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND); + if ( !(cmd & PCI_COMMAND_MEMORY) ) + { + cmd |= PCI_COMMAND_MEMORY; + pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd); + } + + /* + * FIXME: Make Linux coordinate XHCI reset, so the DbC driver can + * prepare for it properly, instead of only detecting it after the + * fact. See EHCI driver for similar handling. + */ + if ( dbc->open && !(readl(®->ctrl) & (1U << DBC_CTRL_DCE)) ) + { + if ( !dbc_init_dbc(dbc) ) + return false; + + dbc_init_work_ring(dbc, &dbc->dbc_owork); + dbc_enable_dbc(dbc); + } + else + { + /* + * dbc_init_dbc() takes care about it, so check only if it wasn't + * called. + */ + cmd = pci_conf_read16(dbc->sbdf, PCI_COMMAND); + if ( !(cmd & PCI_COMMAND_MASTER) ) + { + cmd |= PCI_COMMAND_MASTER; + pci_conf_write16(dbc->sbdf, PCI_COMMAND, cmd); + } + } + } dbc_pop_events(dbc); @@ -1128,10 +1181,34 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) init_timer(&uart->timer, dbc_uart_poll, port, 0); set_timer(&uart->timer, NOW() + MILLISECS(1)); - if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) - printk(XENLOG_WARNING - "Failed to mark read-only %pp used for XHCI console\n", - &uart->dbc.sbdf); + switch ( uart->dbc.share ) + { + case XHCI_SHARE_NONE: + if ( pci_ro_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) + printk(XENLOG_WARNING + "Failed to mark read-only %pp used for XHCI console\n", + &uart->dbc.sbdf); + break; +#ifdef CONFIG_XHCI_SHARE + case XHCI_SHARE_HWDOM: + if ( pci_hide_device(0, uart->dbc.sbdf.bus, uart->dbc.sbdf.devfn) ) + printk(XENLOG_WARNING + "Failed to hide %pp used for XHCI console\n", + &uart->dbc.sbdf); + break; + case XHCI_SHARE_ANY: + /* Do not hide. */ + break; +#endif + } +#ifdef CONFIG_X86 + if ( rangeset_add_range(mmio_ro_ranges, + PFN_DOWN(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset), + PFN_UP(uart->dbc.xhc_mmio_phys + uart->dbc.xhc_dbc_offset + + sizeof(*uart->dbc.dbc_reg)) - 1) ) + printk(XENLOG_INFO + "Error while adding MMIO range of device to mmio_ro_ranges\n"); +#endif } static int cf_check dbc_uart_tx_ready(struct serial_port *port) @@ -1202,13 +1279,18 @@ void __init xhci_dbc_uart_init(void) { struct dbc_uart *uart = &dbc_uart; struct dbc *dbc = &uart->dbc; - const char *e; + const char *e, *opt; if ( strncmp(opt_dbgp, "xhci", 4) ) return; memset(dbc, 0, sizeof(*dbc)); +#ifdef CONFIG_XHCI_SHARE + dbc->share = XHCI_SHARE_HWDOM; +#endif + + e = &opt_dbgp[4]; if ( isdigit(opt_dbgp[4]) ) { dbc->xhc_num = simple_strtoul(opt_dbgp + 4, &e, 10); @@ -1218,7 +1300,7 @@ void __init xhci_dbc_uart_init(void) unsigned int bus, slot, func; e = parse_pci(opt_dbgp + 8, NULL, &bus, &slot, &func); - if ( !e || *e ) + if ( !e || (*e && *e != ',') ) { printk(XENLOG_ERR "Invalid dbgp= PCI device spec: '%s'\n", @@ -1228,6 +1310,41 @@ void __init xhci_dbc_uart_init(void) dbc->sbdf = PCI_SBDF(0, bus, slot, func); } + opt = e; + +#ifdef CONFIG_XHCI_SHARE + /* other options */ + while ( *opt == ',' ) + { + opt++; + e = strchr(opt, ','); + if ( !e ) + e = strchr(opt, '\0'); + + if ( !strncmp(opt, "share=", 6) ) + { + int val = parse_bool(opt + 6, e); + if ( val == -1 && !cmdline_strcmp(opt + 6, "hwdom") ) + dbc->share = XHCI_SHARE_HWDOM; + else if ( val == 0 ) + dbc->share = XHCI_SHARE_NONE; + else if ( val == 1 ) + dbc->share = XHCI_SHARE_ANY; + else + break; + } + else + break; + + opt = e; + } +#endif + + if ( *opt ) + { + printk(XENLOG_ERR "Invalid dbgp= parameters: '%s'\n", opt); + return; + } dbc->dbc_ctx = &dbc_dma_bufs.ctx; dbc->dbc_erst = &dbc_dma_bufs.erst;
That's possible, because the capability was designed specifically to allow separate driver handle it, in parallel to unmodified xhci driver (separate set of registers, pretending the port is "disconnected" for the main xhci driver etc). It works with Linux dom0, although requires an awful hack - re-enabling bus mastering behind dom0's backs. Linux driver does similar thing - see drivers/usb/early/xhci-dbc.c:xdbc_handle_events(). When controller sharing is enabled in kconfig (option marked as experimental), dom0 is allowed to use the controller even if Xen uses it for debug console. Additionally, option `dbgp=xhci,share=` is available to either prevent even dom0 from using it (`no` value), or allow any domain using it (`any` value). In any case, to avoid Linux messing with the DbC, mark this MMIO area as read-only. This might cause issues for Linux's driver (if it tries to write something on the same page - like anoter xcap), but makes Xen's use safe. In practice, as of Linux 5.18, it seems to work without issues. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v4: - minor fix for cmdline parsing - make sharing opt-in build time, with option marked as EXPERIMENTAL - change cmdline syntax to share=<bool>|hwdom - make share=hwdom default (if enabled build-time) Changes in v3: - adjust for xhci-dbc rename - adjust for dbc_ensure_running() split - wrap long lines - add runtime option for sharing USB controller --- docs/misc/xen-command-line.pandoc | 15 ++- xen/drivers/char/Kconfig | 11 +++- xen/drivers/char/xhci-dbc.c | 133 +++++++++++++++++++++++++++++-- 3 files changed, 150 insertions(+), 9 deletions(-)