Re: [PATCH v3 09/10] drivers/char: allow driving the rest of XHCI by a domain while Xen uses DbC

[Jan Beulich <jbeulich@xxxxxxxx>]

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[ <integer> | @pci<bus>:<slot>.<func> ]`
> -> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ]`
> +> `= xhci[ <integer> | @pci<bus>:<slot>.<func> ][,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=<boolean>|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(&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;
> +    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;
> +    }
> +#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, &bus, &slot, &func);
> -        if ( !e || *e )
> +        if ( !e || (*e && *e != ',') )
>          {
>              printk(XENLOG_ERR
>                     "Invalid dbgp= PCI device spec: '%s'\n",
> @@ -1094,6 +1162,37 @@ void __init xhci_dbc_uart_init(void)
>          }
>          dbc->sbdf = PCI_SBDF(0, bus, slot, func);
>      }
> +    opt = e;

Looks like e (and hence opt) cannot be NULL here, ...

> +    /* other options */
> +    while ( opt && *opt == ',' )
> +    {
> +        opt++;
> +        e = strchr(opt, ',');
> +        if ( !e )
> +            e = strchr(opt, '\0');
> +
> +        if ( !strncmp(opt, "share=", 6) )
> +        {
> +            if ( !cmdline_strcmp(opt + 6, "none") )
> +                dbc->share = XHCI_SHARE_NONE;
> +            else if ( !cmdline_strcmp(opt + 6, "hwdom") )
> +                dbc->share = XHCI_SHARE_HWDOM;
> +            else if ( !cmdline_strcmp(opt + 6, "any") )
> +                dbc->share = XHCI_SHARE_ANY;
> +            else
> +                break;
> +        }
> +        else
> +            break;
> +
> +        opt = e;

... nor here. Hence I wonder why the while() and ...

> +    }
> +    if ( !opt || *opt )

... this if() check for it being (non-)NULL. At which point ...

> +    {
> +        printk(XENLOG_ERR "Invalid dbgp= parameters: '%s'\n", opt_dbgp);

... you could make the message here more specific by passing "opt"
instead of the full "opt_dbgp".

Jan