From: Denis Mukhin <dmukhin@ford.com> 

Polling is relevant for early boot only where facilities requiring
run_in_exception_handler() are not used (e.g. 'd' keyhandler).

Rework ns16550_poll() by droppping use of run_in_exception_handler().

The ground work for run_in_exception_handler() removal was done under XSA-453:
https://xenbits.xen.org/xsa/advisory-453.html

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Denis Mukhin <dmukhin@ford.com>
---
 xen/drivers/char/ns16550.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
index 0bbbafb49f6d..c10bff596b3b 100644
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -214,21 +214,14 @@ static void cf_check ns16550_interrupt(int irq, void *dev_id)
     }
 }
 
-/* Safe: ns16550_poll() runs as softirq so not reentrant on a given CPU. */
-static DEFINE_PER_CPU(struct serial_port *, poll_port);
-
-static void cf_check __ns16550_poll(const struct cpu_user_regs *regs)
+static void cf_check ns16550_poll(void *data)
 {
-    struct serial_port *port = this_cpu(poll_port);
+    struct serial_port *port = data;
     struct ns16550 *uart = port->uart;
-    const 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) )
@@ -241,16 +234,9 @@ static void cf_check __ns16550_poll(const struct cpu_user_regs *regs)
         serial_tx_interrupt(port);
 
 out:
-    set_irq_regs(old_regs);
     set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms));
 }
 
-static void cf_check ns16550_poll(void *data)
-{
-    this_cpu(poll_port) = data;
-    run_in_exception_handler(__ns16550_poll);
-}
-
 static int cf_check ns16550_tx_ready(struct serial_port *port)
 {
     struct ns16550 *uart = port->uart;
-- 
2.34.1

On 28/07/2025 9:23 pm, dmkhn@proton.me wrote:
> From: Denis Mukhin <dmukhin@ford.com> 
>
> Polling is relevant for early boot only where facilities requiring
> run_in_exception_handler() are not used (e.g. 'd' keyhandler).
>
> Rework ns16550_poll() by droppping use of run_in_exception_handler().
>
> The ground work for run_in_exception_handler() removal was done under XSA-453:
> https://xenbits.xen.org/xsa/advisory-453.html
>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Denis Mukhin <dmukhin@ford.com>

I wanted to do a before/after comparison, but interestingly I can't even
get poll to trigger.

(XEN) Wallclock source: CMOS RTC
(XEN) 'd' pressed -> dumping registers
(XEN) 
(XEN) *** Dumping CPU0 host state: ***
(XEN) ----[ Xen-4.21-unstable  x86_64  debug=y  Not tainted ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d040203160>] __x86_return_thunk+0/0xea0
(XEN) RFLAGS: 0000000000000206   CONTEXT: hypervisor
...
(XEN) Xen call trace:
(XEN)    [<ffff82d040203160>] R __x86_return_thunk+0/0xea0
(XEN)    [<ffff82d040268e75>] S drivers/char/ns16550.c#ns16550_setup_postirq+0x3f/0x61
(XEN)    [<ffff82d0406175e6>] F drivers/char/ns16550.c#ns16550_init_postirq+0x15f/0x311
(XEN)    [<ffff82d0406199c6>] F serial_init_postirq+0x21/0x5f
(XEN)    [<ffff82d04061722b>] F console_init_postirq+0x9/0x59
(XEN)    [<ffff82d040641e5f>] F __start_xen+0x1bf0/0x23ba
(XEN)    [<ffff82d0402043be>] F __high_start+0x8e/0x90
(XEN) 
(XEN) Allocated console ring of 16 KiB.
(XEN) HVM: ASIDs enabled.


The backtrace is weird because of speculation fixes.  We're actually in
ns_write_reg() having just enabled receive interrupts.  (Maybe we should
have LER active by default in debug builds when retthunk enabled, so we
can see which function we were really in.)

Back to the stacktrace, this is as we're turning interrupts on, rather
than during preirq.  Digging into the code, the thing which starts
polling off is:

    init_timer(&uart->timer, ns16550_poll, port, 0);

in ns16550_init_postirq(), which seems wrong.

__ns16550_poll() (renamed in this patch) discards the timer as soon as
uart->intr_works is seen, and that's clearly coming through ahead of the
timer first firing.

So, polling is activated automatically in init_postirq(), and turns
itself off when the first interrupt is seen.  (This now explains the
weird behaviour I saw when working on the FRED series.)

Doesn't this mean that when there's no input to the console, we've just
got a timer firing every uart->timeout_ms?

Irrespective of ^, we should take the timer out of the timer list when
we're done with it.

~Andrew