[PATCH v3 0/8] limit passing around of cpu_user_regs

[PATCH v3 0/8] limit passing around of cpu_user_regs

[Jan Beulich]

Unlike (synchronous) exception handlers, interrupt handlers don't normally
have a need to know the outer context's register state. Similarly, the vast
majority of key handlers has no need for such.

1: serial: fake IRQ-regs context in poll handlers
2: keyhandler: drop regs parameter from handle_keyregs()
3: serial: drop serial_rx_fn's regs parameter
4: PV-shim: drop pv_console_rx()'s regs parameter
5: serial: drop serial_[rt]x_interrupt()'s regs parameter
6: IRQ: drop regs parameter from handler functions
7: x86/APIC: drop regs parameter from direct vector handler functions
8: consolidate do_bug_frame() / bug_fn_t

Jan

[PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

In preparation of dropping the register parameters from
serial_[rt]x_interrupt() and in turn from IRQ handler functions,
register state needs making available another way for the few key
handlers which need it. Fake IRQ-like state.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
other console poll functions we have, and it's unclear whether that's
actually generally correct.

Andrew suggested to move set_irq_regs() to BUGFRAME_run_fn handling;
it's not clear to me whether that would be (a) correct from an abstract
pov (that's exception, not interrupt context after all) and (b) really
beneficial.
---
v2: New.

--- 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,12 +1269,17 @@ 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);
 
     if ( empty )
         serial_tx_interrupt(port, regs);
 
+    set_irq_regs(old_regs);
+
     if ( spin_trylock_irqsave(&port->tx_lock, flags) )
     {
         if ( dbgp->state == dbgp_idle && !dbgp->in.chunk &&
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -211,10 +211,14 @@ 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) )
@@ -227,6 +231,7 @@ static void cf_check __ns16550_poll(stru
         serial_tx_interrupt(port, regs);
 
 out:
+    set_irq_regs(old_regs);
     set_timer(&uart->timer, NOW() + MILLISECS(uart->timeout_ms));
 }
 
--- 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_tx_interrupt(port, guest_cpu_user_regs());
+
+    set_irq_regs(old_regs);
     set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL));
 }
 

[PATCH v3 2/8] keyhandler: drop regs parameter from handle_keyregs()

[Jan Beulich]

In preparation for further removal of regs parameters, drop it here. In
the two places where it's actually needed, retrieve IRQ context if
available, or else guest context.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
As an alternative to the new boolean parameter, I wonder if we couldn't
special-case the idle vCPU case: It's only there where we would not have
proper context retrievable via guest_cpu_user_regs().
---
v3: Re-base.
v2: New.

--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -73,12 +73,12 @@ static struct keyhandler {
 
 static void cf_check keypress_action(void *unused)
 {
-    handle_keypress(keypress_key, NULL);
+    handle_keypress(keypress_key, true);
 }
 
 static DECLARE_TASKLET(keypress_tasklet, keypress_action, NULL);
 
-void handle_keypress(unsigned char key, struct cpu_user_regs *regs)
+void handle_keypress(unsigned char key, bool need_context)
 {
     struct keyhandler *h;
 
@@ -88,7 +88,7 @@ void handle_keypress(unsigned char key,
     if ( !in_irq() || h->irq_callback )
     {
         console_start_log_everything();
-        h->irq_callback ? h->irq_fn(key, regs) : h->fn(key);
+        h->irq_callback ? h->irq_fn(key, need_context) : h->fn(key);
         console_end_log_everything();
     }
     else
@@ -169,7 +169,7 @@ void cf_check dump_execstate(struct cpu_
 }
 
 static void cf_check dump_registers(
-    unsigned char key, struct cpu_user_regs *regs)
+    unsigned char key, bool need_context)
 {
     unsigned int cpu;
 
@@ -182,8 +182,8 @@ static void cf_check dump_registers(
     cpumask_copy(&dump_execstate_mask, &cpu_online_map);
 
     /* Get local execution state out immediately, in case we get stuck. */
-    if ( regs )
-        dump_execstate(regs);
+    if ( !need_context )
+        dump_execstate(get_irq_regs() ?: guest_cpu_user_regs());
     else
         run_in_exception_handler(dump_execstate);
 
@@ -245,8 +245,7 @@ static void cf_check dump_hwdom_register
     }
 }
 
-static void cf_check reboot_machine(
-    unsigned char key, struct cpu_user_regs *regs)
+static void cf_check reboot_machine(unsigned char key, bool unused)
 {
     printk("'%c' pressed -> rebooting machine\n", key);
     machine_restart(0);
@@ -474,8 +473,7 @@ static void cf_check run_all_nonirq_keyh
 static DECLARE_TASKLET(run_all_keyhandlers_tasklet,
                        run_all_nonirq_keyhandlers, NULL);
 
-static void cf_check run_all_keyhandlers(
-    unsigned char key, struct cpu_user_regs *regs)
+static void cf_check run_all_keyhandlers(unsigned char key, bool need_context)
 {
     struct keyhandler *h;
     unsigned int k;
@@ -491,7 +489,7 @@ static void cf_check run_all_keyhandlers
         if ( !h->irq_fn || !h->diagnostic || !h->irq_callback )
             continue;
         printk("[%c: %s]\n", k, h->desc);
-        h->irq_fn(k, regs);
+        h->irq_fn(k, need_context);
     }
 
     watchdog_enable();
@@ -500,8 +498,7 @@ static void cf_check run_all_keyhandlers
     tasklet_schedule(&run_all_keyhandlers_tasklet);
 }
 
-static void cf_check do_toggle_alt_key(
-    unsigned char key, struct cpu_user_regs *regs)
+static void cf_check do_toggle_alt_key(unsigned char key, bool unused)
 {
     alt_key_handling = !alt_key_handling;
     printk("'%c' pressed -> using %s key handling\n", key,
@@ -566,7 +563,7 @@ void keyhandler_crash_action(enum crash_
         if ( *action == '+' )
             mdelay(10);
         else
-            handle_keypress(*action, NULL);
+            handle_keypress(*action, true);
         action++;
     }
 }
--- a/xen/common/sysctl.c
+++ b/xen/common/sysctl.c
@@ -134,7 +134,7 @@ long do_sysctl(XEN_GUEST_HANDLE_PARAM(xe
         {
             if ( copy_from_guest_offset(&c, op->u.debug_keys.keys, i, 1) )
                 goto out;
-            handle_keypress(c, guest_cpu_user_regs());
+            handle_keypress(c, false);
         }
         ret = 0;
         copyback = 0;
--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -279,7 +279,7 @@ static int *__read_mostly upper_thresh_a
 static int *__read_mostly lower_thresh_adj = &xenlog_lower_thresh;
 static const char *__read_mostly thresh_adj = "standard";
 
-static void cf_check do_toggle_guest(unsigned char key, struct cpu_user_regs *regs)
+static void cf_check do_toggle_guest(unsigned char key, bool unused)
 {
     if ( upper_thresh_adj == &xenlog_upper_thresh )
     {
@@ -306,13 +306,13 @@ static void do_adj_thresh(unsigned char
            loglvl_str(*upper_thresh_adj));
 }
 
-static void cf_check do_inc_thresh(unsigned char key, struct cpu_user_regs *regs)
+static void cf_check do_inc_thresh(unsigned char key, bool unused)
 {
     ++*lower_thresh_adj;
     do_adj_thresh(key);
 }
 
-static void cf_check do_dec_thresh(unsigned char key, struct cpu_user_regs *regs)
+static void cf_check do_dec_thresh(unsigned char key, bool unused)
 {
     if ( *lower_thresh_adj )
         --*lower_thresh_adj;
@@ -531,7 +531,7 @@ static void __serial_rx(char c, struct c
     switch ( console_rx )
     {
     case 0:
-        return handle_keypress(c, regs);
+        return handle_keypress(c, false);
 
     case 1:
         /*
--- a/xen/include/xen/keyhandler.h
+++ b/xen/include/xen/keyhandler.h
@@ -24,9 +24,8 @@ typedef void (keyhandler_fn_t)(unsigned
  *
  * Called in hardirq context with interrupts disabled.
  */
-struct cpu_user_regs;
 typedef void (irq_keyhandler_fn_t)(unsigned char key,
-                                   struct cpu_user_regs *regs);
+                                   bool need_context);
 
 /* Initialize keytable with default handlers. */
 void initialize_keytable(void);
@@ -46,7 +45,7 @@ void register_irq_keyhandler(unsigned ch
                              bool diagnostic);
 
 /* Inject a keypress into the key-handling subsystem. */
-extern void handle_keypress(unsigned char key, struct cpu_user_regs *regs);
+extern void handle_keypress(unsigned char key, bool need_context);
 
 enum crash_reason {
     CRASHREASON_PANIC,

[PATCH v3 3/8] serial: drop serial_rx_fn's regs parameter

[Jan Beulich]

It's simply not needed anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Re-base over earlier (new/split) patches.

--- a/xen/drivers/char/console.c
+++ b/xen/drivers/char/console.c
@@ -526,7 +526,7 @@ static void switch_serial_input(void)
     printk("\n");
 }
 
-static void __serial_rx(char c, struct cpu_user_regs *regs)
+static void __serial_rx(char c)
 {
     switch ( console_rx )
     {
@@ -578,7 +578,7 @@ static void __serial_rx(char c, struct c
 #endif
 }
 
-static void cf_check serial_rx(char c, struct cpu_user_regs *regs)
+static void cf_check serial_rx(char c)
 {
     static int switch_code_count = 0;
 
@@ -594,10 +594,10 @@ static void cf_check serial_rx(char c, s
     }
 
     for ( ; switch_code_count != 0; switch_code_count-- )
-        __serial_rx(switch_code, regs);
+        __serial_rx(switch_code);
 
     /* Finally process the just-received character. */
-    __serial_rx(c, regs);
+    __serial_rx(c);
 }
 
 static void cf_check notify_dom0_con_ring(void *unused)
--- a/xen/drivers/char/serial.c
+++ b/xen/drivers/char/serial.c
@@ -68,7 +68,7 @@ void serial_rx_interrupt(struct serial_p
     spin_unlock_irqrestore(&port->rx_lock, flags);
 
     if ( fn != NULL )
-        (*fn)(c & 0x7f, regs);
+        fn(c & 0x7f);
 }
 
 void serial_tx_interrupt(struct serial_port *port, struct cpu_user_regs *regs)
--- a/xen/drivers/char/xen_pv_console.c
+++ b/xen/drivers/char/xen_pv_console.c
@@ -118,7 +118,7 @@ size_t pv_console_rx(struct cpu_user_reg
     {
         c = cons_ring->in[MASK_XENCONS_IDX(cons++, cons_ring->in)];
         if ( cons_rx_handler )
-            cons_rx_handler(c, regs);
+            cons_rx_handler(c);
         recv++;
     }
 
--- a/xen/include/xen/serial.h
+++ b/xen/include/xen/serial.h
@@ -15,7 +15,7 @@
 struct cpu_user_regs;
 
 /* Register a character-receive hook on the specified COM port. */
-typedef void (*serial_rx_fn)(char c, struct cpu_user_regs *regs);
+typedef void (*serial_rx_fn)(char c);
 void serial_set_rx_handler(int handle, serial_rx_fn fn);
 
 /* Number of characters we buffer for a polling receiver. */

[PATCH v3 4/8] PV-shim: drop pv_console_rx()'s regs parameter

[Jan Beulich]

It's not needed anymore. This is in preparation of dropping the register
parameters from IRQ handler functions.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -181,7 +181,7 @@ static void cf_check xen_evtchn_upcall(s
             port += l1 * BITS_PER_LONG;
 
             if ( pv_console && port == pv_console_evtchn() )
-                pv_console_rx(regs);
+                pv_console_rx();
             else if ( pv_shim )
                 pv_shim_inject_evtchn(port);
         }
--- a/xen/drivers/char/xen_pv_console.c
+++ b/xen/drivers/char/xen_pv_console.c
@@ -94,7 +94,7 @@ evtchn_port_t pv_console_evtchn(void)
     return cons_evtchn;
 }
 
-size_t pv_console_rx(struct cpu_user_regs *regs)
+size_t pv_console_rx(void)
 {
     char c;
     XENCONS_RING_IDX cons, prod;
--- a/xen/include/xen/pv_console.h
+++ b/xen/include/xen/pv_console.h
@@ -9,7 +9,7 @@ void pv_console_init(void);
 void pv_console_set_rx_handler(serial_rx_fn fn);
 void pv_console_init_postirq(void);
 void pv_console_puts(const char *buf, size_t nr);
-size_t pv_console_rx(struct cpu_user_regs *regs);
+size_t pv_console_rx(void);
 evtchn_port_t pv_console_evtchn(void);
 
 #else
@@ -18,7 +18,7 @@ static inline void pv_console_init(void)
 static inline void pv_console_set_rx_handler(serial_rx_fn fn) { }
 static inline void pv_console_init_postirq(void) { }
 static inline void pv_console_puts(const char *buf, size_t nr) { }
-static inline size_t pv_console_rx(struct cpu_user_regs *regs) { return 0; }
+static inline size_t pv_console_rx(void) { return 0; }
 
 #endif /* !CONFIG_XEN_GUEST */
 #endif /* __XEN_PV_CONSOLE_H__ */

[PATCH v3 5/8] serial: drop serial_[rt]x_interrupt()'s regs parameter

[Jan Beulich]

They're simply not needed anymore.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Setting of IRQ regs split off to an earlier patch.

--- 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
@@ -1273,10 +1273,10 @@ static void cf_check _ehci_dbgp_poll(str
     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);
 
--- 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
@@ -224,11 +224,11 @@ static void cf_check __ns16550_poll(stru
         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);
--- 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
@@ -1180,9 +1180,9 @@ static void cf_check dbc_uart_poll(void
     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, guest_cpu_user_regs());
+    serial_tx_interrupt(port);
 
     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.

[PATCH v3 6/8] IRQ: drop regs parameter from handler functions

[Jan Beulich]

It's simply not needed anymore. Note how Linux made this change many
years ago already, in 2.6.19 (late 2006, see [1]).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <jgrall@amazon.com>

[1] https://git.kernel.org/torvalds/c/7d12e780e003f93433d49ce78cfedf4b4c52adc5
---
v2: Arm build fixes.

--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -397,7 +397,7 @@ void gic_interrupt(struct cpu_user_regs
     } while (1);
 }
 
-static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
+static void maintenance_interrupt(int irq, void *dev_id)
 {
     /*
      * This is a dummy interrupt handler.
--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -182,8 +182,7 @@ void irq_set_affinity(struct irq_desc *d
 }
 
 int request_irq(unsigned int irq, unsigned int irqflags,
-                void (*handler)(int irq, void *dev_id,
-                                struct cpu_user_regs *regs),
+                void (*handler)(int irq, void *dev_id),
                 const char *devname, void *dev_id)
 {
     struct irqaction *action;
@@ -276,7 +275,7 @@ void do_IRQ(struct cpu_user_regs *regs,
 
     do
     {
-        action->handler(irq, action->dev_id, regs);
+        action->handler(irq, action->dev_id);
         action = action->next;
     } while ( action );
 
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -241,7 +241,7 @@ int reprogram_timer(s_time_t timeout)
 }
 
 /* Handle the firing timer */
-static void htimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
+static void htimer_interrupt(int irq, void *dev_id)
 {
     if ( unlikely(!(READ_SYSREG(CNTHP_CTL_EL2) & CNTx_CTL_PENDING)) )
         return;
@@ -255,7 +255,7 @@ static void htimer_interrupt(int irq, vo
     WRITE_SYSREG(0, CNTHP_CTL_EL2);
 }
 
-static void vtimer_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs)
+static void vtimer_interrupt(int irq, void *dev_id)
 {
     /*
      * Edge-triggered interrupts can be used for the virtual timer. Even
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -962,7 +962,7 @@ static int __init cf_check irq_ratelimit
 __initcall(irq_ratelimit_init);
 
 int __init request_irq(unsigned int irq, unsigned int irqflags,
-        void (*handler)(int irq, void *dev_id, struct cpu_user_regs *regs),
+        void (*handler)(int irq, void *dev_id),
         const char * devname, void *dev_id)
 {
     struct irqaction * action;
@@ -2009,7 +2009,7 @@ void do_IRQ(struct cpu_user_regs *regs)
         spin_unlock_irq(&desc->lock);
 
         tsc_in = tb_init_done ? get_cycles() : 0;
-        action->handler(irq, action->dev_id, regs);
+        action->handler(irq, action->dev_id);
         TRACE_3D(TRC_HW_IRQ_HANDLED, irq, tsc_in, get_cycles());
 
         spin_lock_irq(&desc->lock);
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -237,8 +237,7 @@ again:
     }
 }
 
-static void cf_check hpet_interrupt_handler(
-    int irq, void *data, struct cpu_user_regs *regs)
+static void cf_check hpet_interrupt_handler(int irq, void *data)
 {
     struct hpet_event_channel *ch = data;
 
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -198,8 +198,7 @@ static void smp_send_timer_broadcast_ipi
     }
 }
 
-static void cf_check timer_interrupt(
-    int irq, void *dev_id, struct cpu_user_regs *regs)
+static void cf_check timer_interrupt(int irq, void *dev_id)
 {
     ASSERT(local_irq_is_enabled());
 
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -29,7 +29,7 @@ int init_one_irq_desc(struct irq_desc *d
     return err;
 }
 
-void cf_check no_action(int cpl, void *dev_id, struct cpu_user_regs *regs)
+void cf_check no_action(int cpl, void *dev_id)
 {
 }
 
--- a/xen/drivers/char/cadence-uart.c
+++ b/xen/drivers/char/cadence-uart.c
@@ -40,7 +40,7 @@ static struct cuart {
 #define cuart_read(uart, off)           readl((uart)->regs + (off))
 #define cuart_write(uart, off,val)      writel((val), (uart)->regs + (off))
 
-static void cuart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
+static void cuart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct cuart *uart = port->uart;
--- a/xen/drivers/char/exynos4210-uart.c
+++ b/xen/drivers/char/exynos4210-uart.c
@@ -45,7 +45,7 @@ static struct exynos4210_uart {
 #define exynos4210_read(uart, off)          readl((uart)->regs + off)
 #define exynos4210_write(uart, off, val)    writel(val, (uart->regs) + off)
 
-static void exynos4210_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
+static void exynos4210_uart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct exynos4210_uart *uart = port->uart;
--- a/xen/drivers/char/imx-lpuart.c
+++ b/xen/drivers/char/imx-lpuart.c
@@ -37,8 +37,7 @@ static struct imx_lpuart {
     struct vuart_info vuart;
 } imx8_com;
 
-static void imx_lpuart_interrupt(int irq, void *data,
-                                 struct cpu_user_regs *regs)
+static void imx_lpuart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct imx_lpuart *uart = port->uart;
--- a/xen/drivers/char/meson-uart.c
+++ b/xen/drivers/char/meson-uart.c
@@ -61,8 +61,7 @@ static struct meson_uart {
     struct vuart_info vuart;
 } meson_com;
 
-static void meson_uart_interrupt(int irq, void *data,
-                                 struct cpu_user_regs *regs)
+static void meson_uart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct meson_uart *uart = port->uart;
--- a/xen/drivers/char/mvebu-uart.c
+++ b/xen/drivers/char/mvebu-uart.c
@@ -67,8 +67,7 @@ static struct mvebu3700_uart {
 #define mvebu3700_read(uart, off)           readl((uart)->regs + (off))
 #define mvebu3700_write(uart, off, val)     writel(val, (uart)->regs + (off))
 
-static void mvebu3700_uart_interrupt(int irq, void *data,
-                                     struct cpu_user_regs *regs)
+static void mvebu3700_uart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct mvebu3700_uart *uart = port->uart;
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -175,8 +175,7 @@ static void handle_dw_usr_busy_quirk(str
     }
 }
 
-static void cf_check ns16550_interrupt(
-    int irq, void *dev_id, struct cpu_user_regs *regs)
+static void cf_check ns16550_interrupt(int irq, void *dev_id)
 {
     struct serial_port *port = dev_id;
     struct ns16550 *uart = port->uart;
--- a/xen/drivers/char/omap-uart.c
+++ b/xen/drivers/char/omap-uart.c
@@ -59,7 +59,7 @@ static struct omap_uart {
     struct vuart_info vuart;
 } omap_com = {0};
 
-static void omap_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
+static void omap_uart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct omap_uart *uart = port->uart;
--- a/xen/drivers/char/pl011.c
+++ b/xen/drivers/char/pl011.c
@@ -82,7 +82,7 @@ static unsigned int pl011_intr_status(st
     return (pl011_read(uart, RIS) & pl011_read(uart, IMSC));
 }
 
-static void pl011_interrupt(int irq, void *data, struct cpu_user_regs *regs)
+static void pl011_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct pl011 *uart = port->uart;
--- a/xen/drivers/char/scif-uart.c
+++ b/xen/drivers/char/scif-uart.c
@@ -102,7 +102,7 @@ static const struct port_params port_par
     },
 };
 
-static void scif_uart_interrupt(int irq, void *data, struct cpu_user_regs *regs)
+static void scif_uart_interrupt(int irq, void *data)
 {
     struct serial_port *port = data;
     struct scif_uart *uart = port->uart;
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -718,8 +718,7 @@ static void cf_check do_amd_iommu_irq(vo
 
 static DECLARE_SOFTIRQ_TASKLET(amd_iommu_irq_tasklet, do_amd_iommu_irq, NULL);
 
-static void cf_check iommu_interrupt_handler(
-    int irq, void *dev_id, struct cpu_user_regs *regs)
+static void cf_check iommu_interrupt_handler(int irq, void *dev_id)
 {
     unsigned long flags;
     struct amd_iommu *iommu = dev_id;
--- a/xen/drivers/passthrough/arm/ipmmu-vmsa.c
+++ b/xen/drivers/passthrough/arm/ipmmu-vmsa.c
@@ -642,7 +642,7 @@ static void ipmmu_domain_irq(struct ipmm
                         domain->d, status, iova);
 }
 
-static void ipmmu_irq(int irq, void *dev, struct cpu_user_regs *regs)
+static void ipmmu_irq(int irq, void *dev)
 {
     struct ipmmu_vmsa_device *mmu = dev;
     unsigned int i;
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1097,16 +1097,14 @@ static irqreturn_t arm_smmu_global_fault
 }
 
 /* Xen: Interrupt handlers wrapper */
-static void arm_smmu_context_fault_xen(int irq, void *dev,
-				       struct cpu_user_regs *regs)
+static void arm_smmu_context_fault_xen(int irq, void *dev)
 {
 	arm_smmu_context_fault(irq, dev);
 }
 
 #define arm_smmu_context_fault arm_smmu_context_fault_xen
 
-static void arm_smmu_global_fault_xen(int irq, void *dev,
-				      struct cpu_user_regs *regs)
+static void arm_smmu_global_fault_xen(int irq, void *dev)
 {
 	arm_smmu_global_fault(irq, dev);
 }
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -880,8 +880,7 @@ static void arm_smmu_priq_tasklet(void *
 
 static int arm_smmu_device_disable(struct arm_smmu_device *smmu);
 
-static void arm_smmu_gerror_handler(int irq, void *dev,
-				struct cpu_user_regs *regs)
+static void arm_smmu_gerror_handler(int irq, void *dev)
 {
 	u32 gerror, gerrorn, active;
 	struct arm_smmu_device *smmu = dev;
@@ -926,12 +925,11 @@ static void arm_smmu_gerror_handler(int
 	writel(gerror, smmu->base + ARM_SMMU_GERRORN);
 }
 
-static void arm_smmu_combined_irq_handler(int irq, void *dev,
-				struct cpu_user_regs *regs)
+static void arm_smmu_combined_irq_handler(int irq, void *dev)
 {
 	struct arm_smmu_device *smmu = dev;
 
-	arm_smmu_gerror_handler(irq, dev, regs);
+	arm_smmu_gerror_handler(irq, dev);
 
 	tasklet_schedule(&(smmu->combined_irq_tasklet));
 }
@@ -945,16 +943,14 @@ static void arm_smmu_combined_irq_taskle
 		arm_smmu_priq_tasklet(dev);
 }
 
-static void arm_smmu_evtq_irq_tasklet(int irq, void *dev,
-				struct cpu_user_regs *regs)
+static void arm_smmu_evtq_irq_tasklet(int irq, void *dev)
 {
 	struct arm_smmu_device *smmu = dev;
 
 	tasklet_schedule(&(smmu->evtq_irq_tasklet));
 }
 
-static void arm_smmu_priq_irq_tasklet(int irq, void *dev,
-				struct cpu_user_regs *regs)
+static void arm_smmu_priq_irq_tasklet(int irq, void *dev)
 {
 	struct arm_smmu_device *smmu = dev;
 
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1117,8 +1117,7 @@ static void cf_check do_iommu_page_fault
         __do_iommu_page_fault(drhd->iommu);
 }
 
-static void cf_check iommu_page_fault(
-    int irq, void *dev_id, struct cpu_user_regs *regs)
+static void cf_check iommu_page_fault(int irq, void *dev_id)
 {
     /*
      * Just flag the tasklet as runnable. This is fine, according to VT-d
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -18,7 +18,7 @@
     ASSERT(!in_irq() && (local_irq_is_enabled() || num_online_cpus() <= 1))
 
 struct irqaction {
-    void (*handler)(int irq, void *dev_id, struct cpu_user_regs *regs);
+    void (*handler)(int irq, void *dev_id);
     const char *name;
     void *dev_id;
     bool free_on_release;
@@ -119,12 +119,11 @@ extern int setup_irq(unsigned int irq, u
                      struct irqaction *new);
 extern void release_irq(unsigned int irq, const void *dev_id);
 extern int request_irq(unsigned int irq, unsigned int irqflags,
-               void (*handler)(int irq, void *dev_id,
-                     struct cpu_user_regs *regs),
+               void (*handler)(int irq, void *dev_id),
                const char *devname, void *dev_id);
 
 extern hw_irq_controller no_irq_type;
-void cf_check no_action(int cpl, void *dev_id, struct cpu_user_regs *regs);
+void cf_check no_action(int cpl, void *dev_id);
 unsigned int cf_check irq_startup_none(struct irq_desc *desc);
 void cf_check irq_actor_none(struct irq_desc *desc);
 #define irq_shutdown_none irq_actor_none

[PATCH v3 7/8] x86/APIC: drop regs parameter from direct vector handler functions

[Jan Beulich]

The only place it was needed is in the spurious handler, and there we
can use get_irq_regs() instead.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1322,7 +1322,7 @@ int reprogram_timer(s_time_t timeout)
     return apic_tmict || !timeout;
 }
 
-static void cf_check apic_timer_interrupt(struct cpu_user_regs *regs)
+static void cf_check apic_timer_interrupt(void)
 {
     ack_APIC_irq();
     perfc_incr(apic_timer);
@@ -1341,7 +1341,7 @@ void smp_send_state_dump(unsigned int cp
 /*
  * Spurious interrupts should _never_ happen with our APIC/SMP architecture.
  */
-static void cf_check spurious_interrupt(struct cpu_user_regs *regs)
+static void cf_check spurious_interrupt(void)
 {
     /*
      * Check if this is a vectored interrupt (most likely, as this is probably
@@ -1355,7 +1355,7 @@ static void cf_check spurious_interrupt(
         is_spurious = !nmi_check_continuation();
         if (this_cpu(state_dump_pending)) {
             this_cpu(state_dump_pending) = false;
-            dump_execstate(regs);
+            dump_execstate(get_irq_regs());
             is_spurious = false;
         }
 
@@ -1372,7 +1372,7 @@ static void cf_check spurious_interrupt(
  * This interrupt should never happen with our APIC/SMP architecture
  */
 
-static void cf_check error_interrupt(struct cpu_user_regs *regs)
+static void cf_check error_interrupt(void)
 {
     static const char *const esr_fields[] = {
         ", Send CS error",
@@ -1407,7 +1407,7 @@ static void cf_check error_interrupt(str
  * This interrupt handles performance counters interrupt
  */
 
-static void cf_check pmu_interrupt(struct cpu_user_regs *regs)
+static void cf_check pmu_interrupt(void)
 {
     ack_APIC_irq();
     vpmu_do_interrupt();
--- a/xen/arch/x86/cpu/mcheck/mce_intel.c
+++ b/xen/arch/x86/cpu/mcheck/mce_intel.c
@@ -58,7 +58,7 @@ bool __read_mostly lmce_support;
 #define MCE_RING                0x1
 static DEFINE_PER_CPU(int, last_state);
 
-static void cf_check intel_thermal_interrupt(struct cpu_user_regs *regs)
+static void cf_check intel_thermal_interrupt(void)
 {
     uint64_t msr_content;
     unsigned int cpu = smp_processor_id();
@@ -642,7 +642,7 @@ static void cpu_mcheck_disable(void)
         clear_cmci();
 }
 
-static void cf_check cmci_interrupt(struct cpu_user_regs *regs)
+static void cf_check cmci_interrupt(void)
 {
     mctelem_cookie_t mctc;
     struct mca_summary bs;
--- a/xen/arch/x86/guest/xen/xen.c
+++ b/xen/arch/x86/guest/xen/xen.c
@@ -158,7 +158,7 @@ static void __init init_memmap(void)
     }
 }
 
-static void cf_check xen_evtchn_upcall(struct cpu_user_regs *regs)
+static void cf_check xen_evtchn_upcall(void)
 {
     struct vcpu_info *vcpu_info = this_cpu(vcpu_info);
     unsigned long pending;
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -2776,7 +2776,7 @@ static struct hvm_function_table __initd
 };
 
 /* Handle VT-d posted-interrupt when VCPU is blocked. */
-static void cf_check pi_wakeup_interrupt(struct cpu_user_regs *regs)
+static void cf_check pi_wakeup_interrupt(void)
 {
     struct vmx_vcpu *vmx, *tmp;
     spinlock_t *lock = &per_cpu(vmx_pi_blocking, smp_processor_id()).lock;
@@ -2808,7 +2808,7 @@ static void cf_check pi_wakeup_interrupt
 }
 
 /* Handle VT-d posted-interrupt when VCPU is running. */
-static void cf_check pi_notification_interrupt(struct cpu_user_regs *regs)
+static void cf_check pi_notification_interrupt(void)
 {
     ack_APIC_irq();
     this_cpu(irq_count)++;
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -72,17 +72,15 @@ extern int opt_irq_vector_map;
 
 #define platform_legacy_irq(irq)	((irq) < 16)
 
-void cf_check event_check_interrupt(struct cpu_user_regs *regs);
-void cf_check invalidate_interrupt(struct cpu_user_regs *regs);
-void cf_check call_function_interrupt(struct cpu_user_regs *regs);
-void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs);
+void cf_check event_check_interrupt(void);
+void cf_check invalidate_interrupt(void);
+void cf_check call_function_interrupt(void);
+void cf_check irq_move_cleanup_interrupt(void);
 
 uint8_t alloc_hipriority_vector(void);
 
-void set_direct_apic_vector(
-    uint8_t vector, void (*handler)(struct cpu_user_regs *regs));
-void alloc_direct_apic_vector(
-    uint8_t *vector, void (*handler)(struct cpu_user_regs *regs));
+void set_direct_apic_vector(uint8_t vector, void (*handler)(void));
+void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void));
 
 void do_IRQ(struct cpu_user_regs *regs);
 
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -743,7 +743,7 @@ void move_native_irq(struct irq_desc *de
     desc->handler->enable(desc);
 }
 
-void cf_check irq_move_cleanup_interrupt(struct cpu_user_regs *regs)
+void cf_check irq_move_cleanup_interrupt(void)
 {
     unsigned vector, me;
 
@@ -913,16 +913,14 @@ uint8_t alloc_hipriority_vector(void)
     return next++;
 }
 
-static void (*direct_apic_vector[X86_NR_VECTORS])(struct cpu_user_regs *regs);
-void set_direct_apic_vector(
-    uint8_t vector, void (*handler)(struct cpu_user_regs *regs))
+static void (*direct_apic_vector[X86_NR_VECTORS])(void);
+void set_direct_apic_vector(uint8_t vector, void (*handler)(void))
 {
     BUG_ON(direct_apic_vector[vector] != NULL);
     direct_apic_vector[vector] = handler;
 }
 
-void alloc_direct_apic_vector(
-    uint8_t *vector, void (*handler)(struct cpu_user_regs *regs))
+void alloc_direct_apic_vector(uint8_t *vector, void (*handler)(void))
 {
     static DEFINE_SPINLOCK(lock);
 
@@ -1907,7 +1905,7 @@ void do_IRQ(struct cpu_user_regs *regs)
     if ( irq < 0 )
     {
         if ( direct_apic_vector[vector] )
-            direct_apic_vector[vector](regs);
+            direct_apic_vector[vector]();
         else
         {
             const char *kind = ", LAPIC";
--- a/xen/arch/x86/smp.c
+++ b/xen/arch/x86/smp.c
@@ -245,7 +245,7 @@ static cpumask_t flush_cpumask;
 static const void *flush_va;
 static unsigned int flush_flags;
 
-void cf_check invalidate_interrupt(struct cpu_user_regs *regs)
+void cf_check invalidate_interrupt(void)
 {
     unsigned int flags = flush_flags;
     ack_APIC_irq();
@@ -387,14 +387,14 @@ void smp_send_nmi_allbutself(void)
     send_IPI_mask(&cpu_online_map, APIC_DM_NMI);
 }
 
-void cf_check event_check_interrupt(struct cpu_user_regs *regs)
+void cf_check event_check_interrupt(void)
 {
     ack_APIC_irq();
     perfc_incr(ipis);
     this_cpu(irq_count)++;
 }
 
-void cf_check call_function_interrupt(struct cpu_user_regs *regs)
+void cf_check call_function_interrupt(void)
 {
     ack_APIC_irq();
     perfc_incr(ipis);

[PATCH v3 8/8] consolidate do_bug_frame() / bug_fn_t

[Jan Beulich]

The type not being used in do_bug_frame() is suspicious. Apparently
that's solely because the type uses a pointer-to-const parameter,
when so far run_in_exception_handler() wanted functions taking pointer-
to-non-const. Expand use of const, in turn requiring common code's
do_bug_frame() as well as [gs]et_irq_regs() to also gain const. This
then brings the former function also closer to the common one, with
Arm's use of vaddr_t remaining as a difference.

While there also replace the bogus use of hard tabs in [gs]et_irq_regs()
(I clearly didn't mean to put it in like this).

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
This is an alternative proposal to
https://lists.xen.org/archives/html/xen-devel/2023-12/msg01385.html.
---
v3: Retain / extend use of const. Make part of series.
v2: [skipped]

--- a/xen/arch/arm/irq.c
+++ b/xen/arch/arm/irq.c
@@ -220,7 +220,7 @@ void do_IRQ(struct cpu_user_regs *regs,
 {
     struct irq_desc *desc = irq_to_desc(irq);
     struct irqaction *action;
-    struct cpu_user_regs *old_regs = set_irq_regs(regs);
+    const struct cpu_user_regs *old_regs = set_irq_regs(regs);
 
     perfc_incr(irqs);
 
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -409,8 +409,7 @@ static always_inline void rep_nop(void)
 void show_code(const struct cpu_user_regs *regs);
 void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs);
 void show_registers(const struct cpu_user_regs *regs);
-#define dump_execution_state() \
-    run_in_exception_handler(show_execution_state_nonconst)
+#define dump_execution_state() run_in_exception_handler(show_execution_state)
 void show_page_walk(unsigned long addr);
 void noreturn fatal_trap(const struct cpu_user_regs *regs, bool show_remote);
 
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1896,7 +1896,7 @@ void do_IRQ(struct cpu_user_regs *regs)
     struct irq_desc  *desc;
     unsigned int      vector = (uint8_t)regs->entry_vector;
     int               irq = this_cpu(vector_irq)[vector];
-    struct cpu_user_regs *old_regs = set_irq_regs(regs);
+    const struct cpu_user_regs *old_regs = set_irq_regs(regs);
 
     perfc_incr(irqs);
     this_cpu(irq_count)++;
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -643,7 +643,7 @@ void show_stack_overflow(unsigned int cp
     printk("\n");
 }
 
-void show_execution_state(const struct cpu_user_regs *regs)
+void cf_check show_execution_state(const struct cpu_user_regs *regs)
 {
     /* Prevent interleaving of output. */
     unsigned long flags = console_lock_recursive_irqsave();
@@ -655,11 +655,6 @@ void show_execution_state(const struct c
     console_unlock_recursive_irqrestore(flags);
 }
 
-void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs)
-{
-    show_execution_state(regs);
-}
-
 void vcpu_show_execution_state(struct vcpu *v)
 {
     unsigned long flags = 0;
--- a/xen/common/bug.c
+++ b/xen/common/bug.c
@@ -10,7 +10,7 @@
  * Returns a negative value in case of an error otherwise
  * BUGFRAME_{run_fn, warn, bug, assert}
  */
-int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc)
+int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc)
 {
     const struct bug_frame *bug = NULL;
     const struct virtual_region *region;
@@ -44,14 +44,10 @@ int do_bug_frame(struct cpu_user_regs *r
 
     if ( id == BUGFRAME_run_fn )
     {
-        void (*fn)(struct cpu_user_regs *) = bug_ptr(bug);
+        bug_fn_t *fn = bug_ptr(bug);
 
         fn(regs);
 
-        /* Re-enforce consistent types, because of the casts involved. */
-        if ( false )
-            run_in_exception_handler(fn);
-
         return id;
     }
 
--- a/xen/common/irq.c
+++ b/xen/common/irq.c
@@ -1,7 +1,7 @@
 #include <xen/irq.h>
 #include <xen/errno.h>
 
-DEFINE_PER_CPU(struct cpu_user_regs *, irq_regs);
+DEFINE_PER_CPU(const struct cpu_user_regs *, irq_regs);
 
 int init_one_irq_desc(struct irq_desc *desc)
 {
--- a/xen/common/keyhandler.c
+++ b/xen/common/keyhandler.c
@@ -135,7 +135,7 @@ static void cf_check show_handlers(unsig
 
 static cpumask_t dump_execstate_mask;
 
-void cf_check dump_execstate(struct cpu_user_regs *regs)
+void cf_check dump_execstate(const struct cpu_user_regs *regs)
 {
     unsigned int cpu = smp_processor_id();
 
--- a/xen/drivers/char/ehci-dbgp.c
+++ b/xen/drivers/char/ehci-dbgp.c
@@ -1246,14 +1246,14 @@ static int cf_check ehci_dbgp_getc(struc
 /* Safe: ehci_dbgp_poll() runs as timer handler, so not reentrant. */
 static struct serial_port *poll_port;
 
-static void cf_check _ehci_dbgp_poll(struct cpu_user_regs *regs)
+static void cf_check _ehci_dbgp_poll(const struct cpu_user_regs *regs)
 {
     struct serial_port *port = poll_port;
     struct ehci_dbgp *dbgp = port->uart;
     unsigned long flags;
     unsigned int timeout = MICROSECS(DBGP_CHECK_INTERVAL);
     bool empty = false;
-    struct cpu_user_regs *old_regs;
+    const struct cpu_user_regs *old_regs;
 
     if ( !dbgp->ehci_debug )
         return;
--- a/xen/drivers/char/ns16550.c
+++ b/xen/drivers/char/ns16550.c
@@ -206,11 +206,11 @@ static void cf_check ns16550_interrupt(i
 /* 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(struct cpu_user_regs *regs)
+static void cf_check __ns16550_poll(const struct cpu_user_regs *regs)
 {
     struct serial_port *port = this_cpu(poll_port);
     struct ns16550 *uart = port->uart;
-    struct cpu_user_regs *old_regs;
+    const struct cpu_user_regs *old_regs;
 
     if ( uart->intr_works )
         return; /* Interrupts work - no more polling */
--- a/xen/drivers/char/xhci-dbc.c
+++ b/xen/drivers/char/xhci-dbc.c
@@ -1164,7 +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;
+    const struct cpu_user_regs *old_regs;
 
     if ( spin_trylock_irqsave(&port->tx_lock, flags) )
     {
--- a/xen/include/xen/bug.h
+++ b/xen/include/xen/bug.h
@@ -101,8 +101,7 @@ typedef void bug_fn_t(const struct cpu_u
 
 #ifndef run_in_exception_handler
 
-static void always_inline run_in_exception_handler(
-    void (*fn)(struct cpu_user_regs *regs))
+static void always_inline run_in_exception_handler(bug_fn_t *fn)
 {
     BUG_FRAME(BUGFRAME_run_fn, 0, fn, 0, NULL);
 }
@@ -133,7 +132,7 @@ static void always_inline run_in_excepti
  * Returns a negative value in case of an error otherwise
  * BUGFRAME_{run_fn, warn, bug, assert}
  */
-int do_bug_frame(struct cpu_user_regs *regs, unsigned long pc);
+int do_bug_frame(const struct cpu_user_regs *regs, unsigned long pc);
 
 #endif /* CONFIG_GENERIC_BUG_FRAME */
 
--- a/xen/include/xen/irq.h
+++ b/xen/include/xen/irq.h
@@ -134,21 +134,22 @@ void cf_check irq_actor_none(struct irq_
  * Per-cpu interrupted context register state - the inner-most interrupt frame
  * on the stack.
  */
-DECLARE_PER_CPU(struct cpu_user_regs *, irq_regs);
+DECLARE_PER_CPU(const struct cpu_user_regs *, irq_regs);
 
-static inline struct cpu_user_regs *get_irq_regs(void)
+static inline const struct cpu_user_regs *get_irq_regs(void)
 {
-	return this_cpu(irq_regs);
+    return this_cpu(irq_regs);
 }
 
-static inline struct cpu_user_regs *set_irq_regs(struct cpu_user_regs *new_regs)
+static inline const struct cpu_user_regs *set_irq_regs(
+    const struct cpu_user_regs *new_regs)
 {
-	struct cpu_user_regs *old_regs, **pp_regs = &this_cpu(irq_regs);
+    const struct cpu_user_regs *old_regs, **pp_regs = &this_cpu(irq_regs);
 
-	old_regs = *pp_regs;
-	*pp_regs = new_regs;
+    old_regs = *pp_regs;
+    *pp_regs = new_regs;
 
-	return old_regs;
+    return old_regs;
 }
 
 struct domain;
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -110,8 +110,7 @@ extern const unsigned int xen_config_dat
 struct cpu_user_regs;
 struct vcpu;
 
-void show_execution_state(const struct cpu_user_regs *regs);
-void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs);
+void cf_check show_execution_state(const struct cpu_user_regs *regs);
 void vcpu_show_execution_state(struct vcpu *v);
 
 #endif /* _LINUX_KERNEL_H */
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -173,7 +173,7 @@ extern char *print_tainted(char *str);
 extern void add_taint(unsigned int taint);
 
 struct cpu_user_regs;
-void cf_check dump_execstate(struct cpu_user_regs *regs);
+void cf_check dump_execstate(const struct cpu_user_regs *regs);
 
 void init_constructors(void);
 

Re: [PATCH v3 3/8] serial: drop serial_rx_fn's regs parameter

[Andrew Cooper]

On 05/02/2024 1:28 pm, Jan Beulich wrote:
> It's simply not needed anymore.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v3 5/8] serial: drop serial_[rt]x_interrupt()'s regs parameter

[Andrew Cooper]

On 05/02/2024 1:30 pm, Jan Beulich wrote:
> They're simply not needed anymore.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH v3 8/8] consolidate do_bug_frame() / bug_fn_t

[Andrew Cooper]

On 05/02/2024 1:32 pm, Jan Beulich wrote:
> The type not being used in do_bug_frame() is suspicious. Apparently
> that's solely because the type uses a pointer-to-const parameter,
> when so far run_in_exception_handler() wanted functions taking pointer-
> to-non-const. Expand use of const, in turn requiring common code's
> do_bug_frame() as well as [gs]et_irq_regs() to also gain const. This
> then brings the former function also closer to the common one, with
> Arm's use of vaddr_t remaining as a difference.
>
> While there also replace the bogus use of hard tabs in [gs]et_irq_regs()
> (I clearly didn't mean to put it in like this).

I meant to query that at the time and clearly forgot to.

> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm still confident we can get rid of the fake frame in the serial
drivers, but this is an improvement nonetheless.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I'll rebase my work over this.  It's going to collide horribly.

~Andrew

Re: [PATCH v3 8/8] consolidate do_bug_frame() / bug_fn_t

[Jan Beulich]

On 05.02.2024 14:51, Andrew Cooper wrote:
> On 05/02/2024 1:32 pm, Jan Beulich wrote:
>> The type not being used in do_bug_frame() is suspicious. Apparently
>> that's solely because the type uses a pointer-to-const parameter,
>> when so far run_in_exception_handler() wanted functions taking pointer-
>> to-non-const. Expand use of const, in turn requiring common code's
>> do_bug_frame() as well as [gs]et_irq_regs() to also gain const. This
>> then brings the former function also closer to the common one, with
>> Arm's use of vaddr_t remaining as a difference.
>>
>> While there also replace the bogus use of hard tabs in [gs]et_irq_regs()
>> (I clearly didn't mean to put it in like this).
> 
> I meant to query that at the time and clearly forgot to.
> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm still confident we can get rid of the fake frame in the serial
> drivers, but this is an improvement nonetheless.
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

> I'll rebase my work over this.  It's going to collide horribly.

For the patch here they're affected only because I stuck the patch at
the end of the series. I think it ought to be possible to move it to
the front, and then it could be left to be determined whether my
introducing of set_irq_regs() in the poll handlers could actually be
avoided by whatever work you have pending / in progress.

Jan

Re: [PATCH v3 8/8] consolidate do_bug_frame() / bug_fn_t

[Julien Grall]

Hi Jan,

On 05/02/2024 13:32, Jan Beulich wrote:
> The type not being used in do_bug_frame() is suspicious. Apparently
> that's solely because the type uses a pointer-to-const parameter,
> when so far run_in_exception_handler() wanted functions taking pointer-
> to-non-const. Expand use of const, in turn requiring common code's
> do_bug_frame() as well as [gs]et_irq_regs() to also gain const. This
> then brings the former function also closer to the common one, with
> Arm's use of vaddr_t remaining as a difference.
> 
> While there also replace the bogus use of hard tabs in [gs]et_irq_regs()
> (I clearly didn't mean to put it in like this).
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

For the Arm parts:

Acked-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Julien Grall]

Hi Jan,

On 05/02/2024 13:27, Jan Beulich wrote:
> In preparation of dropping the register parameters from
> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
> register state needs making available another way for the few key
> handlers which need it. Fake IRQ-like state.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
> other console poll functions we have, and it's unclear whether that's
> actually generally correct.

Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
run_in_exception() doesn't exist. But looking at the caller, no-on seems 
to care about the 'regs'. So is this just a latent bug?

BTW, do you have an idea why the poll function is not run in an 
exception handler?

> 
> Andrew suggested to move set_irq_regs() to BUGFRAME_run_fn handling;
> it's not clear to me whether that would be (a) correct from an abstract
> pov (that's exception, not interrupt context after all) 

I agree with that.

> and (b) really beneficial.

I guess this could help to reduce the amount of churn. I can't really 
make my mind whether this is worth it or not. So I would keep it as you did.

Cheers,

-- 
Julien Grall

Re: [PATCH v3 2/8] keyhandler: drop regs parameter from handle_keyregs()

[Julien Grall]

Hi Jan,

On 05/02/2024 13:28, Jan Beulich wrote:
> In preparation for further removal of regs parameters, drop it here. In
> the two places where it's actually needed, retrieve IRQ context if
> available, or else guest context.
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> As an alternative to the new boolean parameter, I wonder if we couldn't
> special-case the idle vCPU case: It's only there where we would not have
> proper context retrievable via guest_cpu_user_regs().

I am trying to understand the implication. Looking at the code, it seems 
in the case where we pass NULL, we would expect to call 
run_in_exception_handler().

If I am not mistaken, at least for Arm, regs would not be the same as 
guest_cpu_user_regs(). So I think your current approach is more correct.

Did I miss anything?

Cheers,

-- 
Julien Grall

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

On 08.02.2024 23:00, Julien Grall wrote:
> On 05/02/2024 13:27, Jan Beulich wrote:
>> In preparation of dropping the register parameters from
>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
>> register state needs making available another way for the few key
>> handlers which need it. Fake IRQ-like state.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
>> other console poll functions we have, and it's unclear whether that's
>> actually generally correct.
> 
> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
> to care about the 'regs'. So is this just a latent bug?

What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
And I can spot any use of guest_user_regs() on the respective generic
or Arm-specific bug.c paths.

> BTW, do you have an idea why the poll function is not run in an 
> exception handler?

"The poll function" being which one? If you mean the one in xhci-dbc.c
then that's why I had Cc-ed Marek. Moving him to To: - maybe that
manages to finally catch his attention.

>> Andrew suggested to move set_irq_regs() to BUGFRAME_run_fn handling;
>> it's not clear to me whether that would be (a) correct from an abstract
>> pov (that's exception, not interrupt context after all) 
> 
> I agree with that.
> 
>> and (b) really beneficial.
> 
> I guess this could help to reduce the amount of churn. I can't really 
> make my mind whether this is worth it or not. So I would keep it as you did.

Good, thanks.

Jan

Re: [PATCH v3 2/8] keyhandler: drop regs parameter from handle_keyregs()

[Jan Beulich]

On 08.02.2024 23:09, Julien Grall wrote:
> On 05/02/2024 13:28, Jan Beulich wrote:
>> In preparation for further removal of regs parameters, drop it here. In
>> the two places where it's actually needed, retrieve IRQ context if
>> available, or else guest context.
>>
>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> As an alternative to the new boolean parameter, I wonder if we couldn't
>> special-case the idle vCPU case: It's only there where we would not have
>> proper context retrievable via guest_cpu_user_regs().
> 
> I am trying to understand the implication. Looking at the code, it seems 
> in the case where we pass NULL, we would expect to call 
> run_in_exception_handler().

Right, when NULL was passed so far, and when true is passed now, that's
to indicate to invoke run_in_exception_handler().

> If I am not mistaken, at least for Arm, regs would not be the same as 
> guest_cpu_user_regs(). So I think your current approach is more correct.
> 
> Did I miss anything?

Whether regs are the same isn't overly relevant here. The thing that's
relevant is whether what would be logged actually makes sense. And
invoking guest_cpu_user_regs() in idle vCPU context makes no sense.
Whereas in other contexts its result is good enough to show the present
state of the CPU; there's no real need in such a case to go through
run_in_exception_handler().

The present approach therefore isn't necessarily "more correct", but it
is closer to prior behavior.

The corner case that makes me prefer the presently chosen approach is
when a CPU is in the middle of scheduling, especially considering x86's
lazy switching (when current != this_cpu(curr_vcpu). The main reason to
mention the alternative is because iirc Andrew had suggested to move
more in that direction.

Jan

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 2077 bytes --]

On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
> On 08.02.2024 23:00, Julien Grall wrote:
> > On 05/02/2024 13:27, Jan Beulich wrote:
> >> In preparation of dropping the register parameters from
> >> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
> >> register state needs making available another way for the few key
> >> handlers which need it. Fake IRQ-like state.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
> >> other console poll functions we have, and it's unclear whether that's
> >> actually generally correct.
> > 
> > Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
> > run_in_exception() doesn't exist. But looking at the caller, no-on seems 
> > to care about the 'regs'. So is this just a latent bug?
> 
> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
> And I can spot any use of guest_user_regs() on the respective generic
> or Arm-specific bug.c paths.
> 
> > BTW, do you have an idea why the poll function is not run in an 
> > exception handler?
> 
> "The poll function" being which one? If you mean the one in xhci-dbc.c
> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
> manages to finally catch his attention.

TBH, I don't know. That's part of the original xue patch at
https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
and it works for me as it is.

> >> Andrew suggested to move set_irq_regs() to BUGFRAME_run_fn handling;
> >> it's not clear to me whether that would be (a) correct from an abstract
> >> pov (that's exception, not interrupt context after all) 
> > 
> > I agree with that.
> > 
> >> and (b) really beneficial.
> > 
> > I guess this could help to reduce the amount of churn. I can't really 
> > make my mind whether this is worth it or not. So I would keep it as you did.
> 
> Good, thanks.
> 
> Jan

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

On 13.02.2024 04:43, Marek Marczykowski wrote:
> On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
>> On 08.02.2024 23:00, Julien Grall wrote:
>>> On 05/02/2024 13:27, Jan Beulich wrote:
>>>> In preparation of dropping the register parameters from
>>>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
>>>> register state needs making available another way for the few key
>>>> handlers which need it. Fake IRQ-like state.
>>>>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> ---
>>>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
>>>> other console poll functions we have, and it's unclear whether that's
>>>> actually generally correct.
>>>
>>> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
>>> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
>>> to care about the 'regs'. So is this just a latent bug?
>>
>> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
>> And I can spot any use of guest_user_regs() on the respective generic
>> or Arm-specific bug.c paths.
>>
>>> BTW, do you have an idea why the poll function is not run in an 
>>> exception handler?
>>
>> "The poll function" being which one? If you mean the one in xhci-dbc.c
>> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
>> manages to finally catch his attention.
> 
> TBH, I don't know. That's part of the original xue patch at
> https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
> and it works for me as it is.

"Works" meaning what? Doesn't crash on you? Or does also provide
sensible output in _all_ cases (i.e. including when e.g. the poll
happens to run on an idle vCPU)?

Jan

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 2065 bytes --]

On Tue, Feb 13, 2024 at 08:45:54AM +0100, Jan Beulich wrote:
> On 13.02.2024 04:43, Marek Marczykowski wrote:
> > On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
> >> On 08.02.2024 23:00, Julien Grall wrote:
> >>> On 05/02/2024 13:27, Jan Beulich wrote:
> >>>> In preparation of dropping the register parameters from
> >>>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
> >>>> register state needs making available another way for the few key
> >>>> handlers which need it. Fake IRQ-like state.
> >>>>
> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>> ---
> >>>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
> >>>> other console poll functions we have, and it's unclear whether that's
> >>>> actually generally correct.
> >>>
> >>> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
> >>> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
> >>> to care about the 'regs'. So is this just a latent bug?
> >>
> >> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
> >> And I can spot any use of guest_user_regs() on the respective generic
> >> or Arm-specific bug.c paths.
> >>
> >>> BTW, do you have an idea why the poll function is not run in an 
> >>> exception handler?
> >>
> >> "The poll function" being which one? If you mean the one in xhci-dbc.c
> >> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
> >> manages to finally catch his attention.
> > 
> > TBH, I don't know. That's part of the original xue patch at
> > https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
> > and it works for me as it is.
> 
> "Works" meaning what? Doesn't crash on you? Or does also provide
> sensible output in _all_ cases (i.e. including when e.g. the poll
> happens to run on an idle vCPU)?

Generally provides sensible output, for example during boot (it is using
idle vCPU then, right?).

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

On 13.02.2024 15:53, Marek Marczykowski wrote:
> On Tue, Feb 13, 2024 at 08:45:54AM +0100, Jan Beulich wrote:
>> On 13.02.2024 04:43, Marek Marczykowski wrote:
>>> On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
>>>> On 08.02.2024 23:00, Julien Grall wrote:
>>>>> On 05/02/2024 13:27, Jan Beulich wrote:
>>>>>> In preparation of dropping the register parameters from
>>>>>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
>>>>>> register state needs making available another way for the few key
>>>>>> handlers which need it. Fake IRQ-like state.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> ---
>>>>>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
>>>>>> other console poll functions we have, and it's unclear whether that's
>>>>>> actually generally correct.
>>>>>
>>>>> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
>>>>> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
>>>>> to care about the 'regs'. So is this just a latent bug?
>>>>
>>>> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
>>>> And I can spot any use of guest_user_regs() on the respective generic
>>>> or Arm-specific bug.c paths.
>>>>
>>>>> BTW, do you have an idea why the poll function is not run in an 
>>>>> exception handler?
>>>>
>>>> "The poll function" being which one? If you mean the one in xhci-dbc.c
>>>> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
>>>> manages to finally catch his attention.
>>>
>>> TBH, I don't know. That's part of the original xue patch at
>>> https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
>>> and it works for me as it is.
>>
>> "Works" meaning what? Doesn't crash on you? Or does also provide
>> sensible output in _all_ cases (i.e. including when e.g. the poll
>> happens to run on an idle vCPU)?
> 
> Generally provides sensible output, for example during boot (it is using
> idle vCPU then, right?).

Before Dom0 is started: Yes. With the exception of the phase where PV
Dom0's page tables are constructed, albeit in that time window
guest_cpu_user_regs() shouldn't yield sensible data either. I can only
say I'm surprised; since I have no way to properly test with an XHCI
debug port, I'd have to see about faking something to convince myself
(unless you were to supply example output).

Jan

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]

On Tue, Feb 13, 2024 at 04:00:32PM +0100, Jan Beulich wrote:
> On 13.02.2024 15:53, Marek Marczykowski wrote:
> > On Tue, Feb 13, 2024 at 08:45:54AM +0100, Jan Beulich wrote:
> >> On 13.02.2024 04:43, Marek Marczykowski wrote:
> >>> On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
> >>>> On 08.02.2024 23:00, Julien Grall wrote:
> >>>>> On 05/02/2024 13:27, Jan Beulich wrote:
> >>>>>> In preparation of dropping the register parameters from
> >>>>>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
> >>>>>> register state needs making available another way for the few key
> >>>>>> handlers which need it. Fake IRQ-like state.
> >>>>>>
> >>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>> ---
> >>>>>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
> >>>>>> other console poll functions we have, and it's unclear whether that's
> >>>>>> actually generally correct.
> >>>>>
> >>>>> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
> >>>>> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
> >>>>> to care about the 'regs'. So is this just a latent bug?
> >>>>
> >>>> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
> >>>> And I can spot any use of guest_user_regs() on the respective generic
> >>>> or Arm-specific bug.c paths.
> >>>>
> >>>>> BTW, do you have an idea why the poll function is not run in an 
> >>>>> exception handler?
> >>>>
> >>>> "The poll function" being which one? If you mean the one in xhci-dbc.c
> >>>> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
> >>>> manages to finally catch his attention.
> >>>
> >>> TBH, I don't know. That's part of the original xue patch at
> >>> https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
> >>> and it works for me as it is.
> >>
> >> "Works" meaning what? Doesn't crash on you? Or does also provide
> >> sensible output in _all_ cases (i.e. including when e.g. the poll
> >> happens to run on an idle vCPU)?
> > 
> > Generally provides sensible output, for example during boot (it is using
> > idle vCPU then, right?).
> 
> Before Dom0 is started: Yes. With the exception of the phase where PV
> Dom0's page tables are constructed, albeit in that time window
> guest_cpu_user_regs() shouldn't yield sensible data either. I can only
> say I'm surprised; since I have no way to properly test with an XHCI
> debug port, I'd have to see about faking something to convince myself
> (unless you were to supply example output).

Would you like me to test this series with xhci console? Or maybe add
some extra debug prints and include their output? But note, printk from
inside console code generally leads to deadlocks. What I did for some
debugging was to log into some separate buffer and dump it later.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

On 13.02.2024 16:11, Marek Marczykowski wrote:
> On Tue, Feb 13, 2024 at 04:00:32PM +0100, Jan Beulich wrote:
>> On 13.02.2024 15:53, Marek Marczykowski wrote:
>>> On Tue, Feb 13, 2024 at 08:45:54AM +0100, Jan Beulich wrote:
>>>> On 13.02.2024 04:43, Marek Marczykowski wrote:
>>>>> On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
>>>>>> On 08.02.2024 23:00, Julien Grall wrote:
>>>>>>> On 05/02/2024 13:27, Jan Beulich wrote:
>>>>>>>> In preparation of dropping the register parameters from
>>>>>>>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
>>>>>>>> register state needs making available another way for the few key
>>>>>>>> handlers which need it. Fake IRQ-like state.
>>>>>>>>
>>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>>> ---
>>>>>>>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
>>>>>>>> other console poll functions we have, and it's unclear whether that's
>>>>>>>> actually generally correct.
>>>>>>>
>>>>>>> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
>>>>>>> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
>>>>>>> to care about the 'regs'. So is this just a latent bug?
>>>>>>
>>>>>> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
>>>>>> And I can spot any use of guest_user_regs() on the respective generic
>>>>>> or Arm-specific bug.c paths.
>>>>>>
>>>>>>> BTW, do you have an idea why the poll function is not run in an 
>>>>>>> exception handler?
>>>>>>
>>>>>> "The poll function" being which one? If you mean the one in xhci-dbc.c
>>>>>> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
>>>>>> manages to finally catch his attention.
>>>>>
>>>>> TBH, I don't know. That's part of the original xue patch at
>>>>> https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
>>>>> and it works for me as it is.
>>>>
>>>> "Works" meaning what? Doesn't crash on you? Or does also provide
>>>> sensible output in _all_ cases (i.e. including when e.g. the poll
>>>> happens to run on an idle vCPU)?
>>>
>>> Generally provides sensible output, for example during boot (it is using
>>> idle vCPU then, right?).
>>
>> Before Dom0 is started: Yes. With the exception of the phase where PV
>> Dom0's page tables are constructed, albeit in that time window
>> guest_cpu_user_regs() shouldn't yield sensible data either. I can only
>> say I'm surprised; since I have no way to properly test with an XHCI
>> debug port, I'd have to see about faking something to convince myself
>> (unless you were to supply example output).
> 
> Would you like me to test this series with xhci console?

The behavior shouldn't really be connected to this series. But yes, 'd'
debug key output (just the part for the CPU the key handling was
actually invoked from) with the xhci debug console would be of
interest, for the case where that CPU at that time runs an idle vCPU.

> Or maybe add
> some extra debug prints and include their output? But note, printk from
> inside console code generally leads to deadlocks. What I did for some
> debugging was to log into some separate buffer and dump it later.

Right, this would be more involved.

Jan

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 9404 bytes --]

On Tue, Feb 13, 2024 at 04:44:04PM +0100, Jan Beulich wrote:
> On 13.02.2024 16:11, Marek Marczykowski wrote:
> > On Tue, Feb 13, 2024 at 04:00:32PM +0100, Jan Beulich wrote:
> >> On 13.02.2024 15:53, Marek Marczykowski wrote:
> >>> On Tue, Feb 13, 2024 at 08:45:54AM +0100, Jan Beulich wrote:
> >>>> On 13.02.2024 04:43, Marek Marczykowski wrote:
> >>>>> On Mon, Feb 12, 2024 at 10:04:38AM +0100, Jan Beulich wrote:
> >>>>>> On 08.02.2024 23:00, Julien Grall wrote:
> >>>>>>> On 05/02/2024 13:27, Jan Beulich wrote:
> >>>>>>>> In preparation of dropping the register parameters from
> >>>>>>>> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
> >>>>>>>> register state needs making available another way for the few key
> >>>>>>>> handlers which need it. Fake IRQ-like state.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >>>>>>>> ---
> >>>>>>>> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
> >>>>>>>> other console poll functions we have, and it's unclear whether that's
> >>>>>>>> actually generally correct.
> >>>>>>>
> >>>>>>> Is it? Looking at ns16550_poll() we would pass guest_user_regs() if 
> >>>>>>> run_in_exception() doesn't exist. But looking at the caller, no-on seems 
> >>>>>>> to care about the 'regs'. So is this just a latent bug?
> >>>>>>
> >>>>>> What do you mean by "doesn't exist"? ns16550_poll() assumes it exists.
> >>>>>> And I can spot any use of guest_user_regs() on the respective generic
> >>>>>> or Arm-specific bug.c paths.
> >>>>>>
> >>>>>>> BTW, do you have an idea why the poll function is not run in an 
> >>>>>>> exception handler?
> >>>>>>
> >>>>>> "The poll function" being which one? If you mean the one in xhci-dbc.c
> >>>>>> then that's why I had Cc-ed Marek. Moving him to To: - maybe that
> >>>>>> manages to finally catch his attention.
> >>>>>
> >>>>> TBH, I don't know. That's part of the original xue patch at
> >>>>> https://github.com/connojd/xue/blob/master/patches/xen-xue-dbgp.patch
> >>>>> and it works for me as it is.
> >>>>
> >>>> "Works" meaning what? Doesn't crash on you? Or does also provide
> >>>> sensible output in _all_ cases (i.e. including when e.g. the poll
> >>>> happens to run on an idle vCPU)?
> >>>
> >>> Generally provides sensible output, for example during boot (it is using
> >>> idle vCPU then, right?).
> >>
> >> Before Dom0 is started: Yes. With the exception of the phase where PV
> >> Dom0's page tables are constructed, albeit in that time window
> >> guest_cpu_user_regs() shouldn't yield sensible data either. I can only
> >> say I'm surprised; since I have no way to properly test with an XHCI
> >> debug port, I'd have to see about faking something to convince myself
> >> (unless you were to supply example output).
> > 
> > Would you like me to test this series with xhci console?
> 
> The behavior shouldn't really be connected to this series. But yes, 'd'
> debug key output (just the part for the CPU the key handling was
> actually invoked from) with the xhci debug console would be of
> interest, for the case where that CPU at that time runs an idle vCPU.

I managed to press 'd' before dom0 started. Full output at
https://gist.github.com/marmarek/a495cd666f4aafed3a5cfcb8393f515b (it's
Alder Lake, and smt=off, so CPU numbering is weird).
Interestingly, I do _not_ see output for CPU0, where I'd expect the
key handler to run... I see all the idle ones, plus one doing memory
scrubbing.
But also, I don't see info about the handling CPU when doing `xl
debug-key d`. At one time, with `xl debug-key d` I got this:

(XEN) *** Dumping CPU6 guest state (d0v7): ***
(XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:  M     ]----
(XEN) CPU:    6
(XEN) RIP:    e033:[<ffffffff81e1546a>]
(XEN) RFLAGS: 0000000000000286   EM: 0   CONTEXT: pv guest (d0v7)
(XEN) rax: 0000000000000023   rbx: 0000000000000005   rcx: ffffffff81e1546a
(XEN) rdx: 0000000000000000   rsi: 0000000000000000   rdi: 000079147611e010
(XEN) rbp: ffff88810db53200   rsp: ffffc90041c6bde0   r8:  0000000000000000
(XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 0000000000000286
(XEN) r12: 0000000000305000   r13: 00007ffc61097f40   r14: ffff88810db53200
(XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 0000000000b526e0
(XEN) cr3: 00000004ae2a7000   cr2: 00000000006d3118
(XEN) fsb: 0000791475b8a380   gsb: ffff8881897c0000   gss: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e02b   cs: e033
(XEN) Guest stack trace from rsp=ffffc90041c6bde0:
(XEN)    0000000000000001 0000000000000000 ffffffffc02905a6 0000000000000023
(XEN)    000079147611e010 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000000000000000 8064129fc747f100 ffffffffc0291568 0000000000305000
(XEN)    8064129fc747f100 0000000000000005 ffffffff813f7d4d ffffc90041c6bf58
(XEN)    ffffc90041c6bf48 0000000000000000 0000000000000000 0000000000000000
(XEN)    ffffffff81e16158 00000000006d3118 ffffc90041c6bf58 0000000000000040
(XEN)    ffffffff8132f6bb 0000000000000006 ffffc90041c6bf58 00000000006d3118
(XEN)    0000000000000255 ffff888102cf8880 ffff888102cf88f0 ffffffff8108746f
(XEN)    0000000000000000 0000000000000002 ffffc90041c6bf58 ffffc90041c6bf58
(XEN)    00000000006d3118 0000000000000000 0000000000000006 0000000000000000
(XEN)    0000000000000000 ffffffff81e1a975 0000000000000000 0000000000000000
(XEN)    0000000000000000 0000000000000000 ffffffff8200009b 000000000043d9b0
(XEN)    000000000043d990 00007ffc61097f90 00007ffc61097fc0 00007ffc61099d16
(XEN)    00000000006cab40 0000000000000246 0000000000000001 0000000000000000
(XEN)    0000000000000006 ffffffffffffffda 0000791475f1ed6f 00007ffc61097f40
(XEN)    0000000000305000 0000000000000005 0000000000000010 0000791475f1ed6f
(XEN)    0000000000000033 0000000000000246 00007ffc61097ed0 000000000000002b
(XEN)     Fault while accessing guest memory.
(XEN) 
(XEN) *** Dumping CPU0 host state: ***
(XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:  M     ]----
(XEN) CPU:    0
(XEN) RIP:    e008:[<ffff82d04022c07e>] _spin_unlock_irqrestore+0x21/0x27
(XEN) RFLAGS: 0000000000000206   CONTEXT: hypervisor
(XEN) rax: ffff82d0405c1038   rbx: 0000000000000200   rcx: 0000000000000008
(XEN) rdx: ffff830856d07fff   rsi: ffff8308529d5b28   rdi: ffff8308529d5b20
(XEN) rbp: ffff830856d07dc8   rsp: ffff830856d07dc0   r8:  0000000000000001
(XEN) r9:  ffff8308529d5b20   r10: ffff82d0405c13a0   r11: 000000d091e62221
(XEN) r12: ffff82d040476898   r13: 0000000000000296   r14: ffff82d040476918
(XEN) r15: ffff82cffff04700   cr0: 0000000080050033   cr4: 0000000000b526e0
(XEN) cr3: 000000082e7ff000   cr2: ffff888109538618
(XEN) fsb: 0000000000000000   gsb: ffff888189600000   gss: 0000000000000000
(XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
(XEN) Xen code around <ffff82d04022c07e> (_spin_unlock_irqrestore+0x21/0x27):
(XEN)  fd ff ff 48 09 1c 24 9d <48> 8b 5d f8 c9 c3 0f b7 47 04 66 25 ff 0f 66 3d
(XEN) Xen stack trace from rsp=ffff830856d07dc0:
(XEN)    ffff82d0405d0c80 ffff830856d07e08 ffff82d040257c3f 0000000040476898
(XEN)    ffff82d0405c1280 ffff82d040257bca ffff82d040476898 000000d0911fcbc4
(XEN)    0000000000000000 ffff830856d07e30 ffff82d04022d55c ffff82d0405c1280
(XEN)    ffff8308529d5f00 ffff82d0405d0d68 ffff830856d07e70 ffff82d04022de59
(XEN)    ffff830856d07ef8 ffff82d0405c7f00 ffffffffffffffff ffff82d0405c7f00
(XEN)    ffff830856d07fff 0000000000000000 ffff830856d07ea8 ffff82d04022b53e
(XEN)    0000000000000000 0000000000007fff ffff82d0405c7f00 ffff82d0405c11d0
(XEN)    ffff82d0405db2a0 ffff830856d07eb8 ffff82d04022b5d1 ffff830856d07ef0
(XEN)    ffff82d0402fcd15 ffff82d0402fcc88 ffff8308528cb000 ffff830856d07ef8
(XEN)    ffff830856ce2000 0000000000000000 ffff830856d07e18 0000000000000000
(XEN)    0000000000000000 0000000000000000 0000000000000000 ffffffff82c1aa40
(XEN)    0000000000000000 0000000000000246 0000000000007ff0 0000000000000000
(XEN)    000000000fd109eb 0000000000000000 ffffffff81e153aa 4000000000000000
(XEN)    deadbeefdeadf00d deadbeefdeadf00d 0000010000000000 ffffffff81e153aa
(XEN)    000000000000e033 0000000000000246 ffffffff82c03dd0 000000000000e02b
(XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
(XEN)    0000e01000000000 ffff830856ce1000 0000000000000000 0000000000b526e0
(XEN)    0000000000000000 0000000000000000 0006030300000000 0000000000000000
(XEN) Xen call trace:
(XEN)    [<ffff82d04022c07e>] R _spin_unlock_irqrestore+0x21/0x27
(XEN)    [<ffff82d040257c3f>] F xhci-dbc.c#dbc_uart_poll+0x75/0x17c
(XEN)    [<ffff82d04022d55c>] F timer.c#execute_timer+0x45/0x5c
(XEN)    [<ffff82d04022de59>] F timer.c#timer_softirq_action+0x71/0x275
(XEN)    [<ffff82d04022b53e>] F softirq.c#__do_softirq+0x94/0xbe
(XEN)    [<ffff82d04022b5d1>] F do_softirq+0x13/0x15
(XEN)    [<ffff82d0402fcd15>] F domain.c#idle_loop+0x8d/0xe6

(other CPUs in mwait-idle)

> > Or maybe add
> > some extra debug prints and include their output? But note, printk from
> > inside console code generally leads to deadlocks. What I did for some
> > debugging was to log into some separate buffer and dump it later.
> 
> Right, this would be more involved.
> 
> Jan

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

On 15.02.2024 03:19, Marek Marczykowski wrote:
> On Tue, Feb 13, 2024 at 04:44:04PM +0100, Jan Beulich wrote:
>> On 13.02.2024 16:11, Marek Marczykowski wrote:
>>> On Tue, Feb 13, 2024 at 04:00:32PM +0100, Jan Beulich wrote:
>>>> On 13.02.2024 15:53, Marek Marczykowski wrote:
>>>>> Generally provides sensible output, for example during boot (it is using
>>>>> idle vCPU then, right?).
>>>>
>>>> Before Dom0 is started: Yes. With the exception of the phase where PV
>>>> Dom0's page tables are constructed, albeit in that time window
>>>> guest_cpu_user_regs() shouldn't yield sensible data either. I can only
>>>> say I'm surprised; since I have no way to properly test with an XHCI
>>>> debug port, I'd have to see about faking something to convince myself
>>>> (unless you were to supply example output).
>>>
>>> Would you like me to test this series with xhci console?
>>
>> The behavior shouldn't really be connected to this series. But yes, 'd'
>> debug key output (just the part for the CPU the key handling was
>> actually invoked from) with the xhci debug console would be of
>> interest, for the case where that CPU at that time runs an idle vCPU.
> 
> I managed to press 'd' before dom0 started. Full output at
> https://gist.github.com/marmarek/a495cd666f4aafed3a5cfcb8393f515b (it's
> Alder Lake, and smt=off, so CPU numbering is weird).
> Interestingly, I do _not_ see output for CPU0, where I'd expect the
> key handler to run... I see all the idle ones, plus one doing memory
> scrubbing.

Which is precisely the problem, just in not exactly the manifestation
I expected. In dump_execstate() we dump host state only if the
incoming regs don't indicate guest state. Yet for the idle vCPU they
(wrongly) do here - see how guest_mode() calculates the delta to what
guest_cpu_user_regs() returns, i.e. 0 when what guest_cpu_user_regs()
returned is passed in.

Guest state dumping is suppressed for idle vCPU-s. Hence no output
at all for the CPU where the key processing was actually invoked
from.

> But also, I don't see info about the handling CPU when doing `xl
> debug-key d`.

I'm afraid I'm confused, ...

> At one time, with `xl debug-key d` I got this:
> 
> (XEN) *** Dumping CPU6 guest state (d0v7): ***
> (XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:  M     ]----
> (XEN) CPU:    6
> (XEN) RIP:    e033:[<ffffffff81e1546a>]
> (XEN) RFLAGS: 0000000000000286   EM: 0   CONTEXT: pv guest (d0v7)
> (XEN) rax: 0000000000000023   rbx: 0000000000000005   rcx: ffffffff81e1546a
> (XEN) rdx: 0000000000000000   rsi: 0000000000000000   rdi: 000079147611e010
> (XEN) rbp: ffff88810db53200   rsp: ffffc90041c6bde0   r8:  0000000000000000
> (XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 0000000000000286
> (XEN) r12: 0000000000305000   r13: 00007ffc61097f40   r14: ffff88810db53200
> (XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 0000000000b526e0
> (XEN) cr3: 00000004ae2a7000   cr2: 00000000006d3118
> (XEN) fsb: 0000791475b8a380   gsb: ffff8881897c0000   gss: 0000000000000000
> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e02b   cs: e033
> (XEN) Guest stack trace from rsp=ffffc90041c6bde0:
> (XEN)    0000000000000001 0000000000000000 ffffffffc02905a6 0000000000000023
> (XEN)    000079147611e010 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000000000000000 8064129fc747f100 ffffffffc0291568 0000000000305000
> (XEN)    8064129fc747f100 0000000000000005 ffffffff813f7d4d ffffc90041c6bf58
> (XEN)    ffffc90041c6bf48 0000000000000000 0000000000000000 0000000000000000
> (XEN)    ffffffff81e16158 00000000006d3118 ffffc90041c6bf58 0000000000000040
> (XEN)    ffffffff8132f6bb 0000000000000006 ffffc90041c6bf58 00000000006d3118
> (XEN)    0000000000000255 ffff888102cf8880 ffff888102cf88f0 ffffffff8108746f
> (XEN)    0000000000000000 0000000000000002 ffffc90041c6bf58 ffffc90041c6bf58
> (XEN)    00000000006d3118 0000000000000000 0000000000000006 0000000000000000
> (XEN)    0000000000000000 ffffffff81e1a975 0000000000000000 0000000000000000
> (XEN)    0000000000000000 0000000000000000 ffffffff8200009b 000000000043d9b0
> (XEN)    000000000043d990 00007ffc61097f90 00007ffc61097fc0 00007ffc61099d16
> (XEN)    00000000006cab40 0000000000000246 0000000000000001 0000000000000000
> (XEN)    0000000000000006 ffffffffffffffda 0000791475f1ed6f 00007ffc61097f40
> (XEN)    0000000000305000 0000000000000005 0000000000000010 0000791475f1ed6f
> (XEN)    0000000000000033 0000000000000246 00007ffc61097ed0 000000000000002b
> (XEN)     Fault while accessing guest memory.
> (XEN) 
> (XEN) *** Dumping CPU0 host state: ***
> (XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:  M     ]----
> (XEN) CPU:    0
> (XEN) RIP:    e008:[<ffff82d04022c07e>] _spin_unlock_irqrestore+0x21/0x27
> (XEN) RFLAGS: 0000000000000206   CONTEXT: hypervisor
> (XEN) rax: ffff82d0405c1038   rbx: 0000000000000200   rcx: 0000000000000008
> (XEN) rdx: ffff830856d07fff   rsi: ffff8308529d5b28   rdi: ffff8308529d5b20
> (XEN) rbp: ffff830856d07dc8   rsp: ffff830856d07dc0   r8:  0000000000000001
> (XEN) r9:  ffff8308529d5b20   r10: ffff82d0405c13a0   r11: 000000d091e62221
> (XEN) r12: ffff82d040476898   r13: 0000000000000296   r14: ffff82d040476918
> (XEN) r15: ffff82cffff04700   cr0: 0000000080050033   cr4: 0000000000b526e0
> (XEN) cr3: 000000082e7ff000   cr2: ffff888109538618
> (XEN) fsb: 0000000000000000   gsb: ffff888189600000   gss: 0000000000000000
> (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
> (XEN) Xen code around <ffff82d04022c07e> (_spin_unlock_irqrestore+0x21/0x27):
> (XEN)  fd ff ff 48 09 1c 24 9d <48> 8b 5d f8 c9 c3 0f b7 47 04 66 25 ff 0f 66 3d
> (XEN) Xen stack trace from rsp=ffff830856d07dc0:
> (XEN)    ffff82d0405d0c80 ffff830856d07e08 ffff82d040257c3f 0000000040476898
> (XEN)    ffff82d0405c1280 ffff82d040257bca ffff82d040476898 000000d0911fcbc4
> (XEN)    0000000000000000 ffff830856d07e30 ffff82d04022d55c ffff82d0405c1280
> (XEN)    ffff8308529d5f00 ffff82d0405d0d68 ffff830856d07e70 ffff82d04022de59
> (XEN)    ffff830856d07ef8 ffff82d0405c7f00 ffffffffffffffff ffff82d0405c7f00
> (XEN)    ffff830856d07fff 0000000000000000 ffff830856d07ea8 ffff82d04022b53e
> (XEN)    0000000000000000 0000000000007fff ffff82d0405c7f00 ffff82d0405c11d0
> (XEN)    ffff82d0405db2a0 ffff830856d07eb8 ffff82d04022b5d1 ffff830856d07ef0
> (XEN)    ffff82d0402fcd15 ffff82d0402fcc88 ffff8308528cb000 ffff830856d07ef8
> (XEN)    ffff830856ce2000 0000000000000000 ffff830856d07e18 0000000000000000
> (XEN)    0000000000000000 0000000000000000 0000000000000000 ffffffff82c1aa40
> (XEN)    0000000000000000 0000000000000246 0000000000007ff0 0000000000000000
> (XEN)    000000000fd109eb 0000000000000000 ffffffff81e153aa 4000000000000000
> (XEN)    deadbeefdeadf00d deadbeefdeadf00d 0000010000000000 ffffffff81e153aa
> (XEN)    000000000000e033 0000000000000246 ffffffff82c03dd0 000000000000e02b
> (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> (XEN)    0000e01000000000 ffff830856ce1000 0000000000000000 0000000000b526e0
> (XEN)    0000000000000000 0000000000000000 0006030300000000 0000000000000000
> (XEN) Xen call trace:
> (XEN)    [<ffff82d04022c07e>] R _spin_unlock_irqrestore+0x21/0x27
> (XEN)    [<ffff82d040257c3f>] F xhci-dbc.c#dbc_uart_poll+0x75/0x17c
> (XEN)    [<ffff82d04022d55c>] F timer.c#execute_timer+0x45/0x5c
> (XEN)    [<ffff82d04022de59>] F timer.c#timer_softirq_action+0x71/0x275
> (XEN)    [<ffff82d04022b53e>] F softirq.c#__do_softirq+0x94/0xbe
> (XEN)    [<ffff82d04022b5d1>] F do_softirq+0x13/0x15
> (XEN)    [<ffff82d0402fcd15>] F domain.c#idle_loop+0x8d/0xe6

... this looks to be the issuing CPU. What I don't understand is why we
are in _spin_unlock_irqrestore() here, called out of dbc_uart_poll().

Btw - was any/all of this with or without the series here applied?

Jan

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Jan Beulich]

On 05.02.2024 14:27, Jan Beulich wrote:
> In preparation of dropping the register parameters from
> serial_[rt]x_interrupt() and in turn from IRQ handler functions,
> register state needs making available another way for the few key
> handlers which need it. Fake IRQ-like state.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> The use of guest_cpu_user_regs() in dbc_uart_poll() is inconsistent with
> other console poll functions we have, and it's unclear whether that's
> actually generally correct.

It occurs to me that due to this behavior, ...

> --- 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_tx_interrupt(port, guest_cpu_user_regs());
> +
> +    set_irq_regs(old_regs);
>      set_timer(&uart->timer, NOW() + MICROSECS(DBC_POLL_INTERVAL));
>  }
>  
> 

... this ought to be unnecessary, considering what dump_registers()
is changed to in patch 2.

Jan

Re: [PATCH v3 1/8] serial: fake IRQ-regs context in poll handlers

[Marek Marczykowski]

[-- Attachment #1: Type: text/plain, Size: 8383 bytes --]

On Thu, Feb 15, 2024 at 09:39:41AM +0100, Jan Beulich wrote:
> On 15.02.2024 03:19, Marek Marczykowski wrote:
> > On Tue, Feb 13, 2024 at 04:44:04PM +0100, Jan Beulich wrote:
> >> On 13.02.2024 16:11, Marek Marczykowski wrote:
> >>> On Tue, Feb 13, 2024 at 04:00:32PM +0100, Jan Beulich wrote:
> >>>> On 13.02.2024 15:53, Marek Marczykowski wrote:
> >>>>> Generally provides sensible output, for example during boot (it is using
> >>>>> idle vCPU then, right?).
> >>>>
> >>>> Before Dom0 is started: Yes. With the exception of the phase where PV
> >>>> Dom0's page tables are constructed, albeit in that time window
> >>>> guest_cpu_user_regs() shouldn't yield sensible data either. I can only
> >>>> say I'm surprised; since I have no way to properly test with an XHCI
> >>>> debug port, I'd have to see about faking something to convince myself
> >>>> (unless you were to supply example output).
> >>>
> >>> Would you like me to test this series with xhci console?
> >>
> >> The behavior shouldn't really be connected to this series. But yes, 'd'
> >> debug key output (just the part for the CPU the key handling was
> >> actually invoked from) with the xhci debug console would be of
> >> interest, for the case where that CPU at that time runs an idle vCPU.
> > 
> > I managed to press 'd' before dom0 started. Full output at
> > https://gist.github.com/marmarek/a495cd666f4aafed3a5cfcb8393f515b (it's
> > Alder Lake, and smt=off, so CPU numbering is weird).
> > Interestingly, I do _not_ see output for CPU0, where I'd expect the
> > key handler to run... I see all the idle ones, plus one doing memory
> > scrubbing.
> 
> Which is precisely the problem, just in not exactly the manifestation
> I expected. In dump_execstate() we dump host state only if the
> incoming regs don't indicate guest state. Yet for the idle vCPU they
> (wrongly) do here - see how guest_mode() calculates the delta to what
> guest_cpu_user_regs() returns, i.e. 0 when what guest_cpu_user_regs()
> returned is passed in.
> 
> Guest state dumping is suppressed for idle vCPU-s. Hence no output
> at all for the CPU where the key processing was actually invoked
> from.
> 
> > But also, I don't see info about the handling CPU when doing `xl
> > debug-key d`.
> 
> I'm afraid I'm confused, ...
> 
> > At one time, with `xl debug-key d` I got this:
> > 
> > (XEN) *** Dumping CPU6 guest state (d0v7): ***
> > (XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:  M     ]----
> > (XEN) CPU:    6
> > (XEN) RIP:    e033:[<ffffffff81e1546a>]
> > (XEN) RFLAGS: 0000000000000286   EM: 0   CONTEXT: pv guest (d0v7)
> > (XEN) rax: 0000000000000023   rbx: 0000000000000005   rcx: ffffffff81e1546a
> > (XEN) rdx: 0000000000000000   rsi: 0000000000000000   rdi: 000079147611e010
> > (XEN) rbp: ffff88810db53200   rsp: ffffc90041c6bde0   r8:  0000000000000000
> > (XEN) r9:  0000000000000000   r10: 0000000000000000   r11: 0000000000000286
> > (XEN) r12: 0000000000305000   r13: 00007ffc61097f40   r14: ffff88810db53200
> > (XEN) r15: 0000000000000000   cr0: 0000000080050033   cr4: 0000000000b526e0
> > (XEN) cr3: 00000004ae2a7000   cr2: 00000000006d3118
> > (XEN) fsb: 0000791475b8a380   gsb: ffff8881897c0000   gss: 0000000000000000
> > (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e02b   cs: e033
> > (XEN) Guest stack trace from rsp=ffffc90041c6bde0:
> > (XEN)    0000000000000001 0000000000000000 ffffffffc02905a6 0000000000000023
> > (XEN)    000079147611e010 0000000000000000 0000000000000000 0000000000000000
> > (XEN)    0000000000000000 8064129fc747f100 ffffffffc0291568 0000000000305000
> > (XEN)    8064129fc747f100 0000000000000005 ffffffff813f7d4d ffffc90041c6bf58
> > (XEN)    ffffc90041c6bf48 0000000000000000 0000000000000000 0000000000000000
> > (XEN)    ffffffff81e16158 00000000006d3118 ffffc90041c6bf58 0000000000000040
> > (XEN)    ffffffff8132f6bb 0000000000000006 ffffc90041c6bf58 00000000006d3118
> > (XEN)    0000000000000255 ffff888102cf8880 ffff888102cf88f0 ffffffff8108746f
> > (XEN)    0000000000000000 0000000000000002 ffffc90041c6bf58 ffffc90041c6bf58
> > (XEN)    00000000006d3118 0000000000000000 0000000000000006 0000000000000000
> > (XEN)    0000000000000000 ffffffff81e1a975 0000000000000000 0000000000000000
> > (XEN)    0000000000000000 0000000000000000 ffffffff8200009b 000000000043d9b0
> > (XEN)    000000000043d990 00007ffc61097f90 00007ffc61097fc0 00007ffc61099d16
> > (XEN)    00000000006cab40 0000000000000246 0000000000000001 0000000000000000
> > (XEN)    0000000000000006 ffffffffffffffda 0000791475f1ed6f 00007ffc61097f40
> > (XEN)    0000000000305000 0000000000000005 0000000000000010 0000791475f1ed6f
> > (XEN)    0000000000000033 0000000000000246 00007ffc61097ed0 000000000000002b
> > (XEN)     Fault while accessing guest memory.
> > (XEN) 
> > (XEN) *** Dumping CPU0 host state: ***
> > (XEN) ----[ Xen-4.18-unstable  x86_64  debug=y  Tainted:  M     ]----
> > (XEN) CPU:    0
> > (XEN) RIP:    e008:[<ffff82d04022c07e>] _spin_unlock_irqrestore+0x21/0x27
> > (XEN) RFLAGS: 0000000000000206   CONTEXT: hypervisor
> > (XEN) rax: ffff82d0405c1038   rbx: 0000000000000200   rcx: 0000000000000008
> > (XEN) rdx: ffff830856d07fff   rsi: ffff8308529d5b28   rdi: ffff8308529d5b20
> > (XEN) rbp: ffff830856d07dc8   rsp: ffff830856d07dc0   r8:  0000000000000001
> > (XEN) r9:  ffff8308529d5b20   r10: ffff82d0405c13a0   r11: 000000d091e62221
> > (XEN) r12: ffff82d040476898   r13: 0000000000000296   r14: ffff82d040476918
> > (XEN) r15: ffff82cffff04700   cr0: 0000000080050033   cr4: 0000000000b526e0
> > (XEN) cr3: 000000082e7ff000   cr2: ffff888109538618
> > (XEN) fsb: 0000000000000000   gsb: ffff888189600000   gss: 0000000000000000
> > (XEN) ds: 0000   es: 0000   fs: 0000   gs: 0000   ss: e010   cs: e008
> > (XEN) Xen code around <ffff82d04022c07e> (_spin_unlock_irqrestore+0x21/0x27):
> > (XEN)  fd ff ff 48 09 1c 24 9d <48> 8b 5d f8 c9 c3 0f b7 47 04 66 25 ff 0f 66 3d
> > (XEN) Xen stack trace from rsp=ffff830856d07dc0:
> > (XEN)    ffff82d0405d0c80 ffff830856d07e08 ffff82d040257c3f 0000000040476898
> > (XEN)    ffff82d0405c1280 ffff82d040257bca ffff82d040476898 000000d0911fcbc4
> > (XEN)    0000000000000000 ffff830856d07e30 ffff82d04022d55c ffff82d0405c1280
> > (XEN)    ffff8308529d5f00 ffff82d0405d0d68 ffff830856d07e70 ffff82d04022de59
> > (XEN)    ffff830856d07ef8 ffff82d0405c7f00 ffffffffffffffff ffff82d0405c7f00
> > (XEN)    ffff830856d07fff 0000000000000000 ffff830856d07ea8 ffff82d04022b53e
> > (XEN)    0000000000000000 0000000000007fff ffff82d0405c7f00 ffff82d0405c11d0
> > (XEN)    ffff82d0405db2a0 ffff830856d07eb8 ffff82d04022b5d1 ffff830856d07ef0
> > (XEN)    ffff82d0402fcd15 ffff82d0402fcc88 ffff8308528cb000 ffff830856d07ef8
> > (XEN)    ffff830856ce2000 0000000000000000 ffff830856d07e18 0000000000000000
> > (XEN)    0000000000000000 0000000000000000 0000000000000000 ffffffff82c1aa40
> > (XEN)    0000000000000000 0000000000000246 0000000000007ff0 0000000000000000
> > (XEN)    000000000fd109eb 0000000000000000 ffffffff81e153aa 4000000000000000
> > (XEN)    deadbeefdeadf00d deadbeefdeadf00d 0000010000000000 ffffffff81e153aa
> > (XEN)    000000000000e033 0000000000000246 ffffffff82c03dd0 000000000000e02b
> > (XEN)    0000000000000000 0000000000000000 0000000000000000 0000000000000000
> > (XEN)    0000e01000000000 ffff830856ce1000 0000000000000000 0000000000b526e0
> > (XEN)    0000000000000000 0000000000000000 0006030300000000 0000000000000000
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82d04022c07e>] R _spin_unlock_irqrestore+0x21/0x27
> > (XEN)    [<ffff82d040257c3f>] F xhci-dbc.c#dbc_uart_poll+0x75/0x17c
> > (XEN)    [<ffff82d04022d55c>] F timer.c#execute_timer+0x45/0x5c
> > (XEN)    [<ffff82d04022de59>] F timer.c#timer_softirq_action+0x71/0x275
> > (XEN)    [<ffff82d04022b53e>] F softirq.c#__do_softirq+0x94/0xbe
> > (XEN)    [<ffff82d04022b5d1>] F do_softirq+0x13/0x15
> > (XEN)    [<ffff82d0402fcd15>] F domain.c#idle_loop+0x8d/0xe6
> 
> ... this looks to be the issuing CPU. What I don't understand is why we
> are in _spin_unlock_irqrestore() here, called out of dbc_uart_poll().
> 
> Btw - was any/all of this with or without the series here applied?

This was without this series applied.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 2/8] keyhandler: drop regs parameter from handle_keyregs()

[Julien Grall]

Hi Jan,

On 12/02/2024 09:13, Jan Beulich wrote:
> On 08.02.2024 23:09, Julien Grall wrote:
>> On 05/02/2024 13:28, Jan Beulich wrote:
>>> In preparation for further removal of regs parameters, drop it here. In
>>> the two places where it's actually needed, retrieve IRQ context if
>>> available, or else guest context.
>>>
>>> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> ---
>>> As an alternative to the new boolean parameter, I wonder if we couldn't
>>> special-case the idle vCPU case: It's only there where we would not have
>>> proper context retrievable via guest_cpu_user_regs().
>>
>> I am trying to understand the implication. Looking at the code, it seems
>> in the case where we pass NULL, we would expect to call
>> run_in_exception_handler().
> 
> Right, when NULL was passed so far, and when true is passed now, that's
> to indicate to invoke run_in_exception_handler().
> 
>> If I am not mistaken, at least for Arm, regs would not be the same as
>> guest_cpu_user_regs(). So I think your current approach is more correct.
>>
>> Did I miss anything?
> 
> Whether regs are the same isn't overly relevant here. The thing that's
> relevant is whether what would be logged actually makes sense. And
> invoking guest_cpu_user_regs() in idle vCPU context makes no sense.
> Whereas in other contexts its result is good enough to show the present
> state of the CPU; there's no real need in such a case to go through
> run_in_exception_handler().
> 
> The present approach therefore isn't necessarily "more correct", but it
> is closer to prior behavior.
> 
> The corner case that makes me prefer the presently chosen approach is
> when a CPU is in the middle of scheduling, especially considering x86's
> lazy switching (when current != this_cpu(curr_vcpu). The main reason to
> mention the alternative is because iirc Andrew had suggested to move
> more in that direction.

Thanks for the clarification. I don't have any strong preference either 
way. So I would be happy to go with your solution.

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall