|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>
Hi Julien,
On Wed, 2023-02-01 at 22:11 +0000, Julien Grall wrote:
>
>
> On 01/02/2023 17:40, Oleksii wrote:
> > Hi Julien,
>
> Hi Oleksii,
>
> > On Mon, 2023-01-30 at 22:28 +0000, Julien Grall wrote:
> > > Hi Oleksii,
> > >
> > > On 30/01/2023 11:35, Oleksii wrote:
> > > > Hi Julien,
> > > > On Fri, 2023-01-27 at 16:02 +0000, Julien Grall wrote:
> > > > > Hi Oleksii,
> > > > >
> > > > > On 27/01/2023 13:59, Oleksii Kurochko wrote:
> > > > > > The patch introduces macros: BUG(), WARN(),
> > > > > > run_in_exception(),
> > > > > > assert_failed.
> > > > > >
> > > > > > The implementation uses "ebreak" instruction in combination
> > > > > > with
> > > > > > diffrent bug frame tables (for each type) which contains
> > > > > > useful
> > > > > > information.
> > > > > >
> > > > > > Signed-off-by: Oleksii Kurochko
> > > > > > <oleksii.kurochko@xxxxxxxxx>
> > > > > > ---
> > > > > > Changes:
> > > > > > - Remove __ in define namings
> > > > > > - Update run_in_exception_handler() with
> > > > > > register void *fn_ asm(__stringify(BUG_FN_REG)) =
> > > > > > (fn);
> > > > > > - Remove bug_instr_t type and change it's usage to
> > > > > > uint32_t
> > > > > > ---
> > > > > > xen/arch/riscv/include/asm/bug.h | 118
> > > > > > ++++++++++++++++++++++++++++
> > > > > > xen/arch/riscv/traps.c | 128
> > > > > > +++++++++++++++++++++++++++++++
> > > > > > xen/arch/riscv/xen.lds.S | 10 +++
> > > > > > 3 files changed, 256 insertions(+)
> > > > > > create mode 100644 xen/arch/riscv/include/asm/bug.h
> > > > > >
> > > > > > diff --git a/xen/arch/riscv/include/asm/bug.h
> > > > > > b/xen/arch/riscv/include/asm/bug.h
> > > > > > new file mode 100644
> > > > > > index 0000000000..4b15d8eba6
> > > > > > --- /dev/null
> > > > > > +++ b/xen/arch/riscv/include/asm/bug.h
> > > > > > @@ -0,0 +1,118 @@
> > > > > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > > > > +/*
> > > > > > + * Copyright (C) 2012 Regents of the University of
> > > > > > California
> > > > > > + * Copyright (C) 2021-2023 Vates
> > > > >
> > > > > I have to question the two copyrights here given that the
> > > > > majority of
> > > > > the code seems to be taken from the arm implementation (see
> > > > > arch/arm/include/asm/bug.h).
> > > > >
> > > > > With that said, we should consolidate the code rather than
> > > > > duplicating
> > > > > it on every architecture.
> > > > >
> > > > Copyrights should be removed. They were taken from the previous
> > > > implementation of bug.h for RISC-V so I just forgot to remove
> > > > them.
> > > >
> > > > It looks like we should have common bug.h for ARM and RISCV but
> > > > I
> > > > am
> > > > not sure that I know how to do that better.
> > > > Probably the code wants to be moved to xen/include/bug.h and
> > > > using
> > > > ifdef ARM && RISCV ...
> > >
> > > Or you could introduce CONFIG_BUG_GENERIC or else, so it is
> > > easily
> > > selectable by other architecture.
> > >
> > > > But still I am not sure that this is the best one option as at
> > > > least we
> > > > have different implementation for x86_64.
> > >
> > > My main concern is the maintainance effort. For every bug, we
> > > would
> > > need
> > > to fix it in two places. The risk is we may forget to fix one
> > > architecture.
> > > This is not a very ideal situation.
> > >
> > > So I think sharing the header between RISC-V and Arm (or x86, see
> > > below)
> > > is at least a must. We can do the 3rd architecture at a leisure
> > > pace.
> > >
> > > One option would be to introduce asm-generic like Linux (IIRC
> > > this
> > > was a
> > > suggestion from Andrew). This would also to share code between
> > > two of
> > > the archs.
> > >
> > > Also, from a brief look, the difference in implementation is
> > > mainly
> > > because on Arm we can't use %c (some version of GCC didn't
> > > support
> > > it).
> > > Is this also the case on RISC-V? If not, you may want to consider
> > > to
> > > use
> > > the x86 version.
> > >
> > I did several experiments related to '%c' in inline assembly for
> > RISC-V
> > and it seems that '%c' doesn't support all forms of the use of
> > '%c'.
>
> Thanks for checking!
>
> > I wrote the following macros and they have been compiled without
> > any
> > errors:
> > .....
> > #define _ASM_BUGFRAME_TEXT(second_frame) \
> > ".Lbug%=: ebreak\n" \
> > ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n" \
> > ".p2align 2\n" \
> > ".Lfrm%=:\n" \
> > ".long (.Lfrm%=)\n" \
> > ".long (0x55555555)\n" \
> > ".long (.Lbug%=)\n" \
> > ".long (0x55555555)\n" \
> > ".long %c[bf_line_hi]\n" \
> > ".long (0x55555555)\n" \
> > ".long %[bf_line_hi]\n" \
> > ".long (0x55555555)\n" \
> > ".long %[bf_line_lo]\n" \
> > ".long (0x55555555)\n" \
> > ".long %[bf_ptr]\n" \
> > ".long (0x55555555)\n" \
> > ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n" \
> > ".popsection\n" \
> >
> > #define _ASM_BUGFRAME_INFO(type, line, ptr, msg) \
> > [bf_type] "i" (type), \
> > [bf_ptr] "i" (ptr), \
> > [bf_msg] "i" (msg), \
> > [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1)) \
> > << BUG_DISP_WIDTH), \
> > [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) <<
> > BUG_DISP_WIDTH)
> >
> > #define BUG_FRAME(type, line, ptr, second_frame, msg) do { \
> > __asm__ __volatile__ ( _ASM_BUGFRAME_TEXT(second_frame) \
> > :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );
> > \
> > } while (0)
> > ....
> >
> >
> > But if add ".long %c[bf_ptr]\n" then the following compilation
> > error
> > will occur: [ error: invalid 'asm': invalid use of '%c'. ]
> >
> > If you look at the dissembler of _bug_frames_...
> > ......
> > 80201010: 1010 addi a2,sp,32 #
> > .Lfrm%=
> > 80201012: 8020 .2byte 0x8020
> > 80201014: 5555 li a0,-11
> > 80201016: 5555 li a0,-11
> > 80201018: 3022 .2byte 0x3022 # .Lbug%=
> > 8020101a: 8020 .2byte 0x8020
> > 8020101c: 5555 li a0,-11
> > 8020101e: 5555 li a0,-11
> > 80201020: 0000 unimp #
> > %c[bf_line_hi]
> > 80201022: 0000 unimp
> > 80201024: 5555 li a0,-11
> > 80201026: 5555 li a0,-11
> > 80201028: 0000 unimp #
> > %[bf_line_hi]
> > 8020102a: 0000 unimp
> > 8020102c: 5555 li a0,-11
> > 8020102e: 5555 li a0,-11
> > 80201030: 0000 unimp #
> > %[bf_line_lo]
> > 80201032: 1600 addi s0,sp,800
> > 80201034: 5555 li a0,-11
> > 80201036: 5555 li a0,-11
> > 80201038: 10b8 addi a4,sp,104 #
> > %[bf_ptr]
> > 8020103a: 8020 .2byte 0x8020
> > 8020103c: 5555 li a0,-11
> > 8020103e: 5555 li a0,-11
> > 80201040: 2012 .2byte 0x2012 #
> > (.Lbug%= -
> > .Lfrm%=) + %c[bf_line_hi]
> > .....
> > ... it looks like the error will be generated if the name in
> > %c[name]
> > isn't equal to 0.
> >
> > Another thing I noticed is that %[name] can be used instead of
> > %c[name]
> > for RISC-V ( i did a quick check and it works) so it is still
> > possible
> > to follow Intel implementation but required a redefinition of
> > _ASM_BUGFRAME_TEXT where %c[...] won't be used. All the rest will
> > be
> > the same as in x86 implementation:
> > .....
> > #define _ASM_BUGFRAME_TEXT(second_frame) \
> > ".Lbug%=: ebreak\n" \
> > ".pushsection .bug_frames.%[bf_type], \"a\", @progbits\n" \
> > ".p2align 2\n" \
> > ".Lfrm%=:\n" \
> > ".long (.Lbug%= - .Lfrm%=) + %[bf_line_hi]\n" \
> > ".long (%[bf_ptr] - .Lfrm%=) + %[bf_line_lo]\n" \
> > ".if " #second_frame "\n" \
> > ".long 0, %[bf_msg] - .Lfrm%=\n" \
> > ".endif\n" \
> > ".popsection\n" \
> > .....
> >
> > One thing I would like to ask you is why it's better to follow/use
> > x86
> > implementation instead of ARM?
>
> IMHO, the x86 version is cleaner. But...
>
> > It seems that "%c[...]" has the best support only for Intel GCC and
> > thereby ARM implementation looks more universal, doesn't it?
>
> ... you are right, the Arm version is more portable. Although, I do
> wonder how GCC managed to have a different behavior between
> architecture
> as I would have expected %c to be a generic implementation.
>
> Anyway, if you are basing on the Arm one, then you should be able to
> 1) move arch/arm/include/asm/bug.h in asm-generic/bug.h (or
> similar)
> 2) Rename the guard and remove arm specific code.(I am not sure
> from
> where to include arm{32, 64}/bug.h)
> 3) Define BUG_INSTR to ebreak on RISC-V.
> 4) Find a place for all the RISC-V specific header
> 5) Move do_bug_frame() in common/bug.c
>
> I am happy to help testing the Arm version and/or help moving the
> code
> to common.
>
Thanks a lot for the help offered.
I've started to rework bug.h stuff but faced an issue.
I am trying to introduce GENERIC_BUG_FRAME config ( only for ARM now as
some stuff isn't available now for RISC-V such as find_text_region(),
printk() and so on... Thereby I will switch to do_bug_frame() to
generic one a little bit later for RISCV ) so I added the following to
Kconfig:
config GENERIC_DO_BUG_FRAME
bool "Generic implementation of do_bug_frame()"
default y if ARM
default n
help
...
But when I pushed the commit to GitLab all ARM randconfig jobs failed
because they decided not to set GENERIC_BUG_FRAME=y.
Could you please give me a suggestion how I can work around this
problem? ( I thought that it would be enough to use default y but
randconfig can override it ).
Or is it needed to provide an empty implementation for do_bug_frame()
GENERIC_BUG_FRAME=n?
Also I thought about weak function as an option...
Here is pipeline for generic bug frame feature and the patch ( of
course not ready for upstream but at least it shows an idea ):
*
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/766581174
*
https://gitlab.com/xen-project/people/olkur/xen/-/commit/6fc6481202852e0aa2c2cb3877f2d71ac0213511
P.S.: Probably you have some comments ( something that is unacceptable
even now ) about the patch. I will happy to hear them too.
Thanks in advance.
> Cheers,
>
~ Oleksii
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |