[v9] RISC-V IPI Improvements

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

Posted by Anup Patel 3 years, 7 months ago

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.

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

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

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

Posted by Atish Patra 3 years, 7 months ago

On Sat, Sep 3, 2022 at 9:13 AM Anup Patel <apatel@ventanamicro.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.
>
> 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(-)
>
> 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
>

Reviewed-by: Atish Patra <atishp@rivosinc.com>


-- 
Regards,
Atish

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

Posted by Marc Zyngier 3 years, 7 months ago

On Sat, 03 Sep 2022 17:13:03 +0100,
Anup Patel <apatel@ventanamicro.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.
> 
> 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(-)
> 
> 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);

This really begs the question: why on Earth are these things exported
to *modules*? I cannot see a good reason why they should be...

	M>

-- 
Without deviation from the norm, progress is not possible.

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

Posted by Anup Patel 3 years, 7 months ago

On Thu, Sep 8, 2022 at 2:08 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sat, 03 Sep 2022 17:13:03 +0100,
> Anup Patel <apatel@ventanamicro.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.
> >
> > 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(-)
> >
> > 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);
>
> This really begs the question: why on Earth are these things exported
> to *modules*? I cannot see a good reason why they should be...

I agree, the riscv_clear_ipi() should not be exported but the PATCH4
("RISC-V: Treat IPIs as normal Linux IRQs") of this series removes
this function.

Regards,
Anup

>
>         M>
>
> --
> Without deviation from the norm, progress is not possible.

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

Posted by Jessica Clarke 3 years, 7 months ago

]On 3 Sept 2022, at 17:13, Anup Patel <apatel@ventanamicro.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).

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

Jess

> 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(-)
> 
> 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@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

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

Posted by Anup Patel 3 years, 7 months ago

On Sun, Sep 4, 2022 at 1:46 AM Jessica Clarke <jrtc27@jrtc27.com> wrote:
>
> ]On 3 Sept 2022, at 17:13, Anup Patel <apatel@ventanamicro.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@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(-)
> >
> > 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@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>

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

Posted by Jessica Clarke 3 years, 7 months ago

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

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@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(-)
>>> 
>>> 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@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>>