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

[Jan Beulich <jbeulich@xxxxxxxx>]

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