[Crash-utility] [PATCH V2] RISCV64: Use va_kernel_pa_offset in VTOP()

[Song Shuai]

在 2023/8/15 16:42, HAGIO KAZUHITO(萩尾 一仁) 写道:
> On 2023/08/15 15:44, lijiang wrote:
>> On Tue, Aug 15, 2023 at 10:40 AM Song Shuai <suagrfillet at gmail.com> wrote:
>>
>>>
>>> 在 2023/8/14 16:27, lijiang 写道:
>>>>       +static void
>>>>       +riscv64_get_va_kernel_pa_offset(struct machine_specific *ms)
>>>>       +{
>>>>       +       unsigned long kernel_version = riscv64_get_kernel_version();
>>>>       +
>>>>       +       /*
>>>>       +        * Since Linux v6.4 phys_base is not the physical start of
>>>>       the kernel,
>>>>       +        * trying to use "va_kernel_pa_offset" to determine the
>>>>       offset between
>>>>       +        * kernel virtual and physical addresses.
>>>>       +        */
>>>>       +       if (kernel_version >= LINUX(6,4,0)) {
>>>>
>>>> Is it possible to detect the existence of the symbol
>>>> 'linear_mapping_va_to_pa' or 'linear_mapping_pa_to_va' to decide reading
>>>> the value of 'va_kernel_pa_offset'? For example:
>>>> kernel_symbol_exists()/symbol_exists()
>>>>
>>>> if (kernel_symbol_exists("linear_mapping_va_to_pa") ||
>>>> kernel_symbol_exists("linear_mapping_pa_to_va")) {
>>>>        string = pc->read_vmcoreinfo("NUMBER(va_kernel_pa_offset)");
>>>>        ...
>>>> }
>>>
>>> The `linear_mapping_va_to_pa` and `linear_mapping_pa_to_va` symbols will
>>> only be exported when the debug option -- CONFIG_DEBUG_VIRTUAL is
>>> enabled, otherwise they will expanded as macros. As the kernel Makefile
>>
>>
>> That is really problematic. If so, I tend to read the vmcoreinfo directly
>> as below. I haven't tested it , just an idea.
>>
>> string = pc->read_vmcoreinfo("NUMBER(va_kernel_pa_offset)");
>> if (string) {
>>       ms->va_kernel_pa_offset = htol(string, QUIET, NULL);
>>       free(string);
>>       if (CRASHDEBUG(1))
>>           fprintf(fp, "NUMBER(va_kernel_pa_offset): %lx\n",
>> ms->va_kernel_pa_offset);
>> } else
>>       ms->va_kernel_pa_offset = ms->kernel_link_addr - ms->phys_base;
>>
>> But that depends on how you and Kazu think about it.
> 
> I was also thinking about a similar way like this:
> 
>     if ((string = pc->read_vmcoreinfo(...
>        ms->va_kernel_pa_offset = ...
>     else if (kernel_vesrion >= LINUX(6,4,0))
>        error()
>     else
>        ms->va_kernel_pa_offset = ...
> > but personally I don't suppose the kernel commit 3335068f8721 is likely
> to be backported to an old kernel, so the current patch is also fine to
> me.  We can fix it when needed.  I'd defer to riscv folks here.
> 
I agree.

Additionally, this commit (riscv: Use PUD/P4D/PGD pages for the linear 
mapping) was blamed for and fixed by a number of well-known problems 
(such as hibernation, UEFI boot, etc.), so IMO, it's hard to backport it 
to an older kernel.


> Thanks,
> Kazu
> 
> 
>>
>>
>>>
>>> says:
>>>
>>> obj-$(CONFIG_DEBUG_VIRTUAL) += physaddr.o
>>>
>>> Actually, it's hard to extract some explicit infomation from the commit
>>> 3335068f8721 and use them as the first `if` condition, so the kernel
>>> version checking was the final choice.
>>>
>>>
>> The kernel version checking is not very good, although it has some similar
>> code in crash-utility. Once the related kernel code is backported to the
>> old kernel(such as a stable branch), crash won't work well on these vmcore.
>> As you mentioned, the kernel version checking is the last resort if there
>> is no better way.
>>
>> Thanks.
>> Lianbo
>>
>>
>>>>
>>>> I saw the commit 3335068f8721 exported two symbols:
>>>>
>>>> +phys_addr_t linear_mapping_va_to_pa(unsigned long x)
>>>> +{
>>>> +       BUG_ON(!kernel_map.va_pa_offset);
>>>> +
>>>> +       return ((unsigned long)(x) - kernel_map.va_pa_offset);
>>>> +}
>>>> +EXPORT_SYMBOL(linear_mapping_va_to_pa);
>>>> +
>>>> +void *linear_mapping_pa_to_va(unsigned long x)
>>>> +{
>>>> +       BUG_ON(!kernel_map.va_pa_offset);
>>>> +
>>>> +       return ((void *)((unsigned long)(x) + kernel_map.va_pa_offset));
>>>> +}
>>>> +EXPORT_SYMBOL(linear_mapping_pa_to_va);
>> >

-- 
Thanks
Song Shuai