Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
On 05.08.2022 17:49, Marek Marczykowski-Górecki wrote:
> On Fri, Aug 05, 2022 at 10:15:59AM +0200, Jan Beulich wrote:
>> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
>>> 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.
>>
>> Which is one of the main reasons why I view DomU exposure as
>> going too far, despite recognizing the argument that this would only
>> be done if that DomU is fully trusted.
>>
>> Furthermore - what's the effect of this? It would seem to me that
>> while bus mastering is off, the device will not function. What happens
>> to output occurring during that time window?
>
> If no reset happens, the controller will continue sending the data after
> bus mastering is enabled back - no data lost in this case. If reset does
> happen, data that was already handed off to the controller (TRB queued)
> but not sent yet, is lost. But data that is still queued only in the
> work_ring, will be sent after controller is re-initialized. I did
> several tests of this, and I have not noticed any data loss in practice.
>
>> Rather than needing to
>> re-enable bus mastering behind the owning domain's back, can't the
>> disabling of bus mastering be avoided in the driver there?
>
> Linux disables bus mastering when PCI devices are enumerated (before
> xhci driver is loaded at all), and enables it back only when xhci driver
> tells it so. So, if xhci driver in dom0 is blacklisted (which is the
> case in qubes by default...), the console would be much less useful, so
> to say. And I don't think Linux maintainers will appreciate xen-xhci-dbc
> specific code in core PCI handling...
> It isn't an issue for EHCI driver, because EHCI debug port
> interface does not seem to use DMA.
>
>> As we can
>> see from the EHCI driver, there certainly can be communication
>> between Xen and Dom0 for functionality-impacting operations Dom0
>> might perform (there it's a device reset iirc).
>
> Yes, I can see how controller reset can be coordinated this way. But
> also, I see it more like a future improvement if it deemed to be
> necessary, than a strict requirement, as the controller reset is a quick
> event that in practice does not impact the functionality in any
> significant way (with the current code shape). On the other hand, adding
> such synchronization feels like several more iterations of this
> series...
>
> And BTW, if Linux crashes in the middle of controller reset, with such
> synchronization you would not get the crash message at all. While
> admittedly such issue is rather unlikely, I see it as a potential
> downside of coordination here (you could still avoid it by
> `share=none`/`share=no`, with all its consequences).
>
> Generally, what would be your minimal acceptable version? If support for
> sharing the controller with a domain (including dom0) requires
> significantly more work to be accepted, I'd much prefer to drop it in
> this series and have it possibly introduced later (and in the meantime,
> possibly carry as a downstream patch). Unfortunately, I have limited
> time to work on the series, but also, I think having this feature
> upstream - even in partial form - will be very useful for many Xen users
> and developers. Especially, I'd like this series (in some shape) to be
> included in Xen 4.17.
I think I could agree with such logic as a temporary measure, i.e. marked
clearly with a FIXME: or alike. The Kconfig option then also would want
marking "experimental" (or maybe it already is).
>>> @@ -1005,10 +1050,32 @@ static void __init cf_check
>>> dbc_uart_init_postirq(struct serial_port *port)
>>> init_timer(>timer, dbc_uart_poll, port, 0);
>>> set_timer(>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",
>>> - >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",
>>> + >dbc.sbdf);
>>> +break;
>>> +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",
>>> + >dbc.sbdf);
>>> +break;
>>> +case XHCI_SHARE_ANY:
>>> +/* Do not hide. */
>>> +break;
>>> +}
>>> +#ifdef CONFIG_X86
>>> +if ( rangeset_add_range(mmio_ro_ranges,
Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
On Fri, Aug 05, 2022 at 10:15:59AM +0200, Jan Beulich wrote: > On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote: > > 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. > > Which is one of the main reasons why I view DomU exposure as > going too far, despite recognizing the argument that this would only > be done if that DomU is fully trusted. > > Furthermore - what's the effect of this? It would seem to me that > while bus mastering is off, the device will not function. What happens > to output occurring during that time window? If no reset happens, the controller will continue sending the data after bus mastering is enabled back - no data lost in this case. If reset does happen, data that was already handed off to the controller (TRB queued) but not sent yet, is lost. But data that is still queued only in the work_ring, will be sent after controller is re-initialized. I did several tests of this, and I have not noticed any data loss in practice. > Rather than needing to > re-enable bus mastering behind the owning domain's back, can't the > disabling of bus mastering be avoided in the driver there? Linux disables bus mastering when PCI devices are enumerated (before xhci driver is loaded at all), and enables it back only when xhci driver tells it so. So, if xhci driver in dom0 is blacklisted (which is the case in qubes by default...), the console would be much less useful, so to say. And I don't think Linux maintainers will appreciate xen-xhci-dbc specific code in core PCI handling... It isn't an issue for EHCI driver, because EHCI debug port interface does not seem to use DMA. > As we can > see from the EHCI driver, there certainly can be communication > between Xen and Dom0 for functionality-impacting operations Dom0 > might perform (there it's a device reset iirc). Yes, I can see how controller reset can be coordinated this way. But also, I see it more like a future improvement if it deemed to be necessary, than a strict requirement, as the controller reset is a quick event that in practice does not impact the functionality in any significant way (with the current code shape). On the other hand, adding such synchronization feels like several more iterations of this series... And BTW, if Linux crashes in the middle of controller reset, with such synchronization you would not get the crash message at all. While admittedly such issue is rather unlikely, I see it as a potential downside of coordination here (you could still avoid it by `share=none`/`share=no`, with all its consequences). Generally, what would be your minimal acceptable version? If support for sharing the controller with a domain (including dom0) requires significantly more work to be accepted, I'd much prefer to drop it in this series and have it possibly introduced later (and in the meantime, possibly carry as a downstream patch). Unfortunately, I have limited time to work on the series, but also, I think having this feature upstream - even in partial form - will be very useful for many Xen users and developers. Especially, I'd like this series (in some shape) to be included in Xen 4.17. > > --- 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[ | @pci:. ]` > > -> `= xhci[ | @pci:. ]` > > +> `= xhci[ | @pci:. ][,share=none|hwdom|any]` > > > > 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,16 @@ 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 > > (output > > only). 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: > > +* `none`: use the controller exclusively for console, even hardware domain > > + (dom0) cannot use it; this is the default > > +* `hwdom`: hardware domain may use the controller too, ports not used for > > debug > > + console will be available for normal devices > > +* `any`: the controller can be assigned to any domain; it is not safe to > > assign > > + the controller to untrusted domain > > I'm sorry, upon looking here more closely, can we use proper boolean > here as we do elsewhere, i.e. share=no|yes|hwdom (or more generically > expressed share=|hwdom)? > > I also think 'hwdom' should be the default, like we do for EHCI (with, > at present, not even a way to override). Yes, I can do that.
Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> 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.
Which is one of the main reasons why I view DomU exposure as
going too far, despite recognizing the argument that this would only
be done if that DomU is fully trusted.
Furthermore - what's the effect of this? It would seem to me that
while bus mastering is off, the device will not function. What happens
to output occurring during that time window? Rather than needing to
re-enable bus mastering behind the owning domain's back, can't the
disabling of bus mastering be avoided in the driver there? As we can
see from the EHCI driver, there certainly can be communication
between Xen and Dom0 for functionality-impacting operations Dom0
might perform (there it's a device reset iirc).
> --- 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[ | @pci:. ]`
> -> `= xhci[ | @pci:. ]`
> +> `= xhci[ | @pci:. ][,share=none|hwdom|any]`
>
> 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,16 @@ 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 (output
> only). 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:
> +* `none`: use the controller exclusively for console, even hardware domain
> + (dom0) cannot use it; this is the default
> +* `hwdom`: hardware domain may use the controller too, ports not used for
> debug
> + console will be available for normal devices
> +* `any`: the controller can be assigned to any domain; it is not safe to
> assign
> + the controller to untrusted domain
I'm sorry, upon looking here more closely, can we use proper boolean
here as we do elsewhere, i.e. share=no|yes|hwdom (or more generically
expressed share=|hwdom)?
I also think 'hwdom' should be the default, like we do for EHCI (with,
at present, not even a way to override).
> +Choosing `share=hwdom` or `share=any` allows a domain to reset the
> controller,
> +which may cause small portion of the console output to be lost.
As said above - this ought to be avoidable if the period of time the
reset takes is bounded and if the controlling domain announces the
reset and its completion. See ehci-dbgp.c:dbgp_op().
In any event I'd like to ask that you add a statement to the effect of
"no security support when using 'any'".
> @@ -1005,10 +1050,32 @@ static void __init cf_check
> dbc_uart_init_postirq(struct serial_port *port)
> init_timer(>timer, dbc_uart_poll, port, 0);
> set_timer(>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",
> - >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",
> + >dbc.sbdf);
> +break;
> +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",
> + >dbc.sbdf);
> +break;
> +case XHCI_SHARE_ANY:
> +/* Do not hide. */
> +break;
> +}
> +#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");
How can this allow use of the device by a domain? Is there some sort of
guarantee that nothing else will live in the same 4k range? I can't
infer such from xhci_find_dbc().
> @@ -1085,7 +1153,7 @@ void __init xhci_dbc_uart_init(void)
> unsigned int bus, slot, func;
>
> e = parse_pci(opt_dbgp + 8, NULL, , , );
> -if ( !e || *e )
> +if ( !e || (*e && *e != ',') )
> {
> printk(XENLOG_ERR
> "Invalid dbgp=
[PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC
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().
Add an option to allow/deny other domains to use the USB controller. By
default, if XHCI console is enabled, Xen will take the whole controller
for itself, using `dbgp=xhci,share=hwdom` or `=any` allows other ports
to be used by either only dom0 or any dom0 that get this PCI device
assigned.
In any case, to avoid Linux messing with the DbC, mark this MMIO area as
read-only.
Signed-off-by: Marek Marczykowski-Górecki
---
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 | 12 ++-
xen/drivers/char/xhci-dbc.c | 115 ---
2 files changed, 118 insertions(+), 9 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index e53efdb324b3..cc1e1989b17e 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[ | @pci:. ]`
-> `= xhci[ | @pci:. ]`
+> `= xhci[ | @pci:. ][,share=none|hwdom|any]`
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,16 @@ 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 (output
only). 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:
+* `none`: use the controller exclusively for console, even hardware domain
+ (dom0) cannot use it; this is the default
+* `hwdom`: hardware domain may use the controller too, ports not used for debug
+ console will be available for normal devices
+* `any`: the controller can be assigned to any domain; it is not safe to assign
+ the controller to untrusted domain
+
+Choosing `share=hwdom` or `share=any` allows a domain to reset the controller,
+which may cause small portion of the console output to be lost.
### debug_stack_lines
> `= `
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
index 546231a75894..805b447f2300 100644
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -228,6 +229,12 @@ struct dbc_work_ring {
uint64_t dma;
};
+enum xhci_share {
+XHCI_SHARE_NONE = 0,
+XHCI_SHARE_HWDOM,
+XHCI_SHARE_ANY
+};
+
struct dbc {
struct dbc_reg __iomem *dbc_reg;
struct xhci_dbc_ctx *dbc_ctx;
@@ -244,6 +251,7 @@ struct dbc {
void __iomem *xhc_mmio;
bool open;
+enum xhci_share share;
unsigned int xhc_num; /* look for n-th xhc */
};
@@ -871,8 +879,9 @@ 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)
{
@@ -880,6 +889,42 @@ static bool dbc_ensure_running(struct dbc *dbc)
uint32_t ctrl;
uint32_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);
+}
+
+if ( dbc->open && !(readl(>ctrl) & (1U << DBC_CTRL_DCE)) )
+{
+if ( !dbc_init_dbc(dbc) )
+return false;
+
+dbc_init_work_ring(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);
