| Message ID | 7682bbf3a24045ae1fcbdf651a6dd8e609ec3a20.1716260066.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 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
On Wed, May 22, 2024 at 10:05:05AM +0200, Jan Beulich wrote: > 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. That case is just an optimization - if pci_ro_device() is used, nobody else could write to PCI_COMMAND behind the driver backs, so there is no point checking. Anyway, yes, makes sense to adjust dbc->share too. > > + 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? pci_ro_device() prevents device from being assigned to domU at all, so that case is covered already. Dom0 would fail to load any driver (if nothing else - because it can't size the BARs with R/O config space), so a _well behaving_ Dom0 would also not touch the device in this case. But otherwise, yes, it makes sense keep adding to mmio_ro_ranges in the fallback path.
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index 8e2037f1a5f7..cac9d90d3e39 100644 --- 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"); + 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 }
Not the whole page, which may contain other registers too. The XHCI specification describes DbC as designed to be controlled by a different driver, but does not mandate placing registers on a separate page. 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. With `share=yes`, this patch gives domU more access to the emulator (although a HVM with any emulated device already has plenty of it). This configuration is already documented as unsafe with untrusted guests and not security supported. Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> --- Changes in v3: - indentation fix - remove stale comment - fallback to pci_ro_device() if subpage_mmio_ro_add() fails - extend commit message Changes in v2: - adjust for simplified subpage_mmio_ro_add() API --- xen/drivers/char/xhci-dbc.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)