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@xxxxxxxxxx> 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@xxxxxxxxxx>
Cc: HAGIO KAZUHITO <k-hagio-ab@xxxxxxx>
Cc: Lianbo Jiang <lijiang@xxxxxxxxxx>
Cc: Mark Salter <msalter@xxxxxxxxxx>
Cc: Mark Langsdorf <mlangsdo@xxxxxxxxxx>
Cc: Jeremy Linton <jlinton@xxxxxxxxxx>
To: crash-utility@xxxxxxxxxx
---
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
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/crash-utility
