[Crash-utility] [PATCH] Fix pvops Xen detection for arm machine
Qi Zheng
zhengqi.arch at bytedance.com
Tue Dec 21 07:00:07 UTC 2021
On 2021/12/21 2:53 PM, lijiang wrote:
> On Mon, Dec 20, 2021 at 5:26 PM Qi Zheng <zhengqi.arch at bytedance.com> 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 at bytedance.com>
>>>>> To: k-hagio-ab at nec.com, ptesarik at suse.com, lijiang at redhat.com
>>>>> Cc: Qi Zheng <zhengqi.arch at bytedance.com>, crash-utility at redhat.com
>>>>> Subject: [Crash-utility] [PATCH] Fix pvops Xen detection for arm
>>>>> machine
>>>>> Message-ID: <20211216070949.17668-1-zhengqi.arch at bytedance.com>
>>>>> 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?
LGTM, I will send the patch v3.
Thanks,
Qi
> 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 at bytedance.com>
>>>>> ---
>>>>> 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
>>
>
--
Thanks,
Qi
More information about the Crash-utility
mailing list