Re: [XEN PATCH 8/9] x86/smp: make cpu_state per-CPU

[Krystian Hebel <krystian.hebel@xxxxxxxxx>]

On 8.02.2024 13:13, Jan Beulich wrote:
> On 14.11.2023 18:50, Krystian Hebel wrote:
> > This will be used for parallel AP bring-up.
> >
> > CPU_STATE_INIT changed direction.
> Nit: I think you mean "changes" as you describe what the patch does, not
> what has happened before. But ...
>
> > It was previously set by BSP and never
> > consumed by AP. Now it signals that AP got through assembly part of
> > initialization and waits for BSP to call notifiers that set up data
> > structures required for further initialization.
> ... all of this is, afaict, independent of what the title says the
> purpose of this patch is. Since the correctness of the state change
> adjustments doesn't look straightforward to prove, please split the
> mechanical change from the change to the actual logic.
Ack
> > --- a/xen/arch/x86/include/asm/cpufeature.h
> > +++ b/xen/arch/x86/include/asm/cpufeature.h
> > @@ -38,6 +38,7 @@ struct cpuinfo_x86 {
> >       unsigned int cpu_core_id;          /* core ID of each logical CPU */
> >       unsigned int compute_unit_id;      /* AMD compute unit ID of each logical 
> > CPU */
> >       void *stack_base;
> > +    unsigned int cpu_state;
> >       unsigned short x86_clflush_size;
> >   } __cacheline_aligned;
> Is there any reason this cannot be ordinary per-CPU data?
Probably not, will move it away.
> > --- a/xen/arch/x86/smpboot.c
> > +++ b/xen/arch/x86/smpboot.c
> > @@ -65,15 +65,18 @@ struct cpuinfo_x86 cpu_data[NR_CPUS] =
> >           { [0 ... NR_CPUS-1] .apicid = BAD_APICID };
> >   
> >   static int cpu_error;
> > -static enum cpu_state {
> > +enum cpu_state {
> >       CPU_STATE_DYING,    /* slave -> master: I am dying */
> >       CPU_STATE_DEAD,     /* slave -> master: I am completely dead */
> > -    CPU_STATE_INIT,     /* master -> slave: Early bringup phase 1 */
> > -    CPU_STATE_CALLOUT,  /* master -> slave: Early bringup phase 2 */
> > +    CPU_STATE_INIT,     /* slave -> master: Early bringup phase 1 completed */
> > +    CPU_STATE_CALLOUT,  /* master -> slave: Start early bringup phase 2 */
> It's not really clear to me whether the adding of "Start" on the 2nd line
> really adds value.
Ack
> >       CPU_STATE_CALLIN,   /* slave -> master: Completed phase 2 */
> >       CPU_STATE_ONLINE    /* master -> slave: Go fully online now. */
> > -} cpu_state;
> > -#define set_cpu_state(state) do { smp_mb(); cpu_state = (state); } while (0)
> > +};
> > +#define set_cpu_state(cpu, state) do { \
> > +    smp_mb(); \
> > +    cpu_data[cpu].cpu_state = (state); \
> > +} while (0)
> While you merely re-arrange it, I'd still like to ask: Does this really
> need to be smp_mb(), not just smp_wmb()?
Probably not, but I didn't want to change it, assuming there was a reason
that it used smp_wmb() in the first place.
> > @@ -320,6 +317,10 @@ void start_secondary(unsigned int cpu)
> >   
> >       /* Critical region without IDT or TSS.  Any fault is deadly! */
> >   
> > +    /* Wait until data set up by CPU_UP_PREPARE notifiers is ready. */
> > +    while ( cpu_data[cpu].cpu_state != CPU_STATE_CALLOUT )
> > +        cpu_relax();
> I'm afraid I don't understand the comment (and hence whether this loop
> is actually needed here): __cpu_up() is called only after those
> notifiers completed.
Yes, but broadcasted INIT-SIPI-SIPI sequence added in next patch will be
sent before that call is made, and consequently APs potentially can get
to this point before that data is set up.
> > @@ -1161,6 +1171,12 @@ void __init smp_prepare_cpus(void)
> >       cpu_data[0].stack_base = (void *)
> >                ((unsigned long)stack_start & ~(STACK_SIZE - 1));
> >   
> > +    /* Set state as CALLOUT so APs won't change it in initialize_cpu_data() */
> > +    boot_cpu_data.cpu_state = CPU_STATE_CALLOUT;
> This is actually one of the reasons I don't like you putting the item
> as a new field in struct cpuinfo_x86. Otherwise imo initialize_cpu_data()
> ought to gain a respective assertion.
I'll move it out.
> Jan
Best regards,

--
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com