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 <[email protected]>
> ---
> 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
