Re: [PATCH v5 2/4] xen: change <asm/bug.h> to <xen/bug.h>

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.03.2023 11:38, Oleksii Kurochko wrote:
> The idea of the patch is to change all <asm/bug.h> to <xen/bug.h> and
> keep Xen compilable with adding only minimal amount of changes:
> 1. It was added "#include <xen/types.h>" to ARM's "<asm/bug.h>" as it
>   uses uint_{16,32}t in 'struct bug_frame'.
> 2. It was added '#define BUG_FRAME_STRUCT' which means that ARM hasn't
>   been switched to generic implementation yet.
> 3. It was added '#define BUG_FRAME_STRUCT' which means that x86 hasn't
>   been switched to generic implementation yet.
> 4. BUGFRAME_* and _start_bug_frame[], _stop_bug_frame_*[] were removed
>   for ARM & x86 to deal with compilation errors such as:
>       redundant redeclaration of ...
> 
> In the following two patches x86 and ARM archictectures will be
> switched fully:
> * xen/arm: switch ARM to use generic implementation of bug.h
> * xen/x86: switch x86 to use generic implemetation of bug.h
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V5:
>  - Nothing changed
> ---
> Changes in V4:
>       - defines BUG_DISP_WIDTH, BUG_LINE_LO_WIDTH, BUG_LINE_HI_WIDTH were 
> moved into
>         "ifndef BUG_FRAME_STRUCT" in <xen/bug.h> as they are specific for 
> 'struct bug_frame' and so should
>         co-exist together. So the defines were back to <asm/bug.h> until 
> BUG_FRAME_STRUCT will be defined in
>         <asm/bug.h>.
>       - Update the comment message.
> ---
> Changes in V3:
>  * Update patch 2 not to break compilation: move some parts from patches 3 
> and 4
>    to patch 2:
>    * move some generic parts from <asm/bug.h> to <xen/bug.h>
>    * add define BUG_FRAME_STRUCT in ARM's <asm/bug.h>
> ---
> Changes in V2:
>  * Put [PATCH v1 4/4] xen: change <asm/bug.h> to <xen/bug.h> as second patch,
>    update the patch to change all <asm/bug.h> to <xen/bug.h> among the whole 
> project
>    to not break build.
>  * Update the commit message.
> ---
>  xen/arch/arm/include/asm/bug.h       | 17 ++++-------------
>  xen/arch/arm/include/asm/div64.h     |  2 +-
>  xen/arch/arm/vgic/vgic-v2.c          |  2 +-
>  xen/arch/arm/vgic/vgic.c             |  2 +-
>  xen/arch/x86/acpi/cpufreq/cpufreq.c  |  2 +-
>  xen/arch/x86/include/asm/asm_defns.h |  2 +-
>  xen/arch/x86/include/asm/bug.h       | 15 ++-------------
>  xen/drivers/cpufreq/cpufreq.c        |  2 +-
>  xen/include/xen/lib.h                |  2 +-
>  9 files changed, 13 insertions(+), 33 deletions(-)
> 
> diff --git a/xen/arch/arm/include/asm/bug.h b/xen/arch/arm/include/asm/bug.h
> index f4088d0913..9315662c6e 100644
> --- a/xen/arch/arm/include/asm/bug.h
> +++ b/xen/arch/arm/include/asm/bug.h
> @@ -1,6 +1,8 @@
>  #ifndef __ARM_BUG_H__
>  #define __ARM_BUG_H__
>  
> +#include <xen/types.h>
> +
>  #if defined(CONFIG_ARM_32)
>  # include <asm/arm32/bug.h>
>  #elif defined(CONFIG_ARM_64)
> @@ -13,6 +15,8 @@
>  #define BUG_LINE_LO_WIDTH (31 - BUG_DISP_WIDTH)
>  #define BUG_LINE_HI_WIDTH (31 - BUG_DISP_WIDTH)
>  
> +#define BUG_FRAME_STRUCT
> +
>  struct bug_frame {
>      signed int loc_disp;    /* Relative address to the bug address */
>      signed int file_disp;   /* Relative address to the filename */
> @@ -26,13 +30,6 @@ struct bug_frame {
>  #define bug_line(b) ((b)->line)
>  #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
>  
> -#define BUGFRAME_run_fn 0
> -#define BUGFRAME_warn   1
> -#define BUGFRAME_bug    2
> -#define BUGFRAME_assert 3
> -
> -#define BUGFRAME_NR     4
> -
>  /* Many versions of GCC doesn't support the asm %c parameter which would
>   * be preferable to this unpleasantness. We use mergeable string
>   * sections to avoid multiple copies of the string appearing in the
> @@ -89,12 +86,6 @@ struct bug_frame {
>      unreachable();                                              \
>  } while (0)
>  
> -extern const struct bug_frame __start_bug_frames[],
> -                              __stop_bug_frames_0[],
> -                              __stop_bug_frames_1[],
> -                              __stop_bug_frames_2[],
> -                              __stop_bug_frames_3[];
> -
>  #endif /* __ARM_BUG_H__ */
>  /*
>   * Local variables:
> diff --git a/xen/arch/arm/include/asm/div64.h 
> b/xen/arch/arm/include/asm/div64.h
> index 1cd58bc51a..fc667a80f9 100644
> --- a/xen/arch/arm/include/asm/div64.h
> +++ b/xen/arch/arm/include/asm/div64.h
> @@ -74,7 +74,7 @@
>  
>  #elif __GNUC__ >= 4
>  
> -#include <asm/bug.h>
> +#include <xen/bug.h>
>  
>  /*
>   * If the divisor happens to be constant, we determine the appropriate
> diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
> index 1a99d3a8b4..c90e88fddb 100644
> --- a/xen/arch/arm/vgic/vgic-v2.c
> +++ b/xen/arch/arm/vgic/vgic-v2.c
> @@ -16,8 +16,8 @@
>   */
>  
>  #include <asm/new_vgic.h>
> -#include <asm/bug.h>
>  #include <asm/gic.h>
> +#include <xen/bug.h>
>  #include <xen/sched.h>
>  #include <xen/sizes.h>
>  
> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
> index f0f2ea5021..b9463a5f27 100644
> --- a/xen/arch/arm/vgic/vgic.c
> +++ b/xen/arch/arm/vgic/vgic.c
> @@ -15,9 +15,9 @@
>   * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>   */
>  
> +#include <xen/bug.h>
>  #include <xen/list_sort.h>
>  #include <xen/sched.h>
> -#include <asm/bug.h>
>  #include <asm/event.h>
>  #include <asm/new_vgic.h>
>  
> diff --git a/xen/arch/x86/acpi/cpufreq/cpufreq.c 
> b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> index c27cbb2304..18ff2a443b 100644
> --- a/xen/arch/x86/acpi/cpufreq/cpufreq.c
> +++ b/xen/arch/x86/acpi/cpufreq/cpufreq.c
> @@ -27,6 +27,7 @@
>   * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   */
>  
> +#include <xen/bug.h>
>  #include <xen/types.h>
>  #include <xen/errno.h>
>  #include <xen/delay.h>
> @@ -35,7 +36,6 @@
>  #include <xen/sched.h>
>  #include <xen/timer.h>
>  #include <xen/xmalloc.h>
> -#include <asm/bug.h>
>  #include <asm/msr.h>
>  #include <asm/io.h>
>  #include <asm/processor.h>
> diff --git a/xen/arch/x86/include/asm/asm_defns.h 
> b/xen/arch/x86/include/asm/asm_defns.h
> index d9431180cf..a8526cf36c 100644
> --- a/xen/arch/x86/include/asm/asm_defns.h
> +++ b/xen/arch/x86/include/asm/asm_defns.h
> @@ -6,7 +6,7 @@
>  /* NB. Auto-generated from arch/.../asm-offsets.c */
>  #include <asm/asm-offsets.h>
>  #endif
> -#include <asm/bug.h>
> +#include <xen/bug.h>
>  #include <asm/x86-defns.h>
>  #include <xen/stringify.h>
>  #include <asm/cpufeature.h>

While there's an unhelpful mix of asm/ and xen/ here already, may I ask
that you try to avoid making things yet worse: Unless there's a reason
not to, please move the added line past asm/x86-defns.h, adjacent to
xen/stringify.h. Then non-Arm parts
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
(assuming of course no further large rework is necessary because of the
comments on patch 1).

Jan