[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH 4/9] x86/smp: move stack_base to cpu_data
On 5.02.2024 09:41, Jan Beulich wrote:
On 02.02.2024 19:24, Julien Grall wrote:
On 14/11/2023 17:50, Krystian Hebel wrote:
This location is easier to access from assembly. Having it close to
other data required during initialization has also positive (although
rather small) impact on prefetching data from RAM.
I understand your goal but...
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
... cpufeature.h feels a rather odd place for storing the stack. I am
not entirely sure where else to place. Andrew, Jan, Roger?
Well, without having looked at the patch/series itself yet, I can only
say that if struct cpuinfo_x86 really is the place to put this
information, then it's unavoidable to have the field added in this
header. That said, it certainly feels like an abuse - there's nothing
in common with other (collected) data in this structure. "Easier to
access from assembly" may be a fair reason, but then I'd expect the
downsides of alternatives to be discussed explicitly. For example, a
simple new array might be as "easily" accessible from assembly.
Initially I thought I'll be using more fields from this structure
a lot, like
booted_cores or apicid. I'll move this and cpu_state introduced in
following
patch somewhere else.
@@ -37,6 +37,7 @@ struct cpuinfo_x86 {
unsigned int phys_proc_id; /* package ID of each logical CPU */
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;
AFAICT, this means there will be a padding before stack_base and ...
unsigned short x86_clflush_size;
... another one here. Is there any particular reason the new field
wasn't added at the end?
With ...
} __cacheline_aligned;
... this I'm not exactly sure this is a problem right now (there may
be ample padding space anyway, yet I didn't go count). But I agree
with your comment in principle.
I've checked that the size didn't change after adding. I also think
that
I checked that adding it there wouldn't add any padding, but maybe I
miscalculated something. In any way, this will be moved from here.
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -75,13 +75,15 @@ static enum cpu_state {
} cpu_state;
#define set_cpu_state(state) do { smp_mb(); cpu_state = (state); } while (0)
-void *stack_base[NR_CPUS];
-
void initialize_cpu_data(unsigned int cpu)
{
uint32_t apicid = cpu_physical_id(cpu);
+ void *stack = cpu_data[cpu].stack_base;
+
cpu_data[cpu] = boot_cpu_data;
+
cpu_physical_id(cpu) = apicid;
+ cpu_data[cpu].stack_base = stack;
}
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.
BSP still uses it, but APs don't. That said, comment above
declaration says
otherwise, I'll change it, or maybe this variable can be removed
altogether
since it always points to the same place, and there are only two
consumers,
both in assembly.
Jan
Best regards,
--
Krystian Hebel
Firmware Engineer
https://3mdeb.com | @3mdeb_com
|
