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

Commit Message

Jan Beulich Feb. 5, 2024, 1:32 p.m. UTC

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 <[e-mail]>
---
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]

Comments

Andrew Cooper Feb. 5, 2024, 1:51 p.m. UTC | #1

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 <[e-mail]>

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 <[e-mail]>

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

~Andrew

Jan Beulich Feb. 5, 2024, 2:02 p.m. UTC | #2

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 <[e-mail]>
> 
> 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 <[e-mail]>

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

Julien Grall Feb. 8, 2024, 9:36 p.m. UTC | #3

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 <[e-mail]>

For the Arm parts:

Acked-by: Julien Grall <[e-mail]>

Cheers,

Patch

--- 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);