| Message ID | 1f9909dacfd7822a1c7d30ba03bbec93fa2ff6fd.1683321183.git-series.marmarek@invisiblethingslab.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add API for making parts of a MMIO page R/O and use it in XHCI console | expand |
On 05.05.2023 23:25, 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. Please can you clarify whether this is with spec or an erratum? I ask not the least because I continue to wonder whether we really want/need the non-negligible amount of new code added by path 1. > And with share=yes, a domU would use them too. And gain yet more access to the emulator, as mentioned in patch 1. The security implications may (will?) want mentioning. > --- a/xen/drivers/char/xhci-dbc.c > +++ b/xen/drivers/char/xhci-dbc.c > @@ -1221,14 +1221,12 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) > * 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\n"); So how about falling back to just rangeset_add_range(mmio_ro_ranges, ...) in this failure case? (I did mention an alternative to doing it here in the comments on patch 1.) Also, doesn't the comment ahead of the construct become stale? Finally I think indentation of the function call arguments is off by one. Jan
On Tue, May 30, 2023 at 02:04:26PM +0200, Jan Beulich wrote: > On 05.05.2023 23:25, 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. > > Please can you clarify whether this is with spec or an erratum? It is in spec. The spec is written with assumption that different driver may drive the debug console than the rest of the controller. But the spec (apparently) does not anticipate those drivers living in different kernels, so does not mandate isolating registers to separate pages. > I ask > not the least because I continue to wonder whether we really want/need > the non-negligible amount of new code added by path 1. > > > And with share=yes, a domU would use them too. > > And gain yet more access to the emulator, as mentioned in patch 1. The > security implications may (will?) want mentioning. > > > --- a/xen/drivers/char/xhci-dbc.c > > +++ b/xen/drivers/char/xhci-dbc.c > > @@ -1221,14 +1221,12 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) > > * 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\n"); > > So how about falling back to just rangeset_add_range(mmio_ro_ranges, ...) > in this failure case? (I did mention an alternative to doing it here in > the comments on patch 1.) Or fallback to XHCI_SHARE_NONE (IOW, pci_ro_device()). > Also, doesn't the comment ahead of the construct become stale? Indeed. > Finally I think indentation of the function call arguments is off by one. How is it supposed to be? Currently it's 8 spaces over the "if", should it be 4 spaces over the function name?
On 01.07.2023 00:38, Marek Marczykowski-Górecki wrote: > On Tue, May 30, 2023 at 02:04:26PM +0200, Jan Beulich wrote: >> On 05.05.2023 23:25, Marek Marczykowski-Górecki wrote: >>> --- a/xen/drivers/char/xhci-dbc.c >>> +++ b/xen/drivers/char/xhci-dbc.c >>> @@ -1221,14 +1221,12 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) >>> * 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\n"); >> >> So how about falling back to just rangeset_add_range(mmio_ro_ranges, ...) >> in this failure case? (I did mention an alternative to doing it here in >> the comments on patch 1.) > > Or fallback to XHCI_SHARE_NONE (IOW, pci_ro_device()). > >> Also, doesn't the comment ahead of the construct become stale? > > Indeed. > >> Finally I think indentation of the function call arguments is off by one. > > How is it supposed to be? Currently it's 8 spaces over the "if", should > it be 4 spaces over the function name? Yes. All our indentation is using a granularity of 4 spaces, of course taking into account aligning with respective items on earlier lines (the length of which may not be a multiple of 4). It is always the innermost construct relative to which the new indentation level is determined. Jan
diff --git a/xen/drivers/char/xhci-dbc.c b/xen/drivers/char/xhci-dbc.c index 60b781f87202..8e368364c933 100644 --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -1221,14 +1221,12 @@ static void __init cf_check dbc_uart_init_postirq(struct serial_port *port) * 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\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> --- Changes in v2: - adjust for simplified subpage_mmio_ro_add() API --- xen/drivers/char/xhci-dbc.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-)