| Message ID | a1becf0ed2a19304ce122540e67675c06aee1702.1657121519.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Xue - console over USB 3 Debug Capability | expand |
On 06.07.2022 17:32, 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. > Linux driver does similar thing - see > drivers/usb/early/xhci-dbc.c:xdbc_handle_events(). Isn't there a risk that intermediately data was lost? > To avoid Linux messing with the DbC, mark this MMIO area as read-only. In principle this would want to happen quite a bit earlier in the series. I'm okay with it being kept here as long as it is made very obvious to and easily noticeable by committers that this series should only be committed all in one go. Also along with this is where I'd see the pci_hide_device() go. > @@ -817,6 +819,16 @@ static void xue_flush(struct xue *xue, struct xue_trb_ring *trb, > xue_enable_dbc(xue); > } > > + /* Re-enable bus mastering, if dom0 (or other) disabled it in the meantime. */ > + cmd = pci_conf_read16(xue->sbdf, PCI_COMMAND); > +#define XUE_XHCI_CMD_REQUIRED (PCI_COMMAND_MEMORY|PCI_COMMAND_MASTER) > + if ( (cmd & XUE_XHCI_CMD_REQUIRED) != XUE_XHCI_CMD_REQUIRED ) > + { > + cmd |= XUE_XHCI_CMD_REQUIRED; > + pci_conf_write16(xue->sbdf, PCI_COMMAND, cmd); > + } > +#undef XUE_XHCI_CMD_REQUIRED > + > xue_pop_events(xue); > > if ( !(reg->ctrl & (1UL << XUE_CTRL_DCR)) ) > @@ -916,6 +928,13 @@ static void __init cf_check xue_uart_init_postirq(struct serial_port *port) > serial_async_transmit(port); > init_timer(&uart->timer, xue_uart_poll, port, 0); > set_timer(&uart->timer, NOW() + MILLISECS(1)); > + > +#ifdef CONFIG_X86 > + if ( rangeset_add_range(mmio_ro_ranges, > + PFN_DOWN(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset), > + PFN_UP(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset + sizeof(*uart->xue.dbc_reg)) - 1) ) > + printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n"); > +#endif > } According to my counting there are three overly long lines in these two hunks. Jan
On Thu, Jul 14, 2022 at 02:06:07PM +0200, Jan Beulich wrote: > On 06.07.2022 17:32, 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. > > Linux driver does similar thing - see > > drivers/usb/early/xhci-dbc.c:xdbc_handle_events(). > > Isn't there a risk that intermediately data was lost? Yes, there is such risk (although minimal in practice - it happens just once during dom0 boot). You can avoid it by instructing dom0 to not use that USB controller. Having this capability is really helpful (compared with the alternative of using the whole USB controller by either Xen or Linux), as many (most) systems have only a single USB controller. > > To avoid Linux messing with the DbC, mark this MMIO area as read-only. > > In principle this would want to happen quite a bit earlier in the > series. I'm okay with it being kept here as long as it is made > very obvious to and easily noticeable by committers that this series > should only be committed all in one go. > > Also along with this is where I'd see the pci_hide_device() go. Earlier version of the patch set had pci_ro_device() before this patch. I can add pci_ro_device() in the initial patch, and drop it in this one.
On 18.07.2022 14:54, Marek Marczykowski-Górecki wrote: > On Thu, Jul 14, 2022 at 02:06:07PM +0200, Jan Beulich wrote: >> On 06.07.2022 17:32, 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. >>> Linux driver does similar thing - see >>> drivers/usb/early/xhci-dbc.c:xdbc_handle_events(). >> >> Isn't there a risk that intermediately data was lost? > > Yes, there is such risk (although minimal in practice - it happens just > once during dom0 boot). You can avoid it by instructing dom0 to not use > that USB controller. > Having this capability is really helpful (compared with the alternative > of using the whole USB controller by either Xen or Linux), as many > (most) systems have only a single USB controller. No question about the usefulness. But this aspect wants spelling out, and it is one of the arguments against allowing use of the device by other than hwdom. >>> To avoid Linux messing with the DbC, mark this MMIO area as read-only. >> >> In principle this would want to happen quite a bit earlier in the >> series. I'm okay with it being kept here as long as it is made >> very obvious to and easily noticeable by committers that this series >> should only be committed all in one go. >> >> Also along with this is where I'd see the pci_hide_device() go. > > Earlier version of the patch set had pci_ro_device() before this patch. > I can add pci_ro_device() in the initial patch, and drop it in this one. Having pci_ro_device() up to here sounds reasonable, but then you still want to flip to using pci_hide_device() rather than just dropping the call. Jan
diff --git a/xen/drivers/char/xue.c b/xen/drivers/char/xue.c index a6c49bb43e97..3461e51e746a 100644 --- a/xen/drivers/char/xue.c +++ b/xen/drivers/char/xue.c @@ -27,6 +27,7 @@ #include <xen/timer.h> #include <xen/param.h> #include <xen/iommu.h> +#include <xen/rangeset.h> #include <asm/fixmap.h> #include <asm/io.h> #include <xen/mm.h> @@ -807,6 +808,7 @@ static void xue_flush(struct xue *xue, struct xue_trb_ring *trb, { struct xue_dbc_reg *reg = xue->dbc_reg; uint32_t db = (reg->db & 0xFFFF00FF) | (trb->db << 8); + uint32_t cmd; if ( xue->open && !(reg->ctrl & (1UL << XUE_CTRL_DCE)) ) { @@ -817,6 +819,16 @@ static void xue_flush(struct xue *xue, struct xue_trb_ring *trb, xue_enable_dbc(xue); } + /* Re-enable bus mastering, if dom0 (or other) disabled it in the meantime. */ + cmd = pci_conf_read16(xue->sbdf, PCI_COMMAND); +#define XUE_XHCI_CMD_REQUIRED (PCI_COMMAND_MEMORY|PCI_COMMAND_MASTER) + if ( (cmd & XUE_XHCI_CMD_REQUIRED) != XUE_XHCI_CMD_REQUIRED ) + { + cmd |= XUE_XHCI_CMD_REQUIRED; + pci_conf_write16(xue->sbdf, PCI_COMMAND, cmd); + } +#undef XUE_XHCI_CMD_REQUIRED + xue_pop_events(xue); if ( !(reg->ctrl & (1UL << XUE_CTRL_DCR)) ) @@ -916,6 +928,13 @@ static void __init cf_check xue_uart_init_postirq(struct serial_port *port) serial_async_transmit(port); init_timer(&uart->timer, xue_uart_poll, port, 0); set_timer(&uart->timer, NOW() + MILLISECS(1)); + +#ifdef CONFIG_X86 + if ( rangeset_add_range(mmio_ro_ranges, + PFN_DOWN(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset), + PFN_UP(uart->xue.xhc_mmio_phys + uart->xue.xhc_dbc_offset + sizeof(*uart->xue.dbc_reg)) - 1) ) + printk(XENLOG_INFO "Error while adding MMIO range of device to mmio_ro_ranges\n"); +#endif } static int cf_check xue_uart_tx_ready(struct serial_port *port)
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(). To avoid Linux messing with the DbC, mark this MMIO area as read-only. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- xen/drivers/char/xue.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)