Re: [Crash-utility] [PATCH] Fix pvops Xen detection for arm machine

On Mon, Dec 20, 2021 at 5:26 PM Qi Zheng <zhengqi.arch(a)bytedance.com&gt; wrote: > > > > On 2021/12/20 4:35 PM, Qi Zheng wrote: >> >> >> On 2021/12/20 4:29 PM, lijiang wrote: >>> Hi, Qi >>> Thank you for the fix. >>> >>>> Date: Thu, 16 Dec 2021 15:09:49 +0800 >>>> From: Qi Zheng <zhengqi.arch(a)bytedance.com&gt; >>>> To: k-hagio-ab(a)nec.com, ptesarik(a)suse.com, lijiang(a)redhat.com >>>> Cc: Qi Zheng <zhengqi.arch(a)bytedance.com&gt;, crash-utility(a)redhat.com >>>> Subject: [Crash-utility] [PATCH] Fix pvops Xen detection for arm >>>> machine >>>> Message-ID: <20211216070949.17668-1-zhengqi.arch(a)bytedance.com&gt; >>>> Content-Type: text/plain; charset="US-ASCII" >>>> >>>> Since the xen_start_info on the arm/arm64 platform is static defined: >>>> >>>> ./arm/xen/enlighten.c:40:static struct start_info _xen_start_info; >>>> ./arm/xen/enlighten.c:41:struct start_info *xen_start_info = >>>> &_xen_start_info; >>>> ./arm/xen/enlighten.c:42:EXPORT_SYMBOL(xen_start_info); >>>> >>> The above variable is statically defined in the kernel, but the >>> 'xen_vcpu_info' is also a static variable, why does the former not >>> work on arm machines, but the latter works well? Could you please >>> explain more details? >>> [linux]$ grep -nr "xen_vcpu_info" * >>> arch/arm/xen/enlighten.c:51:static struct vcpu_info __percpu >>> *xen_vcpu_info; >> >> Hi Lianbo, >> >> The 'xen_vcpu_info' is also a static variable on the arm/arm64 platform, >> but different from 'xen_start_info', it is assigned during the >> initialization process. > > Like the 'xen_start_info' on the x86 platform: > > - definition: > struct start_info *xen_start_info; > EXPORT_SYMBOL_GPL(xen_start_info); > > - initialization: > startup_xen > --> mov %rsi, xen_start_info > > The 'xen_vcpu_info' on the arm/arm64 platform: > > - definition: > static struct vcpu_info __percpu *xen_vcpu_info; > > - initialization: > xen_guest_init > --> xen_vcpu_info = alloc_percpu(struct vcpu_info); > > So I think that it will works well on the arm/arm64 platform. I have > tested it on the arm64 platform(But I can not test it on the arm/arm64 > xen machine). > Thank you for the explanation, Qi. The xen_start_info is only defined in x86 and arm platform, but because the xen_start_info on the arm/arm64 platform points to a static variable '_xen_start_info'(see its definition as below), which makes that the address of xen_start_info will never be null. As a result, the is_pvops_xen() in in commit 4badc6229c69 ("Fix pvops Xen detection for kernels >= v4.20") always returns TRUE, the reason is that it can always read out the non-null address of xen_start_info, and finally caused the failure. [linux]$ grep -nrw "xen_start_info" * arch/arm/xen/enlighten.c:41:struct start_info *xen_start_info = &_xen_start_info; arch/arm/xen/enlighten.c:42:EXPORT_SYMBOL(xen_start_info); ... arch/x86/xen/enlighten.c:49:struct start_info *xen_start_info; arch/x86/xen/enlighten.c:50:EXPORT_SYMBOL_GPL(xen_start_info); In view of this, it could be better to explicitly narrow the 'xen_start_info' check to x86 with the machine_type(), there is no need to check it on other architectures. What do you think?

For example: diff --git a/kernel.c b/kernel.c index f4598ea217a3..b17f3fb9e85e 100644 --- a/kernel.c +++ b/kernel.c @@ -10757,11 +10757,22 @@ is_pvops_xen(void) STREQ(sym, "paravirt_patch_default"))) return TRUE; - if (symbol_exists("xen_start_info") && - readmem(symbol_value("xen_start_info"), KVADDR, &addr, - sizeof(void *), "xen_start_info", RETURN_ON_ERROR) && - addr != 0) - return TRUE; + if (machine_type("X86") || machine_type("X86_64")) { + if (symbol_exists("xen_start_info") && + readmem(symbol_value("xen_start_info"), KVADDR, &addr, + sizeof(void *), "xen_start_info", RETURN_ON_ERROR) && + addr != 0) + return TRUE; + } + + if (machine_type("ARM") || machine_type("ARM64")) { + if (symbol_exists("xen_vcpu_info") && + readmem(symbol_value("xen_vcpu_info"), KVADDR, &addr, + sizeof(void *), "xen_vcpu_info", RETURN_ON_ERROR) && + addr != 0) + return TRUE; + } + return FALSE; } Although the above if(machine_type("ARM")) block has not been tested on the arm Xen platform, it doesn't affect other architectures. Thanks. Lianbo > Thanks, > Qi > >> >> Thanks, >> Qi >> >>> >>> Thanks. >>> Lianbo >>> >>>> The is_pvops_xen() in commit 4badc6229c69f5cd9da7eb7bdf400a53ec6db01a >>>> ("Fix pvops Xen detection for kernels >= v4.20") always return TRUE. >>>> Then the following error will be reported because p2m_mid_missing >>>> and xen_p2m_addr are not defined: >>>> >>>> crash: cannot resolve "p2m_top" >>>> >>>> For the arm/arm64 platform, fix it by using xen_vcpu_info instead of >>>> xen_start_info to detect Xen dumps. >>>> >>>> Signed-off-by: Qi Zheng <zhengqi.arch(a)bytedance.com&gt; >>>> --- >>>> kernel.c | 15 +++++++++++---- >>>> 1 file changed, 11 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/kernel.c b/kernel.c >>>> index f4598ea..4b4d789 100644 >>>> --- a/kernel.c >>>> +++ b/kernel.c >>>> @@ -10757,11 +10757,18 @@ is_pvops_xen(void) >>>> STREQ(sym, "paravirt_patch_default"))) >>>> return TRUE; >>>> >>>> - if (symbol_exists("xen_start_info") && >>>> - readmem(symbol_value("xen_start_info"), KVADDR, &addr, >>>> - sizeof(void *), "xen_start_info", RETURN_ON_ERROR) && >>>> - addr != 0) >>>> + if (machine_type("ARM") || machine_type("ARM64")) { >>>> + if (symbol_exists("xen_vcpu_info") && >>>> + readmem(symbol_value("xen_vcpu_info"), KVADDR, >>>> &addr, >>>> + sizeof(void *), "xen_vcpu_info", RETURN_ON_ERROR) && >>>> + addr != 0) >>>> + return TRUE; >>>> + } else if (symbol_exists("xen_start_info") && >>>> + readmem(symbol_value("xen_start_info"), KVADDR, >>>> &addr, >>>> + sizeof(void *), "xen_start_info", RETURN_ON_ERROR) && >>>> + addr != 0) { >>>> return TRUE; >>>> + } >>>> >>>> return FALSE; >>>> } >>>> -- >>>> 2.11.0 >>> >> > > -- > Thanks, > Qi >