Re: [PATCH v1 2/2] drivers/ns16550: remove use of run_in_exception_handler()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/07/2025 9:23 pm, dmkhn@xxxxxxxxx wrote:
> From: Denis Mukhin <dmukhin@xxxxxxxx> 
>
> 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@xxxxxxxxxx>
> Signed-off-by: Denis Mukhin <dmukhin@xxxxxxxx>

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