[PATCH v2 0/3] RISC-V: Add new smp features

[PATCH v2 0/3] RISC-V: Add new smp features

[Anup Patel]

On Tue, Aug 28, 2018 at 4:36 AM, Atish Patra <atish.patra@wdc.com> wrote:
> This patch series implements following smp related features.
> Some of the work has been inspired from ARM64.
>
> 1. Decouple linux logical cpu ids from hardware cpu id
> 2. Support cpu hotplug.
>
> Tested on QEMU & HighFive Unleashed board with/without SMP enabled.
>
> v1->v2:
>
> 1. Dropped cpu_ops patch.
> 2. Moved back IRQ cause definiations to irq.h
> 3. Keep boot cpu hart id and assign zero as the cpu id for boot cpu.
> 4. Renamed cpu id and hart id correctly.
>
> Atish Patra (3):
>   RISC-V: Add logical CPU indexing for RISC-V
>   RISC-V: Use Linux logical cpu number instead of hartid
>   RISC-V: Support cpu hotplug.
>

This series looks good to me.

FWIW,
Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

[PATCH v2 0/3] RISC-V: Add new smp features

[Christoph Hellwig]

On Thu, Aug 30, 2018 at 09:53:50AM -0400, Anup Patel wrote:
> > Atish Patra (3):
> >   RISC-V: Add logical CPU indexing for RISC-V
> >   RISC-V: Use Linux logical cpu number instead of hartid
> >   RISC-V: Support cpu hotplug.
> >
> 
> This series looks good to me.

Hmm, that series didn't make it to my inbox, neither directly, nor
through the linux-riscv list.  Where did you see it?

[PATCH v2 0/3] RISC-V: Add new smp features

[Anup Patel]

On Thu, Aug 30, 2018 at 10:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Aug 30, 2018 at 09:53:50AM -0400, Anup Patel wrote:
>> > Atish Patra (3):
>> >   RISC-V: Add logical CPU indexing for RISC-V
>> >   RISC-V: Use Linux logical cpu number instead of hartid
>> >   RISC-V: Support cpu hotplug.
>> >
>>
>> This series looks good to me.
>
> Hmm, that series didn't make it to my inbox, neither directly, nor
> through the linux-riscv list.  Where did you see it?

Strange, I have subscribed to:
linux-kernel at vger.kernel.org
linux-riscv at lists.infradead.org

--
Anup

[PATCH v2 0/3] RISC-V: Add new smp features

[Anup Patel]

On Thu, Aug 30, 2018 at 10:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Aug 30, 2018 at 09:53:50AM -0400, Anup Patel wrote:
>> > Atish Patra (3):
>> >   RISC-V: Add logical CPU indexing for RISC-V
>> >   RISC-V: Use Linux logical cpu number instead of hartid
>> >   RISC-V: Support cpu hotplug.
>> >
>>
>> This series looks good to me.
>
> Hmm, that series didn't make it to my inbox, neither directly, nor
> through the linux-riscv list.  Where did you see it?

Ahh, looks like some issue indeed. I am in TO list of patch series, thats why
I got the patch series.

--
Anup

[PATCH v2 0/3] RISC-V: Add new smp features

[Atish Patra]

On 8/30/18 7:18 AM, Anup Patel wrote:
> On Thu, Aug 30, 2018 at 10:11 AM, Christoph Hellwig <hch@infradead.org> wrote:
>> On Thu, Aug 30, 2018 at 09:53:50AM -0400, Anup Patel wrote:
>>>> Atish Patra (3):
>>>>    RISC-V: Add logical CPU indexing for RISC-V
>>>>    RISC-V: Use Linux logical cpu number instead of hartid
>>>>    RISC-V: Support cpu hotplug.
>>>>
>>>
>>> This series looks good to me.
>>
>> Hmm, that series didn't make it to my inbox, neither directly, nor
>> through the linux-riscv list.  Where did you see it?
> 

Strange.

> Ahh, looks like some issue indeed. I am in TO list of patch series, thats why
> I got the patch series.
> 

So is Christoff & linux-riscv. I see it in lkml but not in linux-riscv.
Apologies for the issues. I will resend it again.

Regards,
Atish
> --
> Anup
> 

[PATCH v2 1/3] RISC-V: Add logical CPU indexing for RISC-V

[Christoph Hellwig]

On Tue, Aug 28, 2018 at 01:36:08AM -0700, Atish Patra wrote:
> Currently, both linux cpu id and hardware cpu id are same.
> This is not recommended as it will lead to discontinuous cpu
> indexing in Linux. Moreover, kdump kernel will run from CPU0
> which would be absent if we follow existing scheme.
> 
> Implement a logical mapping between Linux cpu id and hardware
> cpuid to decouple these two. Always mark the boot processor as
> cpu0 and all other cpus get the logical cpu id based on their
> booting order.
> 
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
> ---
>  arch/riscv/include/asm/smp.h | 18 +++++++++++++++++-
>  arch/riscv/kernel/setup.c    |  2 ++
>  arch/riscv/kernel/smp.c      | 19 +++++++++++++++++++
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
> index 36016845..a5c257b3 100644
> --- a/arch/riscv/include/asm/smp.h
> +++ b/arch/riscv/include/asm/smp.h
> @@ -22,6 +22,13 @@
>  #include <linux/cpumask.h>
>  #include <linux/irqreturn.h>
>  
> +#define INVALID_HARTID -1
> +/*
> + * Mapping between linux logical cpu index and hartid.
> + */
> +extern unsigned long __cpu_logical_map[NR_CPUS];
> +#define cpu_logical_map(cpu)    __cpu_logical_map[cpu]

How about naming this cpuid_to_hardid_map to make things a little
more obvious?  Also shouldn't this be signed given your INVALID_HARTID
definition above.

> +static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
> +static inline void riscv_cpuid_to_hartid_mask(const struct cpumask *in,
> +				       struct cpumask *out) {
> +	cpumask_set_cpu(cpu_logical_map(0), out);
> +}

Please use normal coding style even for stubs:

static inline int riscv_hartid_to_cpuid(int hartid)
{
	return 0;
}

static inline void riscv_cpuid_to_hartid_mask(const struct cpumask *in,
			struct cpumask *out)
{
	cpumask_set_cpu(cpu_logical_map(0), out);
}

> +unsigned long __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HARTID };

Please break the line into the usual format:

unsigned long __cpu_logical_map[NR_CPUS] = {
	[0 ... NR_CPUS-1]	= INVALID_HARTID,
};

[PATCH v2 2/3] RISC-V: Use Linux logical cpu number instead of hartid

[Christoph Hellwig]

> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
> +				     unsigned long size)
> +{
> +	struct cpumask hmask;
> +
> +	riscv_cpuid_to_hartid_mask(cmask, &hmask);
> +	sbi_remote_sfence_vma(hmask.bits, start, size);
> +}
> +
> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)

flush_tlb_all passed NULL to sbi_remote_sfence_vma before, so this
changes what we pass.  I think we should keep the existing behavior.

> @@ -93,10 +94,11 @@ static inline void plic_toggle(int ctxid, int hwirq, int enable)
>  static inline void plic_irq_toggle(struct irq_data *d, int enable)
>  {
>  	int cpu;
> +	struct plic_handler *handler;
>  
>  	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>  	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
> -		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
> +		handler = per_cpu_ptr(&plic_handlers, cpu);

This looks like a spurious change.

[PATCH v2 3/3] RISC-V: Support cpu hotplug.

[Christoph Hellwig]

> +#else
> +static inline bool can_hotplug_cpu(void) { return 0; }
> +static inline void arch_send_call_wakeup_ipi(int cpu) { }

Please use normal coding style for these stubs.

>  #define INTERRUPT_CAUSE_FLAG	(1UL << (__riscv_xlen - 1))
> +#define get_scause(cause)	(cause & ~INTERRUPT_CAUSE_FLAG)

I think this helper is misleading - the cause includes the interrupt
flag.  I'd rather open code this in the other place as well.

> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
> index d7c6ca7c..cb209139 100644
> --- a/arch/riscv/kernel/process.c
> +++ b/arch/riscv/kernel/process.c
> @@ -42,6 +42,13 @@ void arch_cpu_idle(void)
>  	local_irq_enable();
>  }
>  
> +#ifdef CONFIG_HOTPLUG_CPU
> +void arch_cpu_idle_dead(void)
> +{
> +	cpu_play_dead();
> +}
> +#endif

I wonder if it might be worth to introduce a small
arch/riscv/kernel/cpu-hotplug.c file for the various CONFIG_HOTPLUG_CPU
only functions.

[PATCH v2 1/3] RISC-V: Add logical CPU indexing for RISC-V

[Atish Patra]

On 8/30/18 11:03 PM, Christoph Hellwig wrote:
> On Tue, Aug 28, 2018 at 01:36:08AM -0700, Atish Patra wrote:
>> Currently, both linux cpu id and hardware cpu id are same.
>> This is not recommended as it will lead to discontinuous cpu
>> indexing in Linux. Moreover, kdump kernel will run from CPU0
>> which would be absent if we follow existing scheme.
>>
>> Implement a logical mapping between Linux cpu id and hardware
>> cpuid to decouple these two. Always mark the boot processor as
>> cpu0 and all other cpus get the logical cpu id based on their
>> booting order.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>> ---
>>   arch/riscv/include/asm/smp.h | 18 +++++++++++++++++-
>>   arch/riscv/kernel/setup.c    |  2 ++
>>   arch/riscv/kernel/smp.c      | 19 +++++++++++++++++++
>>   3 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/smp.h b/arch/riscv/include/asm/smp.h
>> index 36016845..a5c257b3 100644
>> --- a/arch/riscv/include/asm/smp.h
>> +++ b/arch/riscv/include/asm/smp.h
>> @@ -22,6 +22,13 @@
>>   #include <linux/cpumask.h>
>>   #include <linux/irqreturn.h>
>>   
>> +#define INVALID_HARTID -1
>> +/*
>> + * Mapping between linux logical cpu index and hartid.
>> + */
>> +extern unsigned long __cpu_logical_map[NR_CPUS];
>> +#define cpu_logical_map(cpu)    __cpu_logical_map[cpu]
> 
> How about naming this cpuid_to_hardid_map to make things a little
> more obvious?  

Sure.
Also shouldn't this be signed given your INVALID_HARTID
> definition above.
> 

I don't know what I was thinking after adding INVALID_HARTID. I will fix 
this.

>> +static inline int riscv_hartid_to_cpuid(int hartid) { return 0 ; }
>> +static inline void riscv_cpuid_to_hartid_mask(const struct cpumask *in,
>> +				       struct cpumask *out) {
>> +	cpumask_set_cpu(cpu_logical_map(0), out);
>> +}
> 
> Please use normal coding style even for stubs:
> 
> static inline int riscv_hartid_to_cpuid(int hartid)
> {
> 	return 0;
> }
> 
> static inline void riscv_cpuid_to_hartid_mask(const struct cpumask *in,
> 			struct cpumask *out)
> {
> 	cpumask_set_cpu(cpu_logical_map(0), out);
> }
> 
>> +unsigned long __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = INVALID_HARTID };
> 
> Please break the line into the usual format:
> 


> unsigned long __cpu_logical_map[NR_CPUS] = {
> 	[0 ... NR_CPUS-1]	= INVALID_HARTID,
> };
> 
ok.

Regards,
Atish

[PATCH v2 3/3] RISC-V: Support cpu hotplug.

[Atish Patra]

On 8/30/18 11:19 PM, Christoph Hellwig wrote:
>> +#else
>> +static inline bool can_hotplug_cpu(void) { return 0; }
>> +static inline void arch_send_call_wakeup_ipi(int cpu) { }
> 
> Please use normal coding style for these stubs.

Done.
> 
>>   #define INTERRUPT_CAUSE_FLAG	(1UL << (__riscv_xlen - 1))
>> +#define get_scause(cause)	(cause & ~INTERRUPT_CAUSE_FLAG)
> 
> I think this helper is misleading - the cause includes the interrupt
> flag.  I'd rather open code this in the other place as well.
> 

ok. Done.

>> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
>> index d7c6ca7c..cb209139 100644
>> --- a/arch/riscv/kernel/process.c
>> +++ b/arch/riscv/kernel/process.c
>> @@ -42,6 +42,13 @@ void arch_cpu_idle(void)
>>   	local_irq_enable();
>>   }
>>   
>> +#ifdef CONFIG_HOTPLUG_CPU
>> +void arch_cpu_idle_dead(void)
>> +{
>> +	cpu_play_dead();
>> +}
>> +#endif
> 
> I wonder if it might be worth to introduce a small
> arch/riscv/kernel/cpu-hotplug.c file for the various CONFIG_HOTPLUG_CPU
> only functions.
> 

I have kept it here to match all other arch. Same goes for all hotplug 
functions in smpboot.c

I can move them to a separate file if you think that provides better 
code readability and structure.


Regards,
Atish 		

[PATCH v2 2/3] RISC-V: Use Linux logical cpu number instead of hartid

[Atish Patra]

On 8/30/18 11:11 PM, Christoph Hellwig wrote:
>> -#define flush_tlb_all() sbi_remote_sfence_vma(NULL, 0, -1)
>> +static inline void remote_sfence_vma(struct cpumask *cmask, unsigned long start,
>> +				     unsigned long size)
>> +{
>> +	struct cpumask hmask;
>> +
>> +	riscv_cpuid_to_hartid_mask(cmask, &hmask);
>> +	sbi_remote_sfence_vma(hmask.bits, start, size);
>> +}
>> +
>> +#define flush_tlb_all() remote_sfence_vma(NULL, 0, -1)
> 
> flush_tlb_all passed NULL to sbi_remote_sfence_vma before, so this
> changes what we pass.  I think we should keep the existing behavior.
> 

sure. How about this ?


--- a/arch/riscv/include/asm/tlbflush.h
+++ b/arch/riscv/include/asm/tlbflush.h
@@ -55,8 +55,13 @@ static inline void remote_sfence_vma(struct cpumask 
*cmask, unsigned long start,
  {
         struct cpumask hmask;

-       riscv_cpuid_to_hartid_mask(cmask, &hmask);
-       sbi_remote_sfence_vma(hmask.bits, start, size);
+       if (cmask == NULL) {
+               sbi_remote_sfence_vma(NULL, start, size);
+       } else {
+               cpumask_clear(&hmask);
+               riscv_cpuid_to_hartid_mask(cmask, &hmask);
+               sbi_remote_sfence_vma(hmask.bits, start, size);
+       }
  }

>> @@ -93,10 +94,11 @@ static inline void plic_toggle(int ctxid, int hwirq, int enable)
>>   static inline void plic_irq_toggle(struct irq_data *d, int enable)
>>   {
>>   	int cpu;
>> +	struct plic_handler *handler;
>>   
>>   	writel(enable, plic_regs + PRIORITY_BASE + d->hwirq * PRIORITY_PER_ID);
>>   	for_each_cpu(cpu, irq_data_get_affinity_mask(d)) {
>> -		struct plic_handler *handler = per_cpu_ptr(&plic_handlers, cpu);
>> +		handler = per_cpu_ptr(&plic_handlers, cpu);
> 
> This looks like a spurious change.
> 

I think I did this to avoid possible compiler warnings. Will revert it.

Regards,
Atish

[PATCH v2 3/3] RISC-V: Support cpu hotplug.

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 11:08:35AM -0700, Atish Patra wrote:
> 
> I have kept it here to match all other arch. Same goes for all hotplug
> functions in smpboot.c
> 
> I can move them to a separate file if you think that provides better code
> readability and structure.

I don't really have a strong opinion, but keeping this code together
and reducing the ifdef maze seems worthwile to me.

[PATCH v2 2/3] RISC-V: Use Linux logical cpu number instead of hartid

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 01:35:10PM -0700, Atish Patra wrote:
> sure. How about this ?

That would work, but why not just keep calling sbi_remove_sfence_vma
directly from flush_tlb_all?

[PATCH v2 3/3] RISC-V: Support cpu hotplug.

[Atish Patra]

On 9/4/18 2:36 PM, Christoph Hellwig wrote:
> On Tue, Sep 04, 2018 at 11:08:35AM -0700, Atish Patra wrote:
>>
>> I have kept it here to match all other arch. Same goes for all hotplug
>> functions in smpboot.c
>>
>> I can move them to a separate file if you think that provides better code
>> readability and structure.
> 
> I don't really have a strong opinion, but keeping this code together
> and reducing the ifdef maze seems worthwile to me.
> 
Sure. I will move it a new file accordingly in v3. If nobody else has 
objection for that, we will keep it that way.

Regards,
Atish

[PATCH v2 2/3] RISC-V: Use Linux logical cpu number instead of hartid

[Atish Patra]

On 9/4/18 2:36 PM, Christoph Hellwig wrote:
> On Tue, Sep 04, 2018 at 01:35:10PM -0700, Atish Patra wrote:
>> sure. How about this ?
> 
> That would work, but why not just keep calling sbi_remove_sfence_vma
> directly from flush_tlb_all?
> 
I guess that's fine too. I just wanted to keep all flush_tlb_* same 
format to make it more coherent.

Regards,
Atish

[PATCH v2 2/3] RISC-V: Use Linux logical cpu number instead of hartid

[Christoph Hellwig]

On Tue, Sep 04, 2018 at 02:43:13PM -0700, Atish Patra wrote:
> On 9/4/18 2:36 PM, Christoph Hellwig wrote:
> > On Tue, Sep 04, 2018 at 01:35:10PM -0700, Atish Patra wrote:
> > > sure. How about this ?
> > 
> > That would work, but why not just keep calling sbi_remove_sfence_vma
> > directly from flush_tlb_all?
> > 
> I guess that's fine too. I just wanted to keep all flush_tlb_* same format
> to make it more coherent.

I'd just keep it simple by calling directly.  While the compiler would
probably optimize away the branch in an inline function we can just
avoid it entirely that way.