Re: [PATCH v3 3/4] xen/arm: switch ARM to use generic implementation of bug.h

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

On Sat, 2023-02-25 at 17:05 +0000, Julien Grall wrote:
> 
> 
> On 25/02/2023 16:49, Julien Grall wrote:
> > Hi Oleksii,
> > 
> > On 24/02/2023 11:31, Oleksii Kurochko wrote:
> > > The following changes were made:
> > > * make GENERIC_BUG_FRAME mandatory for ARM
> > 
> > I have asked in patch #1 but will ask it again because I think this
> > should be recorded in the commit message. Can you outline why it is
> > not 
> > possible to completely switch to the generic version?
> 
> I have just tried to remove it on arm64 and it seems to work. This
> was 
> only tested with GCC 10 though. But given the generic version is not
> not 
> using the %c modifier, then I wouldn't expect any issue.
> 
> Cheers,
> 

I tried to switch ARM to generic implementation.

Here is the patch: [1]

diff --git a/xen/arch/arm/include/asm/bug.h
b/xen/arch/arm/include/asm/bug.h
index e6cc37e1d6..ffb0f569fc 100644
--- a/xen/arch/arm/include/asm/bug.h
+++ b/xen/arch/arm/include/asm/bug.h
@@ -1,8 +1,6 @@
 #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)
@@ -11,63 +9,7 @@
 # error "unknown ARM variant"
 #endif
 
-#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 */
-    signed int msg_disp;    /* Relative address to the predicate (for
ASSERT) */
-    uint16_t line;          /* Line number */
-    uint32_t pad0:16;       /* Padding for 8-bytes align */
-};
-
-#define bug_ptr(b) ((const void *)(b) + (b)->file_disp)
-#define bug_line(b) ((b)->line)
-#define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
-
-/* 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
- * Xen image. BUGFRAME_run_fn needs to be handled separately.
- */
-#define BUG_FRAME(type, line, file, has_msg, msg) do {               
\
-    BUILD_BUG_ON((line) >> 16);                                      
\
-    BUILD_BUG_ON((type) >= BUGFRAME_NR);                             
\
-    asm ("1:"BUG_INSTR"\n"                                           
\
-         ".pushsection .rodata.str, \"aMS\", %progbits, 1\n"         
\
-         "2:\t.asciz " __stringify(file) "\n"                        
\
-         "3:\n"                                                      
\
-         ".if " #has_msg "\n"                                        
\
-         "\t.asciz " #msg "\n"                                       
\
-         ".endif\n"                                                  
\
-         ".popsection\n"                                             
\
-         ".pushsection .bug_frames." __stringify(type) ", \"a\",
%progbits\n"\
-         "4:\n"                                                      
\
-         ".p2align 2\n"                                              
\
-         ".long (1b - 4b)\n"                                         
\
-         ".long (2b - 4b)\n"                                         
\
-         ".long (3b - 4b)\n"                                         
\
-         ".hword " __stringify(line) ", 0\n"                         
\
-         ".popsection");                                             
\
-} while (0)
-
-/*
- * GCC will not allow to use "i"  when PIE is enabled (Xen doesn't set
the
- * flag but instead rely on the default value from the compiler). So
the
- * easiest way to implement run_in_exception_handler() is to pass the
to
- * be called function in a fixed register.
- */
-#define  run_in_exception_handler(fn) do {                           
\
-    asm ("mov " __stringify(BUG_FN_REG) ", %0\n"                     
\
-         "1:"BUG_INSTR"\n"                                           
\
-         ".pushsection .bug_frames." __stringify(BUGFRAME_run_fn) ","
\
-         "             \"a\", %%progbits\n"                          
\
-         "2:\n"                                                      
\
-         ".p2align 2\n"                                              
\
-         ".long (1b - 2b)\n"                                         
\
-         ".long 0, 0, 0\n"                                           
\
-         ".popsection" :: "r" (fn) : __stringify(BUG_FN_REG) );      
\
-} while (0)
+#define BUG_ASM_CONST   "c"
 
 #endif /* __ARM_BUG_H__ */
...
(it will be merged with patch 3 if it is OK )

And looks like we can switch ARM to generic implementation as all tests
passed:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396

The only issue is with yocto-arm:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/791549396/failures
But I am not sure that it is because of my patch

Is this enough from a verification point of view?

[1]
https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6
https://gitlab.com/xen-project/people/olkur/xen/-/commit/5ff7a06e1d354e1e42bde1c203f3cf05a3653ad6