Re: [PATCH 9/9] um: Add initial SMP support

[Tiwei Bie <tiwei.bie@linux.dev>]

On Mon, 28 Jul 2025 12:47:08 +0200, Johannes Berg wrote:
> On Sun, 2025-07-27 at 14:29 +0800, Tiwei Bie wrote:
> > 
> > +++ b/arch/um/include/asm/smp.h
> > @@ -2,6 +2,27 @@
> >  #ifndef __UM_SMP_H
> >  #define __UM_SMP_H
> >  
> > -#define hard_smp_processor_id()		0
> > +#if IS_ENABLED(CONFIG_SMP)
> > +
> > +#include <linux/bitops.h>
> > +#include <asm/current.h>
> > +#include <linux/cpumask.h>
> > +#include <shared/smp.h>
> > +
> > +#define raw_smp_processor_id() uml_curr_cpu()
> > +
> > +void arch_smp_send_reschedule(int cpu);
> > +
> > +void arch_send_call_function_single_ipi(int cpu);
> > +
> > +void arch_send_call_function_ipi_mask(const struct cpumask *mask);
> > +
> > +static inline void smp_cpus_done(unsigned int maxcpus) { }
> > +
> > +#else /* !CONFIG_SMP */
> > +
> > +#define raw_smp_processor_id() 0
> 
> This seems a bit odd to me, linux/smp.h also defines
> raw_smp_processor_id() to 0 the same way, unconditionally.
> 
> It almost seems to me we should define raw_smp_processor_id() only for
> SMP? But then also __smp_processor_id()? Maybe not?

I think you're right. I should't define raw_smp_processor_id() for non-SMP.

> 
> linux-arch folks, do you have any comments?
> 
> > --- /dev/null
> > +++ b/arch/um/include/asm/spinlock.h
> > @@ -0,0 +1,8 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __ASM_UM_SPINLOCK_H
> > +#define __ASM_UM_SPINLOCK_H
> > +
> > +#include <asm/processor.h>
> > +#include <asm-generic/spinlock.h>
> > +
> > +#endif /* __ASM_UM_SPINLOCK_H */
> 
> Do we need this file? Maybe asm-generic should be including the right
> things it needs?

I added this file to include asm/processor.h; otherwise, there would be
a lot of compilation errors. Other architectures seem to do the same:

$ grep -r asm/processor.h arch/ | grep asm/spinlock.h
arch/arm/include/asm/spinlock.h:#include <asm/processor.h>
arch/alpha/include/asm/spinlock.h:#include <asm/processor.h>
arch/arc/include/asm/spinlock.h:#include <asm/processor.h>
arch/hexagon/include/asm/spinlock.h:#include <asm/processor.h>
arch/parisc/include/asm/spinlock.h:#include <asm/processor.h>
arch/x86/include/asm/spinlock.h:#include <asm/processor.h>
arch/s390/include/asm/spinlock.h:#include <asm/processor.h>
arch/mips/include/asm/spinlock.h:#include <asm/processor.h>
arch/loongarch/include/asm/spinlock.h:#include <asm/processor.h>

> 
> > +void enter_turnstile(struct mm_id *mm_id);
> > +void exit_turnstile(struct mm_id *mm_id);
> 
> We could add __acquires(turnstile) and __releases(turnstile) or
> something, to have sparse check that it's locked/unlocked correctly, but
> not sure it's worth it.

Will do.

> 
> > +int disable_kmalloc[NR_CPUS] = { 0 };
> 
> nit: you can remove the "0".

Will fix all the nits in the next version.

> 
> > +int smp_sigio_handler(struct uml_pt_regs *regs)
> > +{
> > +	int cpu = raw_smp_processor_id();
> > +
> > +	IPI_handler(cpu, regs);
> > +	if (cpu != 0)
> > +		return 1;
> > +	return 0;
> 
> nit: "return cpu != 0;" perhaps
> 
> > +__uml_setup("ncpus=", uml_ncpus_setup,
> > +"ncpus=<# of desired CPUs>\n"
> > +"    This tells UML how many virtual processors to start. The maximum\n"
> > +"    number of supported virtual processors can be obtained by querying\n"
> > +"    the CONFIG_NR_CPUS option using --showconfig.\n\n"
> 
> 
> I feel like probably this should at least for now be mutually exclusive
> with time-travel= parameters, at least if it's not 1? Or perhaps only
> with time-travel=ext?

Currently, the UML_TIME_TRAVEL_SUPPORT option depends on !SMP:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/um/Kconfig?h=v6.16#n218

so they can't be enabled at the same time during build.

> 
> The timer code is in another patch, will look at that also. I guess
> until then it's more of a gut feeling on "how would this work" :)

Thanks for the review! :)

Regards,
Tiwei