Re: [Crash-utility] [PATCH 2/3] xen: get stack address via stack_base array if available

Hi Juergen, thank you for the patches. On 2023/03/13 22:01, Juergen Gross wrote: > Since many years now the stack address of each percpu stack is > available via the stack_base[] array now. Use that instead of the > indirect method via the percpu variables tss_init or tss_page, > especially as the layout of tss_page has changed in Xen 4.16, > resulting in the stack no longer to be found. > > Signed-off-by: Juergen Gross <jgross(a)suse.com&gt; > --- > xen_hyper.c | 50 ++++++++++++++++++++++++++++++-------------------- > 1 file changed, 30 insertions(+), 20 deletions(-) > > diff --git a/xen_hyper.c b/xen_hyper.c > index 1030c0a..72720e2 100644 > --- a/xen_hyper.c > +++ b/xen_hyper.c > @@ -324,7 +324,7 @@ void > xen_hyper_x86_pcpu_init(void) > { > ulong cpu_info; > - ulong init_tss_base, init_tss; > + ulong init_tss_base, init_tss, stack_base; > ulong sp; > struct xen_hyper_pcpu_context *pcc; > char *buf, *bp; > @@ -340,34 +340,44 @@ xen_hyper_x86_pcpu_init(void) > } > /* get physical cpu context */ > xen_hyper_alloc_pcpu_context_space(XEN_HYPER_MAX_CPUS()); > - if (symbol_exists("per_cpu__init_tss")) { > + if (symbol_exists("stack_base")) { > + stack_base = symbol_value("stack_base"); > + flag = 0; > + } else if (symbol_exists("per_cpu__init_tss")) { > init_tss_base = symbol_value("per_cpu__init_tss"); > - flag = TRUE; > + flag = 1; > } else if (symbol_exists("per_cpu__tss_page")) { > init_tss_base = symbol_value("per_cpu__tss_page"); > - flag = TRUE; > + flag = 1; > } else { > init_tss_base = symbol_value("init_tss"); > - flag = FALSE; > + flag = 2; > } > buf = GETBUF(XEN_HYPER_SIZE(tss)); > for_cpu_indexes(i, cpuid) > { > - if (flag) > - init_tss = xen_hyper_per_cpu(init_tss_base, cpuid); > - else > - init_tss = init_tss_base + > - XEN_HYPER_SIZE(tss) * cpuid; > - if (!readmem(init_tss, KVADDR, buf, > - XEN_HYPER_SIZE(tss), "init_tss", RETURN_ON_ERROR)) { > - error(FATAL, "cannot read init_tss.\n"); > - } > - if (machine_type("X86")) { > - sp = ULONG(buf + XEN_HYPER_OFFSET(tss_esp0)); > - } else if (machine_type("X86_64")) { > - sp = ULONG(buf + XEN_HYPER_OFFSET(tss_rsp0)); > - } else > - sp = 0; > + if (flag) { > + if (flag == 1) > + init_tss = xen_hyper_per_cpu(init_tss_base, cpuid); > + else > + init_tss = init_tss_base + > + XEN_HYPER_SIZE(tss) * cpuid; > + if (!readmem(init_tss, KVADDR, buf, > + XEN_HYPER_SIZE(tss), "init_tss", RETURN_ON_ERROR)) { > + error(FATAL, "cannot read init_tss.\n"); > + } > + if (machine_type("X86")) { > + sp = ULONG(buf + XEN_HYPER_OFFSET(tss_esp0)); > + } else if (machine_type("X86_64")) { > + sp = ULONG(buf + XEN_HYPER_OFFSET(tss_rsp0)); > + } else > + sp = 0; > + } else { > + if (!readmem(stack_base + sizeof(ulong) * cpuid, KVADDR, &sp, > + sizeof(ulong), "stack_base", RETURN_ON_ERROR)) { > + error(FATAL, "cannot read stack_base.\n"); > + } > + } > cpu_info = XEN_HYPER_GET_CPU_INFO(sp); > if (CRASHDEBUG(1)) { > fprintf(fp, "sp=%lx, cpu_info=%lx\n", sp, cpu_info); The following warning is emitted with the patch: $ make clean ; make warn ... cc -c -g -DX86_64 -DLZO -DSNAPPY -DZSTD -DGDB_10_2 xen_hyper.c -Wall -O2 -Wstrict-prototypes -Wmissing-prototypes -fstack-protector -Wformat-security xen_hyper.c: In function ‘xen_hyper_x86_pcpu_init’: xen_hyper.c:390:3: warning: ‘init_tss’ may be used uninitialized in this function [-Wmaybe-uninitialized] xen_hyper_store_pcpu_context_tss(pcc, init_tss, buf); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Assuming we have init_tss = 0 first, the buf also may be used as it is (filled by zero), then xen_hyper_store_pcpu_context_tss() looks meaningless. What about not calling it? if (init_tss) xen_hyper_store_pcpu_context_tss(pcc, init_tss, buf);