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

[Jan Beulich <jbeulich@xxxxxxxx>]

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