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

[Julien Grall <julien@xxxxxxx>]

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.
Cheers,

--
Julien Grall