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
