Re: [PATCH v3 2/2] drivers/char: Use sub-page ro API to make just xhci dbc cap RO

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 2989 bytes --]

On Wed, May 22, 2024 at 10:05:05AM +0200, Jan Beulich wrote:
> On 21.05.2024 04:54, Marek Marczykowski-Górecki wrote:
> > --- a/xen/drivers/char/xhci-dbc.c
> > +++ b/xen/drivers/char/xhci-dbc.c
> > @@ -1216,20 +1216,19 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
> >          break;
> >      }
> >  #ifdef CONFIG_X86
> > -    /*
> > -     * This marks the whole page as R/O, which may include other registers
> > -     * unrelated to DbC. Xen needs only DbC area protected, but it seems
> > -     * Linux's XHCI driver (as of 5.18) works without writting to the whole
> > -     * page, so keep it simple.
> > -     */
> > -    if ( rangeset_add_range(mmio_ro_ranges,
> > -                PFN_DOWN((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> > -                         uart->dbc.xhc_dbc_offset),
> > -                PFN_UP((uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> > -                       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");
> > +    if ( subpage_mmio_ro_add(
> > +             (uart->dbc.bar_val & PCI_BASE_ADDRESS_MEM_MASK) +
> > +              uart->dbc.xhc_dbc_offset,
> > +             sizeof(*uart->dbc.dbc_reg)) )
> > +    {
> > +        printk(XENLOG_WARNING
> > +               "Error while marking MMIO range of XHCI console as R/O, "
> > +               "making the whole device R/O (share=no)\n");
> 
> Since you mention "share=no" here, wouldn't you then better also update the
> respective struct field, even if (right now) there may be nothing subsequently
> using that? Except that dbc_ensure_running() actually is looking at it, and
> that's not an __init function.

That case is just an optimization - if pci_ro_device() is used, nobody
else could write to PCI_COMMAND behind the driver backs, so there is no
point checking. Anyway, yes, makes sense to adjust dbc->share too.

> > +        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);
> > +    }
> >  #endif
> >  }
> 
> It's been a long time since v2 and the description doesn't say anything in
> this regard: Is there a reason not to retain the rangeset addition alongside
> the pci_ro_device() on the fallback path?

pci_ro_device() prevents device from being assigned to domU at all, so
that case is covered already. Dom0 would fail to load any driver (if
nothing else - because it can't size the BARs with R/O config space), so
a _well behaving_ Dom0 would also not touch the device in this case.
But otherwise, yes, it makes sense keep adding to mmio_ro_ranges in the
fallback path.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]