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

[Jan Beulich <jbeulich@suse.com>]

On 27.03.2023 12:09, Marek Marczykowski-Górecki wrote:
> ... not the whole page, which may contain other registers too. In fact
> on Tiger Lake and newer (at least), this page do contain other registers
> that Linux tries to use. And with share=yes, a domU would use them too.
> Without this patch, PV dom0 would fail to initialize the controller,
> while HVM would be killed on EPT violation.
> 
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  xen/drivers/char/xhci-dbc.c | 38 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c
> index 60b781f87202..df2524b0ca18 100644
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1226,9 +1226,43 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port)
>                           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
> +                sizeof(*uart->dbc.dbc_reg)) - 1) ) {

Nit: No need for a brace here (and certainly not a misplaced one).

> +        printk(XENLOG_WARNING

This log level change looks kind of unrelated.

>                 "Error while adding MMIO range of device to mmio_ro_ranges\n");
> +    }
> +    else
> +    {
> +        unsigned long dbc_regs_start = (uart->dbc.bar_val &
> +                PCI_BASE_ADDRESS_MEM_MASK) + uart->dbc.xhc_dbc_offset;
> +        unsigned long dbc_regs_end = dbc_regs_start + sizeof(*uart->dbc.dbc_reg);
> +
> +        /* This being smaller than a page simplifies conditions below */
> +        BUILD_BUG_ON(sizeof(*uart->dbc.dbc_reg) >= PAGE_SIZE - 1);

Why PAGE_SIZE - 1 (or why >= instead of > )? If there is a reason, then
the comment wants to be in sync.

> +        if ( dbc_regs_start & (PAGE_SIZE - 1) ||

Nit: Please parenthesize the & against the || (similarly again below).

Like asked by Roger for patch 1 (iirc), here and below please use
PAGE_OFFSET() in favor of (kind of) open-coding it.

> +                PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end) )

Nit: Style (indentation).

> +        {
> +            if ( subpage_mmio_ro_add(
> +                        _mfn(PFN_DOWN(dbc_regs_start)),
> +                        dbc_regs_start & (PAGE_SIZE - 1),
> +                        PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end)
> +                        ? dbc_regs_end & (PAGE_SIZE - 1)
> +                        : PAGE_SIZE - 1,
> +                        FIX_XHCI_END) )

Nit: I think this is too deep a level of indentation; it should be a
single level (4 blanks) from the start of the function name (also
again another time below).

> +                printk(XENLOG_WARNING
> +                        "Error while adding MMIO range of device to subpage_mmio_ro\n");

Nit: Style (indentation).

> +        }
> +        if ( dbc_regs_end & (PAGE_SIZE - 1) &&
> +                PFN_DOWN(dbc_regs_start) != PFN_DOWN(dbc_regs_end) )
> +        {
> +            if ( subpage_mmio_ro_add(
> +                        _mfn(PFN_DOWN(dbc_regs_end)),
> +                        0,
> +                        dbc_regs_end & (PAGE_SIZE - 1),
> +                        FIX_XHCI_END + PFN_DOWN(sizeof(*uart->dbc.dbc_reg))) )
> +                printk(XENLOG_WARNING
> +                        "Error while adding MMIO range of device to subpage_mmio_ro\n");
> +        }
> +    }

Seeing the uses it occurs to me that the interface is somewhat odd: It
adds a r/o range to a page that is already recorded to be r/o. It would
imo be more logical the other way around: To add an exception (writable)
range. The only alternative would be to include the call to
rangeset_add_range(mmio_ro_ranges, ...) as part of the new function, and
reduce accordingly the range passed earlier in the function. But I think
this would needlessly complicate the code there.

Jan