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

[Juergen Gross]

On 14.03.23 09:49, HAGIO KAZUHITO(萩尾 一仁) wrote:
> 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 at suse.com>
>> ---
>>    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);

Fine with me.


Juergen

-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xB0DE9DD628BF132F.asc
Type: application/pgp-keys
Size: 3098 bytes
Desc: OpenPGP public key
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20230314/a9ed6eb4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20230314/a9ed6eb4/attachment.sig>