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 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