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

From: Juergen Gross <jgross@suse.com>
To: Oleksii <oleksii.kurochko@gmail.com>,
	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: Mon, 30 Jan 2023 12:49:11 +0100	[thread overview]
Message-ID: <e1943855-729e-8bb2-4af2-138be84c0576@suse.com> (raw)
In-Reply-To: <2c4d490bde4f04f956e481257ddc7129c312abb6.camel@gmail.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 2707 bytes --]

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@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 ...
> 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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]