Message-ID: <CAK9=C2U5MjF+qv9z=_cSS4iN66dCY_goF0D_-ujcPPG9VrReDw@mail.gmail.com>
Date:   Sun, 4 Sep 2022 10:09:57 +0530
From:   Anup Patel <apatel@...tanamicro.com>
To:     Jessica Clarke <jrtc27@...c27.com>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Atish Patra <atishp@...shpatra.org>,
        Alistair Francis <Alistair.Francis@....com>,
        Anup Patel <anup@...infault.org>,
        linux-riscv <linux-riscv@...ts.infradead.org>,
        linux-kernel@...r.kernel.org, Bin Meng <bmeng.cn@...il.com>
Subject: Re: [PATCH v9 1/7] RISC-V: Clear SIP bit only when using SBI IPI operations

On Sun, Sep 4, 2022 at 1:46 AM Jessica Clarke <jrtc27@...c27.com> wrote:
>
> ]On 3 Sept 2022, at 17:13, Anup Patel <apatel@...tanamicro.com> wrote:
> >
> > The software interrupt pending (i.e. [M|S]SIP) bit is writeable for
> > S-mode but read-only for M-mode so we clear this bit only when using
> > SBI IPI operations.
>
> Uh, where is this specified? I don’t see that, and that would be odd.
> The spec clearly says that if supervisor software interrupts are
> implemented then the bit is writeable, with no caveats on when (beyond
> the permissions required to access the CSR in general).

For mip.MSIP, refer the below text from section 3.1.9 of the RISC-V privileged
specification:

"MSIP is read-only in mip, and is written by accesses to memory-mapped
control registers, which are used by remote harts to provide machine-level
interprocessor interrupts."

For sip.SSIP, refer the below text from section 4.1.3 of the RISC-V privileged
specification:

"If implemented, SSIP is writable in sip and may also be set to 1 by a
platform-specific interrupt controller."

>
> This patch does make sense for a different reason though: that IPIs may
> not be using software interrupts at all (in the IMSIC case).

Sure, let me help you understand this better.

The IPI support in Linux RISC-V does not assume any particular IPI
mechanism. In fact, we will be supporting multiple IPI mechanism in
Linux RISC-V and one of these will be selected at boot-time based
on platform capabilities:

1) Linux RISC-V NoMMU (M-mode) kernel will use IPIs provided by
the CLINT timer driver (refer, drivers/clocksource/timer-clint.c). This
mechanism uses the CLINT MMIO registers to update the state of
mip.MSIP bit. The M-mode kernel cannot directly modify the mip.MSIP
bit without going through the CLINT MMIO registers.

2) Linux RISC-V MMU (S-mode) kernel without IMSIC will use IPIs
provided by the SBI IPI calls (refer, arch/riscv/kernel/sbi.c). This
mechanism uses the SBI SEND_IPI call which sets the sip.SSIP
bit from M-mode firmware and kernel itself will clear sip.SSIP bit
after handling IPI on target HART.

3) Linux RISC-V MMU (S-mode) kernel with IMSIC will use IPIs
as software injected MSIs (refer,
https://github.com/avpatel/linux/blob/riscv_aia_v1/drivers/irqchip/irq-riscv-imsic.c).
This mechanism does not use "Software Interrupt" bits defined in
the mip/sip CSR. Instead IPIs are regular external interrupts injected
via IMSIC and imsic_handle_irq() of the IMSIC driver clears the
IPI pending through stopei CSR.

Currently, we were blindly clearing mip.[M|S]SIP bit in riscv_clear_ipi()
which is a common function for all three mechanisms above which
is not right considering the diversity in IPI mechanisms supported
by Linux RISC-V.

This patch moves the clearing of SSIP bit to SBI IPI code so that it is
only done for mechanism #2 (described above) and not done for
mechanisms #1 & #3 (described above).

Regards,
Anup

>
> Jess
>
> > Signed-off-by: Anup Patel <apatel@...tanamicro.com>
> > Reviewed-by: Bin Meng <bmeng.cn@...il.com>
> > ---
> > arch/riscv/kernel/sbi.c | 8 +++++++-
> > arch/riscv/kernel/smp.c | 2 --
> > 2 files changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> > index 775d3322b422..fc614650a2e3 100644
> > --- a/arch/riscv/kernel/sbi.c
> > +++ b/arch/riscv/kernel/sbi.c
> > @@ -643,8 +643,14 @@ static void sbi_send_cpumask_ipi(const struct cpumask *target)
> >       sbi_send_ipi(target);
> > }
> >
> > +static void sbi_ipi_clear(void)
> > +{
> > +     csr_clear(CSR_IP, IE_SIE);
> > +}
> > +
> > static const struct riscv_ipi_ops sbi_ipi_ops = {
> > -     .ipi_inject = sbi_send_cpumask_ipi
> > +     .ipi_inject = sbi_send_cpumask_ipi,
> > +     .ipi_clear = sbi_ipi_clear
> > };
> >
> > void __init sbi_init(void)
> > diff --git a/arch/riscv/kernel/smp.c b/arch/riscv/kernel/smp.c
> > index 760a64518c58..c56d67f53ea9 100644
> > --- a/arch/riscv/kernel/smp.c
> > +++ b/arch/riscv/kernel/smp.c
> > @@ -83,8 +83,6 @@ void riscv_clear_ipi(void)
> > {
> >       if (ipi_ops && ipi_ops->ipi_clear)
> >               ipi_ops->ipi_clear();
> > -
> > -     csr_clear(CSR_IP, IE_SIE);
> > }
> > EXPORT_SYMBOL_GPL(riscv_clear_ipi);
> >
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

Powered by blists - more mailing lists