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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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?

> --- 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.

>      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()?

> @@ -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.

> @@ -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.

Jan