| Message ID | befefa60ea42a41543bc6dad70a559816cda8b7c.1679911575.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add API for making parts of a MMIO page R/O and use it in XHCI console | expand |
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
On Wed, Mar 29, 2023 at 11:14:52AM +0200, Jan Beulich wrote: > 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. Indeed looks like off-by-one. > > + 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. I'm trying to make the interface safe against multiple calls making different parts of the page R/O. If it would mark an exception, then handling a page where multiple regions need to be protected would be rather cumbersome. It isn't the case for XHCI driver, but for example it could be in MSI-X (if I'd actually use this API there). Making subpage_mmio_ro_add() to call mmio_ro_ranges() on its own, together with Roger's suggestion of using ioremap() internally instead of using fixmap would make it a bit nicer (if mapping the same page with ioremap() in addition to fixmap isn't a problem).
On 29.03.2023 12:21, Marek Marczykowski-Górecki wrote: > Making subpage_mmio_ro_add() to call mmio_ro_ranges() on its own, > together with Roger's suggestion of using ioremap() internally instead > of using fixmap would make it a bit nicer (if mapping the same page with > ioremap() in addition to fixmap isn't a problem). I don't see any problem there, as long as we don't gain many of these (at which point wasting address space may become an issue). Jan
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) ) { + printk(XENLOG_WARNING "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); + if ( dbc_regs_start & (PAGE_SIZE - 1) || + PFN_DOWN(dbc_regs_start) == PFN_DOWN(dbc_regs_end) ) + { + 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) ) + printk(XENLOG_WARNING + "Error while adding MMIO range of device to subpage_mmio_ro\n"); + } + 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"); + } + } #endif }
... 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(-)