Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38;
MIME-Version: 1.0
References: <20231023082911.23242-1-luxu.kernel@bytedance.com> <CAOnJCUKH0muFHWujXEqJtb4Uv6Kfh5DeJeR2qg9vj7Kc5w43dw@mail.gmail.com>
In-Reply-To: <CAOnJCUKH0muFHWujXEqJtb4Uv6Kfh5DeJeR2qg9vj7Kc5w43dw@mail.gmail.com>
From:   Xu Lu <luxu.kernel@bytedance.com>
Date:   Thu, 26 Oct 2023 21:56:45 +0800
Message-ID: <CAPYmKFuNr18_jJYZ6X4Rrty=bU0cXiuHpAqSt2+YqDWV6rAveg@mail.gmail.com>
Subject: Re: [External] Re: [RFC 00/12] riscv: Introduce Pseudo NMI
To:     Atish Patra <atishp@atishpatra.org>
Cc:     paul.walmsley@sifive.com, palmer@dabbelt.com,
        aou@eecs.berkeley.edu, tglx@linutronix.de, maz@kernel.org,
        anup@brainfault.org, dengliang.1214@bytedance.com,
        liyu.yukiteru@bytedance.com, sunjiadong.lff@bytedance.com,
        xieyongji@bytedance.com, lihangjing@bytedance.com,
        chaiwen.cc@bytedance.com, linux-kernel@vger.kernel.org,
        linux-riscv@lists.infradead.org,
        =?UTF-8?B?Q2zDqW1lbnQgTMOpZ2Vy?= <cleger@rivosinc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Oct 26, 2023 at 7:02=E2=80=AFAM Atish Patra <atishp@atishpatra.org>=
 wrote:
>
> On Mon, Oct 23, 2023 at 1:29=E2=80=AFAM Xu Lu <luxu.kernel@bytedance.com>=
 wrote:
> >
> > Sorry to resend this patch series as I forgot to Cc the open list befor=
e.
> > Below is formal content.
> >
> > The existing RISC-V kernel lacks an NMI mechanism as there is still no
> > ratified resumable NMI extension in RISC-V community, which can not
> > satisfy some scenarios like high precision perf sampling. There is an
> > incoming hardware extension called Smrnmi which supports resumable NMI
> > by providing new control registers to save status when NMI happens.
> > However, it is still a draft and requires privilege level switches for
> > kernel to utilize it as NMIs are automatically trapped into machine mod=
e.
> >
> > This patch series introduces a software pseudo NMI mechanism in RISC-V.
> > The existing RISC-V kernel disables interrupts via per cpu control
> > register CSR_STATUS, the SIE bit of which controls the enablement of al=
l
> > interrupts of whole cpu. When SIE bit is clear, no interrupt is enabled=
.
> > This patch series implements NMI by switching interrupt disable way to
> > another per cpu control register CSR_IE. This register controls the
> > enablement of each separate interrupt. Each bit of CSR_IE corresponds
> > to a single major interrupt and a clear bit means disablement of
> > corresponding interrupt.
> >
> > To implement pseudo NMI, we switch to CSR_IE masking when disabling
> > irqs. When interrupts are disabled, all bits of CSR_IE corresponding to
> > normal interrupts are cleared while bits corresponding to NMIs are stil=
l
> > kept as ones. The SIE bit of CSR_STATUS is now untouched and always kep=
t
> > as one.
> >
> > We measured performacne of Pseudo NMI patches based on v6.6-rc4 on SiFi=
ve
> > FU740 Soc with hackbench as our benchmark. The result shows 1.90%
> > performance degradation.
> >
> >   "hackbench 200 process 1000" (average over 10 runs)
> >   +-----------+----------+------------+
> >   |           | v6.6-rc4 | Pseudo NMI |
> >   +-----------+----------+------------+
> >   |   time    | 251.646s |  256.416s  |
> >   +-----------+----------+------------+
> >
> > The overhead mainly comes from two parts:
> >
> >   1. Saving and restoring CSR_IE register during kernel entry/return.
> >   This part introduces about 0.57% performance overhead.
> >
> >   2. The extra instructions introduced by 'irqs_enabled_ie'. It is a
> >   special value representing normal CSR_IE when irqs are enabled. It is
> >   implemented via ALTERNATIVE to adapt to platforms without PMU. This
> >   part introduces about 1.32% performance overhead.
> >
>
> We had an evaluation of this approach earlier this year and concluded
> with the similar findings.
> The pseudo NMI is only useful for profiling use case which doesn't
> happen all the time in the system
> Adding the cost to the hotpath and sacrificing performance for
> everything for something for performance profiling
> is not desirable at all.

Thanks a lot for your reply!

First, please allow me to explain that CSR_IE Pseudo NMI actually can suppo=
rt
more than PMU profiling. For example, if we choose to make external major
interrupt as NMI and use ithreshold or eithreshold in AIA to control which =
minor
external interrupts can be sent to CPU, then we actually can support multip=
le
minor interrupts as NMI while keeping the other minor interrupts still
normal irqs.
This is what we are working on now.

Also, if we take virtualization scenarios into account, CSR_IE Pseudo NMI c=
an
support NMI passthrough to VM without too much effort from hypervisor, if o=
nly
corresponding interrupt can be delegated to VS-mode. I wonder if SSE suppor=
ts
interrupt passthrough to VM?

>
> That's why, an SBI extension Supervisor Software Events (SSE) is under
> development.
> https://lists.riscv.org/g/tech-prs/message/515
>
> Instead of selective disabling of interrupts, SSE takes an orthogonal
> approach where M-mode would invoke a special trap
> handler. That special handler will invoke the driver specific handler
> which would be registered by the driver (i.e. perf driver)
> This covers both firmware first RAS and perf use cases.
>
> The above version of the specification is a bit out-of-date and the
> revised version will be sent soon.
> Clement(cc'd) has also done a PoC of SSE and perf driver using the SSE
> framework. This resulted in actual saving
> in performance for RAS/perf without sacrificing the normal performance.
>
> Clement is planning to send the series soon with more details.

The SSE extension you mentioned is a brilliant design and does solve a lot =
of
problems!

We have considered implementing NMI via SBI calls before. The main problem
is that if a driver using NMI needs to cooperate with SBI code, extra
coupling will
be introduced as the driver vendor and firmware vendor may not be the same =
one.
We think perhaps it is better to keep SBI code as simple and stable as poss=
ible.

Please correct me if there is any misunderstanding.

Thanks again and looking forward to your reply.

>
> > Limits:
> >
> >   CSR_IE is now used for disabling irqs and any other code should
> >   not touch this register to avoid corrupting irq status, which means
> >   we do not support masking a single interrupt now.
> >
> >   We have tried to fix this by introducing a per cpu variable to save
> >   CSR_IE value when disabling irqs. Then all operatations on CSR_IE
> >   will be redirected to this variable and CSR_IE's value will be
> >   restored from this variable when enabling irqs. Obviously this method
> >   introduces extra memory accesses in hot code path.
> >
>
>
>
> > TODO:
> >
> >   1. The adaption to hypervisor extension is ongoing.
> >
> >   2. The adaption to advanced interrupt architecture is ongoing.
> >
> > This version of Pseudo NMI is rebased on v6.6-rc7.
> >
> > Thanks in advance for comments.
> >
> > Xu Lu (12):
> >   riscv: Introduce CONFIG_RISCV_PSEUDO_NMI
> >   riscv: Make CSR_IE register part of context
> >   riscv: Switch to CSR_IE masking when disabling irqs
> >   riscv: Switch back to CSR_STATUS masking when going idle
> >   riscv: kvm: Switch back to CSR_STATUS masking when entering guest
> >   riscv: Allow requesting irq as pseudo NMI
> >   riscv: Handle pseudo NMI in arch irq handler
> >   riscv: Enable NMIs during irqs disabled context
> >   riscv: Enable NMIs during exceptions
> >   riscv: Enable NMIs during interrupt handling
> >   riscv: Request pmu overflow interrupt as NMI
> >   riscv: Enable CONFIG_RISCV_PSEUDO_NMI in default
> >
> >  arch/riscv/Kconfig                 | 10 ++++
> >  arch/riscv/include/asm/csr.h       | 17 ++++++
> >  arch/riscv/include/asm/irqflags.h  | 91 ++++++++++++++++++++++++++++++
> >  arch/riscv/include/asm/processor.h |  4 ++
> >  arch/riscv/include/asm/ptrace.h    |  7 +++
> >  arch/riscv/include/asm/switch_to.h |  7 +++
> >  arch/riscv/kernel/asm-offsets.c    |  3 +
> >  arch/riscv/kernel/entry.S          | 18 ++++++
> >  arch/riscv/kernel/head.S           | 10 ++++
> >  arch/riscv/kernel/irq.c            | 17 ++++++
> >  arch/riscv/kernel/process.c        |  6 ++
> >  arch/riscv/kernel/suspend_entry.S  |  1 +
> >  arch/riscv/kernel/traps.c          | 54 ++++++++++++++----
> >  arch/riscv/kvm/vcpu.c              | 18 ++++--
> >  drivers/clocksource/timer-clint.c  |  4 ++
> >  drivers/clocksource/timer-riscv.c  |  4 ++
> >  drivers/irqchip/irq-riscv-intc.c   | 66 ++++++++++++++++++++++
> >  drivers/perf/riscv_pmu_sbi.c       | 21 ++++++-
> >  18 files changed, 340 insertions(+), 18 deletions(-)
> >
> > --
> > 2.20.1
> >
>
>
> --
> Regards,
> Atish