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

From: Oleksii <oleksii.kurochko@gmail.com>
To: Julien Grall <julien@xen.org>, xen-devel@lists.xenproject.org
Cc: Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	 Stefano Stabellini <sstabellini@kernel.org>,
	Gianluca Guida <gianluca@rivosinc.com>,
	Bob Eshleman <bobbyeshleman@gmail.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	 Connor Davis <connojdavis@gmail.com>
Subject: Re: [PATCH v2 12/14] xen/riscv: introduce an implementation of macros from <asm/bug.h>
Date: Tue, 31 Jan 2023 14:34:14 +0200	[thread overview]
Message-ID: <fdbf044d79b23b33b75a684f5124fa218f826a32.camel@gmail.com> (raw)
In-Reply-To: <873d4754-0314-0022-96a9-e54ed78ac6ef@xen.org>

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@gmail.com>
> > > > ---
> > > > 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.
> 
No, it shouldn't be an issue for RISC-V. I'll double check.
Any way it should bug.h should be shared between archs so I am going to
rework that in this patch series and sent the changes in the next
version of the patch series.

Thanks.

~Oleksii
> Cheers,
> 