Re: [Crash-utility] [PATCH V2] RISCV64: Use va_kernel_pa_offset in VTOP()
在 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(a)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