Re: [PATCH v4 1/3] xen/arm: add support for run_in_exception_handler()

[Jürgen Groß <jgross@xxxxxxxx>]

On 14.12.20 11:17, Julien Grall wrote:
> Hi Juergen,
>
> On 14/12/2020 07:56, Juergen Gross wrote:
> > Add support to run a function in an exception handler for Arm. Do it
> > the same way as on x86 via a bug_frame.
> >
> > Unfortunately inline assembly on Arm seems to be less capable than on
> > x86, leading to functions called via run_in_exception_handler() having
> > to be globally visible.
> Jan already commented on this, so I am not going to comment again.
Maybe I can ask some Arm specific question related to this:

In my experiments the only working solution was using the "i" constraint
for the function pointer. Do you know whether this is supported for all
gcc versions we care about?

Or is there another way to achieve the desired functionality? I'm using
now the following macros:

#define BUG_FRAME_run_fn(fn) do {                                      \
    asm ("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 - 2b)\n"                                           \
         ".long 0\n"                                                   \
         ".hword 0, 0\n"                                               \
         ".popsection" :: "i" (fn));                                   \
} while (0)

#define run_in_exception_handler(fn) BUG_FRAME_run_fn(fn)

> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V4:
> > - new patch
> >
> > I have verified the created bugframe is correct by inspecting the
> > created binary.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   xen/arch/arm/traps.c       | 10 +++++++++-
> >   xen/drivers/char/ns16550.c |  3 ++-
> >   xen/include/asm-arm/bug.h  | 32 +++++++++++++++++++++-----------
> >   3 files changed, 32 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > index 22bd1bd4c6..6e677affe2 100644
> > --- a/xen/arch/arm/traps.c
> > +++ b/xen/arch/arm/traps.c
> > @@ -1236,8 +1236,16 @@ int do_bug_frame(const struct cpu_user_regs 
> > *regs, vaddr_t pc)
> >       if ( !bug )
> >           return -ENOENT;
> > +    if ( id == BUGFRAME_run_fn )
> > +    {
> > +        void (*fn)(const struct cpu_user_regs *) = bug_ptr(bug);
> > +
> > +        fn(regs);
> > +        return 0;
> > +    }
> > +
> >       /* WARN, BUG or ASSERT: decode the filename pointer and line 
> > number. */
> > -    filename = bug_file(bug);
> > +    filename = bug_ptr(bug);
> >       if ( !is_kernel(filename) )
> >           return -EINVAL;
> >       fixup = strlen(filename);
> > diff --git a/xen/drivers/char/ns16550.c b/xen/drivers/char/ns16550.c
> > index 9235d854fe..dd6500acc8 100644
> > --- a/xen/drivers/char/ns16550.c
> > +++ b/xen/drivers/char/ns16550.c
> > @@ -192,7 +192,8 @@ static void ns16550_interrupt(
> >   /* Safe: ns16550_poll() runs as softirq so not reentrant on a given 
> > CPU. */
> >   static DEFINE_PER_CPU(struct serial_port *, poll_port);
> > -static void __ns16550_poll(struct cpu_user_regs *regs)
> > +/* run_in_exception_handler() on Arm requires globally visible 
> > symbol. */
> > +void __ns16550_poll(struct cpu_user_regs *regs)
> >   {
> >       struct serial_port *port = this_cpu(poll_port);
> >       struct ns16550 *uart = port->uart;
> > diff --git a/xen/include/asm-arm/bug.h b/xen/include/asm-arm/bug.h
> > index 36c803357c..a7da2c306f 100644
> > --- a/xen/include/asm-arm/bug.h
> > +++ b/xen/include/asm-arm/bug.h
> > @@ -15,34 +15,38 @@
> >   struct bug_frame {
> >       signed int loc_disp;    /* Relative address to the bug address */
> > -    signed int file_disp;   /* Relative address to the filename */
> > +    signed int ptr_disp;    /* Relative address to the filename or 
> > function */
> >       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_loc(b) ((const void *)(b) + (b)->loc_disp)
> > -#define bug_file(b) ((const void *)(b) + (b)->file_disp);
> > +#define bug_ptr(b) ((const void *)(b) + (b)->ptr_disp);
> >   #define bug_line(b) ((b)->line)
> >   #define bug_msg(b) ((const char *)(b) + (b)->msg_disp)
> > -#define BUGFRAME_warn   0
> > -#define BUGFRAME_bug    1
> > -#define BUGFRAME_assert 2
> > +#define BUGFRAME_run_fn 0
> > +#define BUGFRAME_warn   1
> > +#define BUGFRAME_bug    2
> > +#define BUGFRAME_assert 3
> Why did you renumber it? IOW, why can't BUGFRAME_run_fn be defined as 3?
This matches x86 definition. IMO there is no reason to have a different
definition and this will make it more obvious that it might be a good
idea to have a common include/xen/bug.h header.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature