[Crash-utility] [PATCHv2] crash-utility/arm64: store phy_offset and memstart_addr separately
piliu
piliu at redhat.com
Wed Mar 31 04:05:34 UTC 2021
On 3/30/21 3:34 AM, Bhupesh Sharma wrote:
> Hi Pingfan,
>
> Thanks for the patch.
>
> Some comments below:
>
> On Thu, 25 Mar 2021 at 09:07, piliu <piliu at redhat.com> wrote:
>>
>>
>> In case of making mistake due to my limited knowledge on arm64, I also
>> CC this patch to some guys from arm team. If any comment, please also
>> kindly give them.
>>
>> Thanks,
>> Pingfan
>>
>> On 3/25/21 11:00 AM, Pingfan Liu wrote:
>>> Crash encounters a bug like the following:
>>> ...
>>> License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
>>> This is free software: you are free to change and redistribute it.
>>> There is NO WARRANTY, to the extent permitted by law. Type "show copying"
>>> and "show warranty" for details.
>>> This GDB was configured as "aarch64-unknown-linux-gnu"...
>>>
>>> crash: read error: kernel virtual address: ffff000f789c0050 type: "IRQ stack pointer"
>>> crash: read error: kernel virtual address: ffff000f78a60050 type: "IRQ stack pointer"
>>> crash: read error: kernel virtual address: ffff000f78b00050 type: "IRQ stack pointer"
>>> ...
>>>
>>> This bug connects with kernel commit 7bc1a0f9e176 ("arm64: mm: use
>>> single quantity to represent the PA to VA translation"), memstart_addr
>>> can be negative, which makes it different from real phy_offset.
>
> 7bc1a0f9e176 also did away with the usage of 'physvirt_offset' inside
> the kernel, so we can remove the following function and fix its
> callers in the crash code:
>
I am not sure whether understand your words. But physvirt_offset concept
is required in crash for PTOV().
So in my opinion, this function can hardly been removed.
As for removing kernel_symbol_search("physvirt_offset"), should the
compatibility been taken into account?
> static void
> arm64_calc_physvirt_offset(void)
> {
> struct machine_specific *ms = machdep->machspec;
> ulong physvirt_offset;
> struct syment *sp;
>
> ms->physvirt_offset = ms->memstart_addr - ms->page_offset;
>
> if ((sp = kernel_symbol_search("physvirt_offset")) &&
> machdep->machspec->kimage_voffset) {
> if (READMEM(pc->mfd, &physvirt_offset, sizeof(physvirt_offset),
> sp->value, sp->value -
> machdep->machspec->kimage_voffset) > 0) {
> ms->physvirt_offset = physvirt_offset;
> }
> }
> }
>
>>> In crash utility, PTOV() needs memstart_addr to calculate VA from PA,
>>> while getting PFN offset in a dumpfile, phy_offset is required. So
>>> storing them separately for different purpose.
>>>
>>> Signed-off-by: Pingfan Liu <piliu at redhat.com>
>>> Cc: HAGIO KAZUHITO <k-hagio-ab at nec.com>
>>> Cc: Lianbo Jiang <lijiang at redhat.com>
>>> Cc: Mark Salter <msalter at redhat.com>
>>> Cc: Mark Langsdorf <mlangsdo at redhat.com>
>>> Cc: Jeremy Linton <jlinton at redhat.com>
>>> To: crash-utility at redhat.com
>>> ---
>>> arm64.c | 25 ++++++++++++++++++++++---
>>> defs.h | 1 +
>>> 2 files changed, 23 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arm64.c b/arm64.c
>>> index 37aed07..a0bee62 100644
>>> --- a/arm64.c
>>> +++ b/arm64.c
>>> @@ -24,6 +24,9 @@
>>>
>>> #define NOT_IMPLEMENTED(X) error((X), "%s: function not implemented\n", __func__)
>>>
>>> +#define MEMSTART_ADDR_OFFSET \
>>> + (0xffffffffffffffff << 48 - 0xffffffffffffffff << 56)
>
> Can we use the linux macro directly here:
> `#define _PAGE_OFFSET(va) (-(UL(1) << (va)))` and add a comment
> that it was borrowed `arch/arm64/include/asm/memory.h` instead, so
> that the code is easier to read.
>
I will try.
>>> +
>>> static struct machine_specific arm64_machine_specific = { 0 };
>>> static int arm64_verify_symbol(const char *, ulong, char);
>>> static void arm64_parse_cmdline_args(void);
>>> @@ -687,6 +690,7 @@ arm64_dump_machdep_table(ulong arg)
>>> fprintf(fp, " kimage_voffset: %016lx\n", ms->kimage_voffset);
>>> }
>>> fprintf(fp, " phys_offset: %lx\n", ms->phys_offset);
>>> + fprintf(fp, " memstart_addr: %lx\n", ms->memstart_addr);
>>> fprintf(fp, "__exception_text_start: %lx\n", ms->__exception_text_start);
>>> fprintf(fp, " __exception_text_end: %lx\n", ms->__exception_text_end);
>>> fprintf(fp, " __irqentry_text_start: %lx\n", ms->__irqentry_text_start);
>>> @@ -987,7 +991,7 @@ arm64_calc_physvirt_offset(void)
>>> ulong physvirt_offset;
>>> struct syment *sp;
>>>
>>> - ms->physvirt_offset = ms->phys_offset - ms->page_offset;
>>> + ms->physvirt_offset = ms->memstart_addr - ms->page_offset;
>>>
>>> if ((sp = kernel_symbol_search("physvirt_offset")) &&
>>> machdep->machspec->kimage_voffset) {
>>> @@ -1028,7 +1032,11 @@ arm64_calc_phys_offset(void)
>>> ms->kimage_voffset && (sp = kernel_symbol_search("memstart_addr"))) {
>>> if (pc->flags & PROC_KCORE) {
>>> if ((string = pc->read_vmcoreinfo("NUMBER(PHYS_OFFSET)"))) {
>>> - ms->phys_offset = htol(string, QUIET, NULL);
>>> + ms->memstart_addr = htol(string, QUIET, NULL);
>>> + if (ms->memstart_addr < 0)
>>> + ms->phys_offset = ms->memstart_addr + MEMSTART_ADDR_OFFSET;
>>> + else
>>> + ms->phys_offset = ms->memstart_addr;
>>> free(string);
>>> return;
>>> }
>>> @@ -1080,7 +1088,18 @@ arm64_calc_phys_offset(void)
>>> } else if (DISKDUMP_DUMPFILE() && diskdump_phys_base(&phys_offset)) {
>>> ms->phys_offset = phys_offset;
>>> } else if (KDUMP_DUMPFILE() && arm64_kdump_phys_base(&phys_offset)) {
>>> - ms->phys_offset = phys_offset;
>>> + /*
>>> + * When running a 52bits kernel on 48bits hardware. Kernel plays a trick:
>>> + * if (IS_ENABLED(CONFIG_ARM64_VA_BITS_52) && (vabits_actual != 52))
>>> + * memstart_addr -= _PAGE_OFFSET(48) - _PAGE_OFFSET(52);
>>> + *
>>> + * In crash, this should be detected to get a real physical start address.
>>> + */
>>> + ms->memstart_addr = phys_offset;
>>> + if ((long)phys_offset < 0)
>>> + ms->phys_offset = phys_offset + MEMSTART_ADDR_OFFSET;
>>> + else
>>> + ms->phys_offset = phys_offset;
>
> Do we really need phys_offset and memstart_addr in crash now?
> See the arm64 kernel directly defines it as:
>
> /* PHYS_OFFSET - the physical address of the start of memory. */
> #define PHYS_OFFSET ({ VM_BUG_ON(memstart_addr & 1); memstart_addr; })
>
> So, do we need two separate variables in crash code. If we want the
> code to be symmetrical to the kernel code and we decide to use the two
> seperate variables, we can define PHYS_OFFSET as memstart_addr and use
> a similar naming convention as the kernel code.
>
There are two distinguish usages of memstart_addr: PTOV() and calculate
PFN. If no two separate variables, then at each referred site, it needs
to have an conditional judge 'if (ms->memstart_addr < 0)'. It sacrifices
performance.
What about renaming phys_offset as absolute_memstart_addr? By this way,
it can follow the kernel naming.
Thanks,
Pingfan
More information about the Crash-utility
mailing list