Re: [PATCH v2 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>

[Juergen Gross <jgross@xxxxxxxx>]

On 30.01.23 12: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 ...
> But still I am not sure that this is the best one option as at least we
> have different implementation for x86_64.
There are already a lot of duplicated #defines in the Arm and x86 asm/bug.h
files.

I'd create xen/include/xen/bug.h including asm/bug.h first and then adding
all the common stuff.

In case 2 archs are sharing some #define FOO you could #define FOO in the
asm/bug.h for the arch not using the common definition and do #ifndef FOO
in xen/include/xen/bug.h around the variant shared by the other archs.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature