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
signature.asc
Description: PGP signature
