These are used from common code, so their signatures should be
consistent across architectures. This is achieved / guaranteed easiest
when their declarations are in a common header.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
There's no really good header to put the decls, imo; I wanted to avoid
the already overcrowded sched.h. show_execution_state_nonconst(), being
there solely for dump_execution_state(), could of course live in the
upcoming xen/bug.h ...

Is there a reason that Arm (still) expands dump_execution_state() to
WARN()? Without that moving x86's show_execution_state_nonconst()
definition to common code would also make sense (it could be done
anyway, but then at the expense of introducing dead code to Arm),
perhaps (see also above) into the upcoming common/bug.c.

--- a/xen/arch/arm/include/asm/domain.h
+++ b/xen/arch/arm/include/asm/domain.h
@@ -232,7 +232,6 @@ struct arch_vcpu
 
 }  __cacheline_aligned;
 
-void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 void vcpu_switch_to_aarch64_mode(struct vcpu *);
 
--- a/xen/arch/arm/include/asm/processor.h
+++ b/xen/arch/arm/include/asm/processor.h
@@ -557,7 +557,6 @@ extern register_t __cpu_logical_map[];
 #ifndef __ASSEMBLY__
 void panic_PAR(uint64_t par);
 
-void show_execution_state(const struct cpu_user_regs *regs);
 /* Debugging functions are declared with external linkage to aid development. */
 void show_registers(const struct cpu_user_regs *regs);
 void show_stack(const struct cpu_user_regs *regs);
--- a/xen/arch/x86/include/asm/domain.h
+++ b/xen/arch/x86/include/asm/domain.h
@@ -681,7 +681,6 @@ void domain_cpu_policy_changed(struct do
 bool update_secondary_system_time(struct vcpu *,
                                   struct vcpu_time_info *);
 
-void vcpu_show_execution_state(struct vcpu *);
 void vcpu_show_registers(const struct vcpu *);
 
 static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
--- a/xen/arch/x86/include/asm/processor.h
+++ b/xen/arch/x86/include/asm/processor.h
@@ -464,8 +464,6 @@ 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);
-void show_execution_state(const struct cpu_user_regs *regs);
-void cf_check show_execution_state_nonconst(struct cpu_user_regs *regs);
 #define dump_execution_state() \
     run_in_exception_handler(show_execution_state_nonconst)
 void show_page_walk(unsigned long addr);
--- a/xen/include/xen/kernel.h
+++ b/xen/include/xen/kernel.h
@@ -107,5 +107,12 @@ bool_t is_active_kernel_text(unsigned lo
 extern const char xen_config_data[];
 extern const unsigned int xen_config_data_size;
 
+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 vcpu_show_execution_state(struct vcpu *);
+
 #endif /* _LINUX_KERNEL_H */

Hi Jan,

On 16/03/2023 11:55, Jan Beulich wrote:
> These are used from common code, so their signatures should be
> consistent across architectures. This is achieved / guaranteed easiest
> when their declarations are in a common header.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> There's no really good header to put the decls, imo; I wanted to avoid
> the already overcrowded sched.h. show_execution_state_nonconst(), being
> there solely for dump_execution_state(), could of course live in the
> upcoming xen/bug.h ...
> 
> Is there a reason that Arm (still) expands dump_execution_state() to
> WARN()? Without that moving x86's show_execution_state_nonconst()
> definition to common code would also make sense (it could be done
> anyway, but then at the expense of introducing dead code to Arm),
> perhaps (see also above) into the upcoming common/bug.c.

run_in_exception_handler() was only properly implemented a couple of 
years ago on Arm and we didn't switch dump_execution_state() to use it.

That said, the current implementation of run_in_exception_handler() 
would not be correct for dump_execution_state() because it clobbers 
x0/r0 on Arm.

This should be addressed with Oleksandr work to consolidate the bug 
infrastructure. So it would be fine to move the helper to common code now.

Alternatively this can be left as a clean-up because 
dump_executation_state() could now be common.

Anyway:

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

Cheers,

-- 
Julien Grall

On 21.03.2023 17:44, Julien Grall wrote:
> On 16/03/2023 11:55, Jan Beulich wrote:
>> These are used from common code, so their signatures should be
>> consistent across architectures. This is achieved / guaranteed easiest
>> when their declarations are in a common header.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> There's no really good header to put the decls, imo; I wanted to avoid
>> the already overcrowded sched.h. show_execution_state_nonconst(), being
>> there solely for dump_execution_state(), could of course live in the
>> upcoming xen/bug.h ...
>>
>> Is there a reason that Arm (still) expands dump_execution_state() to
>> WARN()? Without that moving x86's show_execution_state_nonconst()
>> definition to common code would also make sense (it could be done
>> anyway, but then at the expense of introducing dead code to Arm),
>> perhaps (see also above) into the upcoming common/bug.c.
> 
> run_in_exception_handler() was only properly implemented a couple of 
> years ago on Arm and we didn't switch dump_execution_state() to use it.
> 
> That said, the current implementation of run_in_exception_handler() 
> would not be correct for dump_execution_state() because it clobbers 
> x0/r0 on Arm.
> 
> This should be addressed with Oleksandr work to consolidate the bug 
> infrastructure. So it would be fine to move the helper to common code now.

Well, Oleksii's work hasn't landed yet. I'll try to remember to follow
up once it has.

> Alternatively this can be left as a clean-up because 
> dump_executation_state() could now be common.
> 
> Anyway:
> 
> Acked-by: Julien Grall <jgrall@amazon.com>

Thanks.

Jan

On 16/03/2023 11:55 am, Jan Beulich wrote:
> These are used from common code, so their signatures should be
> consistent across architectures. This is achieved / guaranteed easiest
> when their declarations are in a common header.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

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

> ---
> There's no really good header to put the decls, imo; I wanted to avoid
> the already overcrowded sched.h.

Yeah - lets please not make sched any worse than it already is.

I can't suggest a better location than kernel.h, but I certainly would
like something else if one were to be found.

>  show_execution_state_nonconst(), being
> there solely for dump_execution_state(), could of course live in the
> upcoming xen/bug.h ...
>
> Is there a reason that Arm (still) expands dump_execution_state() to
> WARN()? Without that moving x86's show_execution_state_nonconst()
> definition to common code would also make sense (it could be done
> anyway, but then at the expense of introducing dead code to Arm),
> perhaps (see also above) into the upcoming common/bug.c.

That sounds like something that should be fixed up...