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
