| Message ID | 05b1284a-988c-4f91-9cde-4751332aaa96@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | limit passing around of cpu_user_regs | expand |
On 11/01/2024 7:34 am, Jan Beulich wrote: > In the the polling functions (ab)using set_irq_regs() is necessary > to balance the change. I have to admit that I don't know what "balance the change" is supposed to refer to in this context. > --- a/xen/drivers/char/ehci-dbgp.c > +++ b/xen/drivers/char/ehci-dbgp.c > @@ -1268,11 +1269,16 @@ static void cf_check _ehci_dbgp_poll(str > spin_unlock_irqrestore(&port->tx_lock, flags); > } > > + /* Mimic interrupt context. */ > + old_regs = set_irq_regs(regs); > + > if ( dbgp->in.chunk ) > - serial_rx_interrupt(port, regs); > + serial_rx_interrupt(port); > > if ( empty ) > - serial_tx_interrupt(port, regs); > + serial_tx_interrupt(port); > + > + set_irq_regs(old_regs); Looking at this logic, it has occured to me that patch 2 probably ought to have ASSERT(!local_irqs_enabled()) in set_irq_regs(). While the main arch irq dispatch can reasonably have it as an implicit expectation, uses like this could do with the check. This construct is very nasty. What actually needs it? If it's only handle_keypress(), isn't there a latent issue between patch 3 and 5, given that patch 3 uses set_irq_regs() before this patch sets it up? Might it be better to do this in the main handling of BUGFRAME_run_fn, rather than at a few select users? We're already abusing BUGFRAME_run_fn to set up an IRQ-like context for these poll functions. I suppose a different question is what it would take to get rid of this. Is it something a bit more cleanup would solve, or is there some more fundamental untangling required? > --- a/xen/drivers/char/xhci-dbc.c > +++ b/xen/drivers/char/xhci-dbc.c > @@ -1175,10 +1176,15 @@ static void cf_check dbc_uart_poll(void > spin_unlock_irqrestore(&port->tx_lock, flags); > } > > + /* Mimic interrupt context. */ > + old_regs = set_irq_regs(guest_cpu_user_regs()); This is not a bug in your patch, but... The use of guest_cpu_user_regs() here is different to all the other poll functions. Is this actually correct? If we're really in interrupt context and then we fake up a poll like this, then we don't have a total order of frames recorded in the irq_regs pointer. I can't see a specific issue, but it also doesn't feel as if it is something we should allow. ~Andrew
On 11.01.2024 19:07, Andrew Cooper wrote: > On 11/01/2024 7:34 am, Jan Beulich wrote: >> In the the polling functions (ab)using set_irq_regs() is necessary >> to balance the change. > > I have to admit that I don't know what "balance the change" is supposed > to refer to in this context. Maybe just a lack of proper English on my part. What I'm trying to say is that the removal of the function parameter comes with the need to make the pointer available via set_irq_regs() (which is, in a way at least, an abuse of the function). >> --- a/xen/drivers/char/ehci-dbgp.c >> +++ b/xen/drivers/char/ehci-dbgp.c >> @@ -1268,11 +1269,16 @@ static void cf_check _ehci_dbgp_poll(str >> spin_unlock_irqrestore(&port->tx_lock, flags); >> } >> >> + /* Mimic interrupt context. */ >> + old_regs = set_irq_regs(regs); >> + >> if ( dbgp->in.chunk ) >> - serial_rx_interrupt(port, regs); >> + serial_rx_interrupt(port); >> >> if ( empty ) >> - serial_tx_interrupt(port, regs); >> + serial_tx_interrupt(port); >> + >> + set_irq_regs(old_regs); > > Looking at this logic, it has occured to me that patch 2 probably ought > to have ASSERT(!local_irqs_enabled()) in set_irq_regs(). While the main > arch irq dispatch can reasonably have it as an implicit expectation, > uses like this could do with the check. Why would IRQs need to be off for set_irq_regs()? It's all a matter of proper nesting, and any IRQ (nested into here or nested inside another IRQ) would properly save/restore the outer context's pointer (irrespective of what kind of context that actually is). Note also how __ns16550_poll() doesn't itself disable interrupts. While apparently not the case right now, I'm also of the opinion that IRQs could in principle be turned back on transiently while handling BUGFRAME_run_fn (and perhaps also BUGFRAME_warn). > This construct is very nasty. What actually needs it? > > If it's only handle_keypress(), isn't there a latent issue between patch > 3 and 5, given that patch 3 uses set_irq_regs() before this patch sets > it up? I think you're right - looks like I need to re-order (or fold, in case there would then be a build issue). > Might it be better to do this in the main handling of BUGFRAME_run_fn, > rather than at a few select users? We're already abusing > BUGFRAME_run_fn to set up an IRQ-like context for these poll functions. Hmm. It would then look at least a little less abusive, I suppose. Otoh the handler function being passed registers is quite natural imo, for being exception (not interrupt) related. Or are you suggesting to "pass" registers both ways (i.e. keep the handler function parameter while additionally also using set_irq_regs())? That would feel a little odd, for being redundant. Also I've never viewed use of BUGFRAME_run_fn here as having the purpose of setting up an IRQ-like context. I've always understood it as merely a means to get at a meaningful struct cpu_user_regs instance (i.e. covering the case of running in idle vCPU context; see below). Much like BUGFRAME_warn imo has only this as a purpose of involving generation of an exception. > I suppose a different question is what it would take to get rid of > this. Is it something a bit more cleanup would solve, or is there some > more fundamental untangling required? Well, what exactly is "this" here? Something needs to set the pointer, even if I add a patch to switch handle_keypress() itself to not take a regs parameter anymore. >> --- a/xen/drivers/char/xhci-dbc.c >> +++ b/xen/drivers/char/xhci-dbc.c >> @@ -1175,10 +1176,15 @@ static void cf_check dbc_uart_poll(void >> spin_unlock_irqrestore(&port->tx_lock, flags); >> } >> >> + /* Mimic interrupt context. */ >> + old_regs = set_irq_regs(guest_cpu_user_regs()); > > This is not a bug in your patch, but... > > The use of guest_cpu_user_regs() here is different to all the other poll > functions. Is this actually correct? I think it is okay-ish right now, but indeed I meant to have a post- commit-message remark about this. In particular, ... > If we're really in interrupt context and then we fake up a poll like > this, then we don't have a total order of frames recorded in the > irq_regs pointer. I can't see a specific issue, but it also doesn't > feel as if it is something we should allow. ... I don't see any ordering constraint. dbc_uart_poll() is a timer handler, so will never itself run in interrupt context. And any IRQ would cleanly nest. Nevertheless register state will likely not be very meaningful when the timer ends up running in the context of an idle vCPU. Marek, what's the background of you having done this differently to other poll handlers? Jan
--- a/xen/drivers/char/cadence-uart.c +++ b/xen/drivers/char/cadence-uart.c @@ -51,7 +51,7 @@ static void cuart_interrupt(int irq, voi /* ACK. */ if ( status & UART_SR_INTR_RTRIG ) { - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); cuart_write(uart, R_UART_CISR, UART_SR_INTR_RTRIG); } } while ( status & UART_SR_INTR_RTRIG ); --- a/xen/drivers/char/ehci-dbgp.c +++ b/xen/drivers/char/ehci-dbgp.c @@ -1253,6 +1253,7 @@ static void cf_check _ehci_dbgp_poll(str unsigned long flags; unsigned int timeout = MICROSECS(DBGP_CHECK_INTERVAL); bool empty = false; + struct cpu_user_regs *old_regs; if ( !dbgp->ehci_debug ) return; @@ -1268,11 +1269,16 @@ static void cf_check _ehci_dbgp_poll(str spin_unlock_irqrestore(&port->tx_lock, flags); } + /* Mimic interrupt context. */ + old_regs = set_irq_regs(regs); + if ( dbgp->in.chunk ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); if ( empty ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); + + set_irq_regs(old_regs); if ( spin_trylock_irqsave(&port->tx_lock, flags) ) { --- a/xen/drivers/char/exynos4210-uart.c +++ b/xen/drivers/char/exynos4210-uart.c @@ -81,7 +81,7 @@ static void exynos4210_uart_interrupt(in if ( status & (UINTM_RXD | UINTM_ERROR) ) { /* uart->regs[UINTM] |= RXD|ERROR; */ - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); /* uart->regs[UINTM] &= ~(RXD|ERROR); */ exynos4210_write(uart, UINTP, UINTM_RXD | UINTM_ERROR); } @@ -89,7 +89,7 @@ static void exynos4210_uart_interrupt(in if ( status & (UINTM_TXD | UINTM_MODEM) ) { /* uart->regs[UINTM] |= TXD|MODEM; */ - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); /* uart->regs[UINTM] &= ~(TXD|MODEM); */ exynos4210_write(uart, UINTP, UINTM_TXD | UINTM_MODEM); } --- a/xen/drivers/char/imx-lpuart.c +++ b/xen/drivers/char/imx-lpuart.c @@ -48,10 +48,10 @@ static void imx_lpuart_interrupt(int irq rxcnt = imx_lpuart_read(uart, UARTWATER) >> UARTWATER_RXCNT_OFF; if ( (sts & UARTSTAT_RDRF) || (rxcnt > 0) ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); if ( sts & UARTSTAT_TDRE ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); imx_lpuart_write(uart, UARTSTAT, sts); } --- a/xen/drivers/char/meson-uart.c +++ b/xen/drivers/char/meson-uart.c @@ -69,10 +69,10 @@ static void meson_uart_interrupt(int irq uint32_t st = readl(uart->regs + AML_UART_STATUS_REG); if ( !(st & AML_UART_RX_FIFO_EMPTY) ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); if ( !(st & AML_UART_TX_FIFO_FULL) ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); } static void __init meson_uart_init_preirq(struct serial_port *port) --- a/xen/drivers/char/mvebu-uart.c +++ b/xen/drivers/char/mvebu-uart.c @@ -76,10 +76,10 @@ static void mvebu3700_uart_interrupt(int if ( st & (STATUS_RX_RDY | STATUS_OVR_ERR | STATUS_FRM_ERR | STATUS_BRK_DET) ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); if ( st & STATUS_TX_RDY ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); } static void __init mvebu3700_uart_init_preirq(struct serial_port *port) --- a/xen/drivers/char/ns16550.c +++ b/xen/drivers/char/ns16550.c @@ -188,9 +188,9 @@ static void cf_check ns16550_interrupt( u8 lsr = ns_read_reg(uart, UART_LSR); if ( (lsr & uart->lsr_mask) == uart->lsr_mask ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); if ( lsr & UART_LSR_DR ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); /* A "busy-detect" condition is observed on Allwinner/sunxi UART * after LCR is written during setup. It needs to be cleared at @@ -211,22 +211,27 @@ static void cf_check __ns16550_poll(stru { struct serial_port *port = this_cpu(poll_port); struct ns16550 *uart = port->uart; + struct cpu_user_regs *old_regs; if ( uart->intr_works ) return; /* Interrupts work - no more polling */ + /* Mimic interrupt context. */ + old_regs = set_irq_regs(regs); + while ( ns_read_reg(uart, UART_LSR) & UART_LSR_DR ) { if ( ns16550_ioport_invalid(uart) ) goto out; - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); } if ( ( ns_read_reg(uart, UART_LSR) & uart->lsr_mask ) == uart->lsr_mask ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); out: + set_irq_regs(old_regs); set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms)); } --- a/xen/drivers/char/omap-uart.c +++ b/xen/drivers/char/omap-uart.c @@ -70,9 +70,9 @@ static void omap_uart_interrupt(int irq, { lsr = omap_read(uart, UART_LSR) & 0xff; if ( lsr & UART_LSR_THRE ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); if ( lsr & UART_LSR_DR ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); if ( port->txbufc == port->txbufp ) { reg = omap_read(uart, UART_IER); --- a/xen/drivers/char/pl011.c +++ b/xen/drivers/char/pl011.c @@ -95,7 +95,7 @@ static void pl011_interrupt(int irq, voi pl011_write(uart, ICR, status & ~(TXI|RTI|RXI)); if ( status & (RTI|RXI) ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); /* TODO if ( status & (DSRMI|DCDMI|CTSMI|RIMI) ) @@ -103,7 +103,7 @@ static void pl011_interrupt(int irq, voi */ if ( status & (TXI) ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); status = pl011_intr_status(uart); } while (status != 0); --- a/xen/drivers/char/scif-uart.c +++ b/xen/drivers/char/scif-uart.c @@ -119,11 +119,11 @@ static void scif_uart_interrupt(int irq, { /* TX Interrupt */ if ( status & SCFSR_TDFE ) - serial_tx_interrupt(port, regs); + serial_tx_interrupt(port); /* RX Interrupt */ if ( status & (SCFSR_RDF | SCFSR_DR) ) - serial_rx_interrupt(port, regs); + serial_rx_interrupt(port); /* Error Interrupt */ if ( status & params->error_mask ) --- a/xen/drivers/char/serial.c +++ b/xen/drivers/char/serial.c @@ -45,7 +45,7 @@ static inline void serial_stop_tx(struct port->driver->stop_tx(port); } -void serial_rx_interrupt(struct serial_port *port, struct cpu_user_regs *regs) +void serial_rx_interrupt(struct serial_port *port) { char c; serial_rx_fn fn = NULL; @@ -71,7 +71,7 @@ void serial_rx_interrupt(struct serial_p fn(c & 0x7f); } -void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs) +void serial_tx_interrupt(struct serial_port *port) { int i, n; unsigned long flags; --- a/xen/drivers/char/xhci-dbc.c +++ b/xen/drivers/char/xhci-dbc.c @@ -1164,6 +1164,7 @@ static void cf_check dbc_uart_poll(void struct dbc_uart *uart = port->uart; struct dbc *dbc = &uart->dbc; unsigned long flags = 0; + struct cpu_user_regs *old_regs; if ( spin_trylock_irqsave(&port->tx_lock, flags) ) { @@ -1175,10 +1176,15 @@ static void cf_check dbc_uart_poll(void spin_unlock_irqrestore(&port->tx_lock, flags); } + /* Mimic interrupt context. */ + old_regs = set_irq_regs(guest_cpu_user_regs()); + while ( dbc_work_ring_size(&dbc->dbc_iwork) ) - serial_rx_interrupt(port, guest_cpu_user_regs()); + serial_rx_interrupt(port); + + serial_tx_interrupt(port); - serial_tx_interrupt(port, guest_cpu_user_regs()); + set_irq_regs(old_regs); set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL)); } --- a/xen/include/xen/serial.h +++ b/xen/include/xen/serial.h @@ -12,8 +12,6 @@ #include <xen/init.h> #include <xen/spinlock.h> -struct cpu_user_regs; - /* Register a character-receive hook on the specified COM port. */ typedef void (*serial_rx_fn)(char c); void serial_set_rx_handler(int handle, serial_rx_fn fn); @@ -155,8 +153,8 @@ void serial_register_uart(int idx, struc /* Place the serial port into asynchronous transmit mode. */ void serial_async_transmit(struct serial_port *port); /* Process work in interrupt context. */ -void serial_rx_interrupt(struct serial_port *port, struct cpu_user_regs *regs); -void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs); +void serial_rx_interrupt(struct serial_port *port); +void serial_tx_interrupt(struct serial_port *port); /* * Initialisers for individual uart drivers.
In the the polling functions (ab)using set_irq_regs() is necessary to balance the change. This is in preparation of dropping the register parameters from IRQ handler functions. Signed-off-by: Jan Beulich <jbeulich@suse.com>