From: "Clément Léger" <cleger@rivosinc.com>
To: Conor Dooley <conor@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Anup Patel <anup@brainfault.org>,
Atish Patra <atishp@atishpatra.org>,
linux-riscv@lists.infradead.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
kvm-riscv@lists.infradead.org, Ved Shanbhogue <ved@rivosinc.com>
Subject: Re: [RFC PATCH 5/7] riscv: add double trap driver
Date: Wed, 24 Apr 2024 10:56:14 +0200 [thread overview]
Message-ID: <5b3f9017-b107-4de1-ab0d-854de160a5f6@rivosinc.com> (raw)
In-Reply-To: <20240423-headsman-arrival-16a2d13342b2@spud>
On 23/04/2024 18:39, Conor Dooley wrote:
> On Thu, Apr 18, 2024 at 04:26:44PM +0200, Clément Léger wrote:
>> Add a small driver to request double trap enabling as well as
>> registering a SSE handler for double trap. This will also be used by KVM
>> SBI FWFT extension support to detect if it is possible to enable double
>> trap in VS-mode.
>>
>> Signed-off-by: Clément Léger <cleger@rivosinc.com>
>> ---
>> arch/riscv/include/asm/sbi.h | 1 +
>> drivers/firmware/Kconfig | 7 +++
>> drivers/firmware/Makefile | 1 +
>> drivers/firmware/riscv_dbltrp.c | 95 +++++++++++++++++++++++++++++++++
>> include/linux/riscv_dbltrp.h | 19 +++++++
>> 5 files changed, 123 insertions(+)
>> create mode 100644 drivers/firmware/riscv_dbltrp.c
>> create mode 100644 include/linux/riscv_dbltrp.h
>>
>> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
>> index 744aa1796c92..9cd4ca66487c 100644
>> --- a/arch/riscv/include/asm/sbi.h
>> +++ b/arch/riscv/include/asm/sbi.h
>> @@ -314,6 +314,7 @@ enum sbi_sse_attr_id {
>> #define SBI_SSE_ATTR_INTERRUPTED_FLAGS_SPIE (1 << 2)
>>
>> #define SBI_SSE_EVENT_LOCAL_RAS 0x00000000
>> +#define SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP 0x00000001
>> #define SBI_SSE_EVENT_GLOBAL_RAS 0x00008000
>> #define SBI_SSE_EVENT_LOCAL_PMU 0x00010000
>> #define SBI_SSE_EVENT_LOCAL_SOFTWARE 0xffff0000
>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> index 59f611288807..a037f6e89942 100644
>> --- a/drivers/firmware/Kconfig
>> +++ b/drivers/firmware/Kconfig
>> @@ -197,6 +197,13 @@ config RISCV_SSE_TEST
>> Select if you want to enable SSE extension testing at boot time.
>> This will run a series of test which verifies SSE sanity.
>>
>> +config RISCV_DBLTRP
>> + bool "Enable Double trap handling"
>> + depends on RISCV_SSE && RISCV_SBI
>> + default n
>> + help
>> + Select if you want to enable SSE double trap handler.
>> +
>> config SYSFB
>> bool
>> select BOOT_VESA_SUPPORT
>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>> index fb7b0c08c56d..ad67a1738c0f 100644
>> --- a/drivers/firmware/Makefile
>> +++ b/drivers/firmware/Makefile
>> @@ -18,6 +18,7 @@ obj-$(CONFIG_RASPBERRYPI_FIRMWARE) += raspberrypi.o
>> obj-$(CONFIG_FW_CFG_SYSFS) += qemu_fw_cfg.o
>> obj-$(CONFIG_RISCV_SSE) += riscv_sse.o
>> obj-$(CONFIG_RISCV_SSE_TEST) += riscv_sse_test.o
>> +obj-$(CONFIG_RISCV_DBLTRP) += riscv_dbltrp.o
>
> As previously mentioned, I'd like to see all of these riscv specific
> things in a riscv directory.
Acked, I was not aware of that.
>
>> obj-$(CONFIG_SYSFB) += sysfb.o
>> obj-$(CONFIG_SYSFB_SIMPLEFB) += sysfb_simplefb.o
>> obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o
>> diff --git a/drivers/firmware/riscv_dbltrp.c b/drivers/firmware/riscv_dbltrp.c
>> new file mode 100644
>> index 000000000000..72f9a067e87a
>> --- /dev/null
>> +++ b/drivers/firmware/riscv_dbltrp.c
>> @@ -0,0 +1,95 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2023 Rivos Inc.
>> + */
>> +
>> +#define pr_fmt(fmt) "riscv-dbltrp: " fmt
>> +
>> +#include <linux/cpu.h>
>> +#include <linux/init.h>
>> +#include <linux/riscv_dbltrp.h>
>> +#include <linux/riscv_sse.h>
>> +
>> +#include <asm/sbi.h>
>> +
>> +static bool double_trap_enabled;
>> +
>> +static int riscv_sse_dbltrp_handle(uint32_t evt, void *arg,
>> + struct pt_regs *regs)
>> +{
>> + __show_regs(regs);
>> + panic("Double trap !\n");
>> +
>> + return 0;
>> +}
>> +
>> +struct cpu_dbltrp_data {
>> + int error;
>> +};
>> +
>> +static void
>> +sbi_cpu_enable_double_trap(void *data)
>
> This should easily fit on one line.
>
>> +{
>> + struct sbiret ret;
>> + struct cpu_dbltrp_data *cdd = data;
>> +
>> + ret = sbi_ecall(SBI_EXT_FWFT, SBI_EXT_FWFT_SET,
>> + SBI_FWFT_DOUBLE_TRAP_ENABLE, 1, 0, 0, 0, 0);
>> +
>> + if (ret.error) {
>> + cdd->error = 1;
>
> If this is a boolean, make it a boolean please. All the code in this
> patch treats it as one.
>
>> + pr_err("Failed to enable double trap on cpu %d\n", smp_processor_id());
>> + }
>> +}
>> +
>> +static int sbi_enable_double_trap(void)
>> +{
>> + struct cpu_dbltrp_data cdd = {0};
>> +
>> + on_each_cpu(sbi_cpu_enable_double_trap, &cdd, 1);
>> + if (cdd.error)
>> + return -1;
>
> Can this be an errno please?
>
>> +
>> + double_trap_enabled = true;
>> +
>> + return 0;
>> +}
>> +
>> +bool riscv_double_trap_enabled(void)
>> +{
>> + return double_trap_enabled;
>> +}
>> +EXPORT_SYMBOL(riscv_double_trap_enabled);
>
> Can we just use double_trap everywhere? dbltrp reads like sound that a
> beatboxer would make and this looks a lot nicer than the other functions
> in the file...
Ahah you gave me a good laugh :D
>
>> +
>> +static int __init riscv_dbltrp(void)
>
> I think this function is missing an action work - init or probe?
>
>> +{
>> + struct sse_event *evt;
>> +
>> + if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SSDBLTRP)) {
>> + pr_err("Ssdbltrp extension not available\n");
>> + return 1;
>> + }
>> +
>> + if (!sbi_probe_extension(SBI_EXT_FWFT)) {
>> + pr_err("Can not enable double trap, SBI_EXT_FWFT is not available\n");
>> + return 1;
>> + }
>> +
>> + if (sbi_enable_double_trap()) {
>> + pr_err("Failed to enable double trap on all cpus\n");
>> + return 1;
>
> Why do we return 1s here, but an errno via PTR_ERR() below?
> Shouldn't all of these be returning a negative errono?
> This particular one should probably propagate the error it got from
> sbi_enable_double_trap().
Indeed, I'll use some errno everywhere.
Thanks,
Clément
>
> Cheers,
> Conor.
>
>> + }
>> +
>> + evt = sse_event_register(SBI_SSE_EVENT_LOCAL_DOUBLE_TRAP, 0,
>> + riscv_sse_dbltrp_handle, NULL);
>> + if (IS_ERR(evt)) {
>> + pr_err("SSE double trap register failed\n");
>> + return PTR_ERR(evt);
>> + }
>> +
>> + sse_event_enable(evt);
>> + pr_info("Double trap handling registered\n");
>> +
>> + return 0;
>> +}
>> +device_initcall(riscv_dbltrp);
>> diff --git a/include/linux/riscv_dbltrp.h b/include/linux/riscv_dbltrp.h
>> new file mode 100644
>> index 000000000000..6de4f43fae6b
>> --- /dev/null
>> +++ b/include/linux/riscv_dbltrp.h
>> @@ -0,0 +1,19 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2023 Rivos Inc.
>> + */
>> +
>> +#ifndef __LINUX_RISCV_DBLTRP_H
>> +#define __LINUX_RISCV_DBLTRP_H
>> +
>> +#if defined(CONFIG_RISCV_DBLTRP)
>> +bool riscv_double_trap_enabled(void);
>> +#else
>> +
>> +static inline bool riscv_double_trap_enabled(void)
>> +{
>> + return false;
>> +}
>> +#endif
>> +
>> +#endif /* __LINUX_RISCV_DBLTRP_H */
>> --
>> 2.43.0
>>
next prev parent reply other threads:[~2024-04-24 8:56 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 14:26 [RFC PATCH 0/7] riscv: Add support for Ssdbltrp extension Clément Léger
2024-04-18 14:26 ` [RFC PATCH 1/7] riscv: kvm: add support for FWFT SBI extension Clément Léger
2024-04-26 23:44 ` Deepak Gupta
2024-04-30 7:26 ` Clément Léger
2024-04-18 14:26 ` [RFC PATCH 2/7] dt-bindings: riscv: add Ssdbltrp ISA extension description Clément Léger
2024-04-23 16:30 ` Conor Dooley
2024-04-24 7:20 ` Clément Léger
2024-04-24 7:40 ` Conor Dooley
2024-04-24 21:38 ` Ved Shanbhogue
2024-04-18 14:26 ` [RFC PATCH 3/7] riscv: add Ssdbltrp ISA extension parsing Clément Léger
2024-04-18 14:26 ` [RFC PATCH 4/7] riscv: handle Ssdbltrp mstatus SDT bit Clément Léger
2024-04-18 14:26 ` [RFC PATCH 5/7] riscv: add double trap driver Clément Léger
2024-04-23 16:39 ` Conor Dooley
2024-04-24 8:56 ` Clément Léger [this message]
2024-04-26 23:59 ` Deepak Gupta
2024-05-14 8:06 ` Clément Léger
2024-05-14 14:38 ` Deepak Gupta
2024-04-18 14:26 ` [RFC PATCH 6/7] riscv: kvm: add SBI FWFT support for SBI_FWFT_DOUBLE_TRAP_ENABLE Clément Léger
2024-04-27 1:17 ` Deepak Gupta
2024-04-27 15:36 ` Deepak Gupta
2024-05-14 9:43 ` Clément Léger
2024-05-14 16:05 ` Deepak Gupta
2024-04-18 14:26 ` [RFC PATCH 7/7] RISC-V: KVM: add support for double trap exception Clément Léger
2024-04-27 1:33 ` Deepak Gupta
2024-04-30 15:35 ` Clément Léger
2024-04-23 16:24 ` [RFC PATCH 0/7] riscv: Add support for Ssdbltrp extension Conor Dooley
2024-04-24 7:23 ` Clément Léger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5b3f9017-b107-4de1-ab0d-854de160a5f6@rivosinc.com \
--to=cleger@rivosinc.com \
--cc=anup@brainfault.org \
--cc=aou@eecs.berkeley.edu \
--cc=atishp@atishpatra.org \
--cc=conor@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=kvm-riscv@lists.infradead.org \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh+dt@kernel.org \
--cc=ved@rivosinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).