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

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

On 16.01.21 18:19, Julien Grall wrote:
> Hi Juergen,
>
> On 16/01/2021 10:33, Juergen Gross wrote:
> > Add support to run a function in an exception handler for Arm. Do it
> > as on x86 via a bug_frame, but pass the function pointer via a
> > register (this needs to be done that way, because inline asm support
> > for 32-bit Arm lacks the needed functionality to reference an
> > arbitrary function via the bugframe).
> >
> > Use the same BUGFRAME_* #defines as on x86 in order to make a future
> > common header file more easily achievable.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> > V4:
> > - new patch
> >
> > V5:
> > - adjust BUG_FRAME() macro (Jan Beulich)
> > - adjust arm linker script (Jan Beulich)
> > - drop #ifdef x86 in common/virtual_region.c
> >
> > V6:
> > - use register for function address (Arm32 build failure noticed by
> >    Julien Grall)
> Thank you for trying to sort out the issue on Arm32 :).
>
> > +/*
> > + * Unfortunately gcc for arm 32-bit doesn't support the "i" 
> > constraint, so
> > + * the easiest way to implement run_in_exception_handler() is to pass 
> > the
> > + * to be called function in a fixed register.
> There are a few uses of "i" in Linux Arm32 (such as jump label), 
> therefore I am pretty confident "i" works at least in some situation.
> I did some more digging this afternoon. Our use of "i" is very similar 
> to Linux, so I decided to look at the GCC flags used.
> It turns out that Linux will always build with -fno-pie on Arm (either 
> 32 or 64) whereas Xen will let the compiler to decide (PIE is enabled by 
> on my compiler).
> I wrote a small example to test the issue (based on Linux static key)
>
> static void test(void)
> {
> }
>
> int main(void)
> {
>      asm volatile (".pushsection .bug_frames, \"aw\"\n"
>                ".quad %0\n"
>                ".popsection\n"
>                :: "i"(test));
>
>      return 0;
> }
>
> If I add -fno-pie on the command, the constraint error disappears.
>
> On the previous version, you rewrote that didn't see any error. Is it 
> possible that your compiler is disabling PIE by default?
> I think we need to code to be position independent for at least UEFI. I 
> also have plan to look at selecting the Xen virtual address at boot time 
> (this is necessary to solve some memory issue on Arm).
>  From a quick test, if I use -fno-pie -fpic, then the snipped above will 
> build fine. But it is not entirely clear whether the code would still be 
> position independent.
> I need to have a look how Linux is dealing with KASLR given that 
> -fno-pie is used...
> > + */
> > +#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)
> My concern with this approach is we are going to clobber multiple 
> registers. On Arm64, this code will be replaced by:
>      13cc:       90000001        adrp    x1, 0 <show_execution_state>
>      13d0:       91000021        add     x1, x1, #0x0
>      13d4:       aa0103e0        mov     x0, x1
>      13d8:       d4200020        brk     #0x1
>
> I guess we can optimize it to just clobber one register. Do we expect 
> the function executed to rely/replace the content of the registers?
No, it is just to have an interrupt frame to print out. Basically it is
just a "normal" function call with no parameters and return value via
an interrupt. So other than the BUG_ON() the registers are quite
uninteresting, it is nothing meant to be used for diagnosis AFAICS.


Juergen

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

Attachment: OpenPGP_signature
Description: OpenPGP digital signature