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.
> + 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?
Jan
