Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3696.80.82.1.1\))
Subject: Re: [PATCH v9 1/7] RISC-V: Clear SIP bit only when using SBI IPI
 operations
From:   Jessica Clarke <jrtc27@jrtc27.com>
In-Reply-To: <CAK9=C2U5MjF+qv9z=_cSS4iN66dCY_goF0D_-ujcPPG9VrReDw@mail.gmail.com>
Date:   Sun, 4 Sep 2022 22:45:23 +0100
Cc:     Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Daniel Lezcano <daniel.lezcano@linaro.org>,
        Atish Patra <atishp@atishpatra.org>,
        Alistair Francis <Alistair.Francis@wdc.com>,
        Anup Patel <anup@brainfault.org>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Bin Meng <bmeng.cn@gmail.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <E4342C74-F530-4F7F-B5EE-B86F3682D7A8@jrtc27.com>
References: <20220903161309.32848-1-apatel@ventanamicro.com>
 <20220903161309.32848-2-apatel@ventanamicro.com>
 <6ABE254E-C7DA-468B-9A6B-D10FF32E0E89@jrtc27.com>
 <CAK9=C2U5MjF+qv9z=_cSS4iN66dCY_goF0D_-ujcPPG9VrReDw@mail.gmail.com>
To:     Anup Patel <apatel@ventanamicro.com>
Precedence: bulk

On 4 Sept 2022, at 05:39, Anup Patel <apatel@ventanamicro.com> wrote:
>=20
> On Sun, Sep 4, 2022 at 1:46 AM Jessica Clarke <jrtc27@jrtc27.com> =
wrote:
>>=20
>> ]On 3 Sept 2022, at 17:13, Anup Patel <apatel@ventanamicro.com> =
wrote:
>>>=20
>>> 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.
>>=20
>> Uh, where is this specified? I don=E2=80=99t 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).
>=20
> For mip.MSIP, refer the below text from section 3.1.9 of the RISC-V =
privileged
> specification:
>=20
> "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."
>=20
> For sip.SSIP, refer the below text from section 4.1.3 of the RISC-V =
privileged
> specification:
>=20
> "If implemented, SSIP is writable in sip and may also be set to 1 by a
> platform-specific interrupt controller."

I misinterpreted you; I thought you were saying SSIP could be set from
S-mode but not M-mode, not that you were talking about the bit
corresponding to the mode. I agree MSIP is different to SSIP
(unfortunately).

>> This patch does make sense for a different reason though: that IPIs =
may
>> not be using software interrupts at all (in the IMSIC case).
>=20
> Sure, let me help you understand this better.
>=20
> 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:
>=20
> 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.
>=20
> 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.
>=20
> 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-ris=
cv-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.
>=20
> 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.
>=20
> 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).

Yes, that=E2=80=99s what I was trying to communicate as to what the =
motivation
of the patch should be (though omitted the M-mode case because I
sometimes forget that Linux tries to support this odd-ball
configuration). I always agreed with the content of the patch, just
disagreed with the message. Now you=E2=80=99ve explained what you meant =
I don=E2=80=99t
think it=E2=80=99s wrong, but I do think it could be clearer, especially =
since
the IMSIC case (and I guess ACLINT SSWI too, which is yet another
option) should be stated to justify why it=E2=80=99s tied to the SBI IPI
implementation and not under !IS_ENABLED(CONFIG_RISCV_M_MODE).

Jess

> Regards,
> Anup
>=20
>>=20
>> Jess
>>=20
>>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>>> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>> arch/riscv/kernel/sbi.c | 8 +++++++-
>>> arch/riscv/kernel/smp.c | 2 --
>>> 2 files changed, 7 insertions(+), 3 deletions(-)
>>>=20
>>> 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);
>>> }
>>>=20
>>> +static void sbi_ipi_clear(void)
>>> +{
>>> +     csr_clear(CSR_IP, IE_SIE);
>>> +}
>>> +
>>> static const struct riscv_ipi_ops sbi_ipi_ops =3D {
>>> -     .ipi_inject =3D sbi_send_cpumask_ipi
>>> +     .ipi_inject =3D sbi_send_cpumask_ipi,
>>> +     .ipi_clear =3D sbi_ipi_clear
>>> };
>>>=20
>>> 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);
>>>=20
>>> --
>>> 2.34.1
>>>=20
>>>=20
>>> _______________________________________________
>>> linux-riscv mailing list
>>> linux-riscv@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>=20