Re: [PATCH v9 1/7] RISC-V: Clear SIP bit only when using SBI IPI operations

[Jessica Clarke]

On 4 Sept 2022, at 05:39, Anup Patel <apatel@xxxxxxxxxxxxxxxx> wrote:
> 
> On Sun, Sep 4, 2022 at 1:46 AM Jessica Clarke <jrtc27@xxxxxxxxxx> wrote:
>> 
>> ]On 3 Sept 2022, at 17:13, Anup Patel <apatel@xxxxxxxxxxxxxxxx> 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."

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).
> 
> 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).

Yes, that’s 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’ve explained what you meant I don’t
think it’s 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’s tied to the SBI IPI
implementation and not under !IS_ENABLED(CONFIG_RISCV_M_MODE).

Jess

> Regards,
> Anup
> 
>> 
>> Jess
>> 
>>> Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
>>> Reviewed-by: Bin Meng <bmeng.cn@xxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxx
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>