Re: [XEN PATCH 4/9] x86/smp: move stack_base to cpu_data

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 05/02/2024 08:41, Jan Beulich wrote:
> On 02.02.2024 19:24, Julien Grall wrote:
> > >    static bool smp_store_cpu_info(unsigned int id)
> > > @@ -579,8 +581,6 @@ static int do_boot_cpu(int apicid, int cpu)
> > >            printk("Booting processor %d/%d eip %lx\n",
> > >                   cpu, apicid, start_eip);
> > >    
> > > -    stack_start = stack_base[cpu] + STACK_SIZE - sizeof(struct cpu_info);
> > > -
> > You remove this line because I can't quite figure out where stack_start
> > is now set. This is used...
> This line sets a global variable, which ...
>
> > > @@ -856,7 +856,7 @@ int setup_cpu_root_pgt(unsigned int cpu)
> > >    
> > >        /* Install direct map page table entries for stack, IDT, and TSS. */
> > >        for ( off = rc = 0; !rc && off < STACK_SIZE; off += PAGE_SIZE )
> > > -        rc = clone_mapping(__va(__pa(stack_base[cpu])) + off, rpt);
> > > +        rc = clone_mapping(__va(__pa(cpu_data[cpu].stack_base)) + off, rpt);
> > >    
> > >        if ( !rc )
> > >            rc = clone_mapping(idt_tables[cpu], rpt);
> > > @@ -1007,10 +1007,10 @@ static void cpu_smpboot_free(unsigned int cpu, bool 
> > > remove)
> > >            FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
> > >            FREE_XENHEAP_PAGE(idt_tables[cpu]);
> > >    
> > > -        if ( stack_base[cpu] )
> > > +        if ( cpu_data[cpu].stack_base )
> > >            {
> > > -            memguard_unguard_stack(stack_base[cpu]);
> > > -            FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
> > > +            memguard_unguard_stack(cpu_data[cpu].stack_base);
> > > +            FREE_XENHEAP_PAGES(cpu_data[cpu].stack_base, STACK_ORDER);
> > >            }
> > >        }
> > >    }
> > > @@ -1044,11 +1044,11 @@ static int cpu_smpboot_alloc(unsigned int cpu)
> > >        if ( node != NUMA_NO_NODE )
> > >            memflags = MEMF_node(node);
> > >    
> > > -    if ( stack_base[cpu] == NULL &&
> > > -         (stack_base[cpu] = cpu_alloc_stack(cpu)) == NULL )
> > > +    if ( cpu_data[cpu].stack_base == NULL &&
> > > +         (cpu_data[cpu].stack_base = cpu_alloc_stack(cpu)) == NULL )
> > >                goto out;
> > >    
> > > -    info = get_cpu_info_from_stack((unsigned long)stack_base[cpu]);
> > > +    info = get_cpu_info_from_stack((unsigned long)cpu_data[cpu].stack_base);
> > ... here.
> ... pretty clearly is not used here (anymore). Instead I'd raise the
> question of what the remaining purpose of that variable then is.
> Looking through updates this patch alone makes to use sites of
> stack_start, it's unclear whether the use from assembly code has gone
> away already - brief checking suggests it hasn't.
Whoops you are right. I am not sure how I thought this line was (still) 
using stack_start.
Cheers,

--
Julien Grall