On Thu, Aug 29, 2024 at 03:30:26PM GMT, Andrew Jones wrote:
> On Mon, Aug 26, 2024 at 01:08:24AM GMT, James Raphael Tiovalen wrote:
...
> > +.section .data
> > +.balign PAGE_SIZE
> > +.global sbi_hsm_hart_start_checks
> > +sbi_hsm_hart_start_checks: .space CONFIG_NR_CPUS
> > +.global sbi_hsm_non_retentive_hart_suspend_checks
> > +sbi_hsm_non_retentive_hart_suspend_checks: .space CONFIG_NR_CPUS
> > +.global sbi_hsm_stop_hart
> > +sbi_hsm_stop_hart: .space CONFIG_NR_CPUS
>
> I don't think it should be necessary to create these arrays in assembly.
> We should be able to make global arrays in C in riscv/sbi.c and still
> access them from the assembly as you've done.
>
> CONFIG_NR_CPUS will support all possible cpuids, but hartids have their
> own range and the code above is indexing these arrays by hartid. Since
> we should be able to define the arrays in C, then we could also either
>
> 1) assert that max_hartid + 1 <= NR_CPUS
> 2) dynamically allocate the arrays using max_hartid + 1 for the size
> and then assign global variables the physical addresses of those
> allocated regions to use in the assembly
>
> (1) is probably good enough
Actually we have to do (1) unless we want to open a big can of worms
because we're currently shoehorning hartids into cpumasks, but cpumasks
are based on NR_CPUS for size. To do it right, we should have hartmasks,
but they may be very large and/or sparse.
>
> Seems on-cpus is missing an API for "all, but ...". How about, as a
> separate patch, adding
>
> void on_cpus_async(cpumask_t *mask, void (*func)(void *data), void *data)
>
> to lib/on-cpus.c
>
> Then here you'd copy the present mask to your own mask and clear 'me' from
> it for an 'all present, but me' mask.
I'll write a patch for on_cpus_async() now.
> but, instead, let's provide the following (untested) function in lib/riscv/sbi.c
>
> struct sbiret sbi_send_ipi_cpumask(cpumask_t *mask)
> {
> struct sbiret ret;
>
> for (int i = 0; i < CPUMASK_NR_LONGS; ++i) {
> if (cpumask_bits(mask)[i]) {
> ret = sbi_send_ipi(cpumask_bits(mask)[i], i * BITS_PER_LONG);
> if (ret.error)
> break;
> }
> }
>
> return ret;
> }
>
I'll write a patch adding this now too.
Thanks,
drew
