6:34 a.m.
Hi, Aditya
Thank you for the patch.
On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
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 "powerpc64le-unknown-linux-gnu".
Type "show configuration" for configuration details.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>;.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
KERNEL: /home/linux/vmlinux
DUMPFILE: /var/crash/127.0.0.1-2023-09-18-06:58:57/vmcore
CPUS: 8
DATE: Mon Sep 18 06:57:40 EDT 2023
UPTIME: 00:03:19
LOAD AVERAGE: 0.20, 0.31, 0.14
TASKS: 186
NODENAME: ibm-p9z-16-lp10.khw3.lab.eng.bos.redhat.com
RELEASE: 6.6.0-rc2
VERSION: #2 SMP Mon Sep 18 06:11:14 EDT 2023
MACHINE: ppc64le (2800 Mhz)
MEMORY: 16 GB
PANIC: "Kernel panic - not syncing: sysrq triggered crash"
PID: 4337
COMMAND: "bash"
TASK: c000000052a49900 [THREAD_INFO: c000000052a49900]
CPU: 3
STATE: TASK_RUNNING (PANIC)
crash
Thanks
Lianbo
also fails in absence of 'vmemmap_list' in upstream linux
> The fix for makedumpfile will also been sent to upstream
> ---
> ---
> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 33 insertions(+), 14 deletions(-)
> diff --git a/ppc64.c b/ppc64.c
> index fc34006f4863..1e84c5f56773 100644
> --- a/ppc64.c
> +++ b/ppc64.c
> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
> long backing_size, virt_addr_offset, phys_offset, list_offset;
> ulong *vmemmap_list;
> char *vmemmap_buf;
> - struct machine_specific *ms;
> -
> + struct machine_specific *ms = machdep->machspec;
> +
> + ld = &list_data;
> + BZERO(ld, sizeof(struct list_data));
> +
> + /*
> + * vmemmap_list is missing or set to 0 in the kernel would imply
> + * vmemmap region is mapped in the kernel pagetable. So, read
> vmemmap_list
> + * anyway and use the translation method accordingly.
> + */
> + readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
> sizeof(void *),
> + "vmemmap_list", RETURN_ON_ERROR);
> + if (!ld->start) {
> + /*
> + * vmemmap_list is set to 0 or missing. Do kernel
> pagetable walk
> + * for vmemmamp address translation.
> + */
> + ms->vmemmap_list = NULL;
> + ms->vmemmap_cnt = 0;
> +
> + machdep->flags |= VMEMMAP_AWARE;
> + return;
> + }
> +
> if (!(kernel_symbol_exists("vmemmap_list")) ||
> !(kernel_symbol_exists("mmu_psize_defs")) ||
> !(kernel_symbol_exists("mmu_vmemmap_psize")) ||
> @@ -1293,8 +1315,6 @@ void ppc64_vmemmap_init(void)
> !MEMBER_EXISTS("vmemmap_backing", "list"))
> return;
> - ms = machdep->machspec;
> -
> backing_size = STRUCT_SIZE("vmemmap_backing");
> virt_addr_offset = MEMBER_OFFSET("vmemmap_backing",
"virt_addr");
> phys_offset = MEMBER_OFFSET("vmemmap_backing", "phys");
> @@ -1313,14 +1333,8 @@ void ppc64_vmemmap_init(void)
> ms->vmemmap_psize = 1 << shift;
> - ld = &list_data;
> - BZERO(ld, sizeof(struct list_data));
> - if (!readmem(symbol_value("vmemmap_list"),
> - KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
> - RETURN_ON_ERROR))
> - return;
> - ld->end = symbol_value("vmemmap_list");
> - ld->list_head_offset = list_offset;
> + ld->end = symbol_value("vmemmap_list");
> + ld->list_head_offset = list_offset;
> hq_open();
> cnt = do_list(ld);
> @@ -1366,7 +1380,7 @@ ppc64_vmemmap_to_phys(ulong kvaddr, physaddr_t
> *paddr, int verbose)
> {
> int i;
> ulong offset;
> - struct machine_specific *ms;
> + struct machine_specific *ms = machdep->machspec;
> if (!(machdep->flags & VMEMMAP_AWARE)) {
> /*
> @@ -1386,7 +1400,12 @@ ppc64_vmemmap_to_phys(ulong kvaddr, physaddr_t
> *paddr, int verbose)
> return FALSE;
> }
> - ms = machdep->machspec;
> + /**
> + * When vmemmap_list is not populated, kernel does the mapping in
> init_mm
> + * page table, so do a pagetable walk in kernel page table
> + */
> + if (!ms->vmemmap_list)
> + return ppc64_vtop_level4(kvaddr, (ulong
> *)vt->kernel_pgd[0], paddr, verbose);
> for (i = 0; i < ms->vmemmap_cnt; i++) {
> if ((kvaddr >= ms->vmemmap_list[i].virt) &&
> --
> 2.41.0
From: Aditya Gupta <adityag(a)linux.ibm.com To: crash-utility(a)redhat.com
Cc: Hari Bathini <hbathini(a)linux.ibm.com>, Mahesh J Salgaonkar
<mahesh(a)linux.ibm.com>, Sourabh Jain <sourabhjain(a)linux.ibm.com>,
"Aneesh Kumar K.V" <aneesh.kumar(a)linux.ibm.com Subject: [Crash-utility] [PATCH] ppc64: do page traversal if
vmemmap_list not populated
Message-ID: <20230911093032.419388-1-adityag(a)linux.ibm.com Content-Type: text/plain; charset="US-ASCII";
x-default=true
Currently 'crash-tool' fails on vmcore collected on upstream kernel on
PowerPC64 with the error:
crash: invalid kernel virtual address: 0 type: "first list entry
Presently the address translation for vmemmap addresses is done using
the vmemmap_list. But with the below commit in Linux, vmemmap_list can
be empty, in case of Radix MMU on PowerPC64
368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a
different vmemmap handling function)
In case vmemmap_list is empty, then it's head is NULL, which crash tries
to access and fails due to accessing NULL.
Instead of depending on 'vmemmap_list' for address translation for
vmemmap addresses, do a kernel pagetable walk to get the physical
address associated with given virtual address
Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com
---
Testing
=======
Git tree with patch applied:
https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1
This can be tested with '/proc/vmcore' as the vmcore, since makedumpfile
Can you help to describe in detail how to reproduce this issue? Or does
this require any kernel configs to be enabled first? I did not reproduce
the current issue with '/proc/kcore' or vmcore(via cp).
Test kernel commit: ce9ecca0238b ("Linux 6.6-rc2")
# ./crash /home/linux/vmlinux
/var/crash/127.0.0.1-2023-09-18-06\:58\:57/vmcore
crash 8.0.3++
Copyright (C) 2002-2022 Red Hat, Inc.
Copyright (C) 2004, 2005, 2006, 2010 IBM Corporation
Copyright (C) 1999-2006 Hewlett-Packard Co
Copyright (C) 2005, 2006, 2011, 2012 Fujitsu Limited
Copyright (C) 2006, 2007 VA Linux Systems Japan K.K.
Copyright (C) 2005, 2011, 2020-2022 NEC Corporation
Copyright (C) 1999, 2002, 2007 Silicon Graphics, Inc.
Copyright (C) 1999, 2000, 2001, 2002 Mission Critical Linux, Inc.
Copyright (C) 2015, 2021 VMware, Inc.
This program is free software, covered by the GNU General Public License,
and you are welcome to change it and/or distribute copies of it under
certain conditions. Enter "help copying" to see the conditions.
This program has absolutely no warranty. Enter "help warranty" for details.
GNU gdb (GDB) 10.2
Copyright (C) 2021 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html
1:23 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
Hello lijiang,
On Mon, Sep 18, 2023 at 07:34:04PM +0800, lijiang wrote:
Hi, Aditya
Thank you for the patch.
On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
> ...
>
> Currently 'crash-tool' fails on vmcore collected on upstream kernel on
> PowerPC64 with the error:
>
> crash: invalid kernel virtual address: 0 type: "first list entry
>
> Presently the address translation for vmemmap addresses is done using
> the vmemmap_list. But with the below commit in Linux, vmemmap_list can
> be empty, in case of Radix MMU on PowerPC64
>
> 368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a
> different vmemmap handling function)
>
> In case vmemmap_list is empty, then it's head is NULL, which crash tries
> to access and fails due to accessing NULL.
>
> Instead of depending on 'vmemmap_list' for address translation for
> vmemmap addresses, do a kernel pagetable walk to get the physical
> address associated with given virtual address
>
> Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com>
> Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
>
> ---
>
> Testing
> =======
>
> Git tree with patch applied:
> https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1
>
> This can be tested with '/proc/vmcore' as the vmcore, since makedumpfile
>
Can you help to describe in detail how to reproduce this issue? Or does
this require any kernel configs to be enabled first? I did not reproduce
the current issue with '/proc/kcore' or vmcore(via cp).
Test kernel commit: ce9ecca0238b ("Linux 6.6-rc2")
# ./crash /home/linux/vmlinux
Thanks for testing it.
This issue occurs only in case of Radix MMU.
Overall, these are all the requirements:
1. Upstream linux (master branch) (your commit will also work, ce9ecca0238b)
2. 'CONFIG_PPC_BOOK3S_64' should be 'y' in kernel config (this should be
there
in default configs)
3. Check in dmesg of the crashed kernel, if it prints 'hash-mmu' or
'radix-mmu'. It should be 'radix-mmu'.
I guess, the system that was crashed might be using 'hash-mmu'.
also fails in absence of 'vmemmap_list' in upstream linux
Yes, it will fail in Hash MMU case, as we depend on 'vmemmap_list' in that case,
as the virtual to physical address mapping is not available in page table, in
case of Hash-MMU.
Only in radix MMU case, it will still work, even if 'vmemmap_list' is removed,
since we have the mappings in kernel page table, which is used by this patch.
Let me know if the issue still doesn't reproduce even after using a system with
Radix MMU.
Thanks,
- Aditya Gupta
9:21 p.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Tue, Sep 19, 2023 at 2:23 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hello lijiang,
On Mon, Sep 18, 2023 at 07:34:04PM +0800, lijiang wrote:
> Hi, Aditya
> Thank you for the patch.
>
> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com>
wrote:
>
> > ...
> >
> > Currently 'crash-tool' fails on vmcore collected on upstream kernel on
> > PowerPC64 with the error:
> >
> > crash: invalid kernel virtual address: 0 type: "first list entry
> >
> > Presently the address translation for vmemmap addresses is done using
> > the vmemmap_list. But with the below commit in Linux, vmemmap_list can
> > be empty, in case of Radix MMU on PowerPC64
> >
> > 368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a
> > different vmemmap handling function)
> >
> > In case vmemmap_list is empty, then it's head is NULL, which crash
tries
> > to access and fails due to accessing NULL.
> >
> > Instead of depending on 'vmemmap_list' for address translation for
> > vmemmap addresses, do a kernel pagetable walk to get the physical
> > address associated with given virtual address
> >
> > Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com>
> > Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
> >
> > ---
> >
> > Testing
> > =======
> >
> > Git tree with patch applied:
> > https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1
> >
> > This can be tested with '/proc/vmcore' as the vmcore, since
makedumpfile
> >
>
> Can you help to describe in detail how to reproduce this issue? Or does
> this require any kernel configs to be enabled first? I did not reproduce
> the current issue with '/proc/kcore' or vmcore(via cp).
>
> Test kernel commit: ce9ecca0238b ("Linux 6.6-rc2")
>
> # ./crash /home/linux/vmlinux
Thanks for testing it.
This issue occurs only in case of Radix MMU.
Overall, these are all the requirements:
1. Upstream linux (master branch) (your commit will also work,
ce9ecca0238b)
2. 'CONFIG_PPC_BOOK3S_64' should be 'y' in kernel config (this should be
there
in default configs)
# grep "CONFIG_PPC_BOOK3S_64" /home/linux/.config
CONFIG_PPC_BOOK3S_64=y
3. Check in dmesg of the crashed kernel, if it prints 'hash-mmu' or
'radix-mmu'. It should be 'radix-mmu'.
# dmesg|grep mmu
[ 0.000000] hash-mmu: Page sizes from device-tree:
[ 0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000,
avpnm=0x00000000, tlbiel=1, penc=0
[ 0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000,
avpnm=0x00000000, tlbiel=1, penc=7
[ 0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000,
avpnm=0x00000000, tlbiel=1, penc=56
[ 0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110,
avpnm=0x00000000, tlbiel=1, penc=1
[ 0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110,
avpnm=0x00000000, tlbiel=1, penc=8
[ 0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100,
avpnm=0x00000001, tlbiel=0, penc=0
[ 0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120,
avpnm=0x000007ff, tlbiel=0, penc=3
[ 0.000000] hash-mmu: Initializing hash mmu with SLB
[ 0.000000] mmu_features = 0xfc006e01
[ 0.000000] hash-mmu: ppc64_pft_size = 0x1b
[ 0.000000] hash-mmu: htab_hash_mask = 0xfffff
I guess, the system that was crashed might be using
'hash-mmu'.
> also fails in absence of 'vmemmap_list' in upstream linux
Yes, it will fail in Hash MMU case, as we depend on 'vmemmap_list' in that
case,
as the virtual to physical address mapping is not available in page table,
in
case of Hash-MMU.
Only in radix MMU case, it will still work, even if 'vmemmap_list' is
removed,
since we have the mappings in kernel page table, which is used by this
patch.
Let me know if the issue still doesn't reproduce even after using a system
with
Radix MMU.
Yes, still not reproduce on my side. But, looks like we have the same
system with Radix MMU, it's strange.
Thanks.
Lianbo
Thanks,
- Aditya Gupta
6:17 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
Hello lijiang,
On Wed, Sep 20, 2023 at 10:21:18AM +0800, lijiang wrote:
On Tue, Sep 19, 2023 at 2:23 PM Aditya Gupta
<adityag(a)linux.ibm.com> wrote:
> Hello lijiang,
>
> On Mon, Sep 18, 2023 at 07:34:04PM +0800, lijiang wrote:
> > Hi, Aditya
> > Thank you for the patch.
> >
> > ...
> >
> > Test kernel commit: ce9ecca0238b ("Linux 6.6-rc2")
> >
> > # ./crash /home/linux/vmlinux
>
> Thanks for testing it.
>
> This issue occurs only in case of Radix MMU.
>
> Overall, these are all the requirements:
> 1. Upstream linux (master branch) (your commit will also work,
> ce9ecca0238b)
> 2. 'CONFIG_PPC_BOOK3S_64' should be 'y' in kernel config (this
should be
> there
> in default configs)
>
# grep "CONFIG_PPC_BOOK3S_64" /home/linux/.config
CONFIG_PPC_BOOK3S_64=y
3. Check in dmesg of the crashed kernel, if it prints 'hash-mmu' or
> 'radix-mmu'. It should be 'radix-mmu'.
>
>
# dmesg|grep mmu
[ 0.000000] hash-mmu: Page sizes from device-tree:
[ 0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000,
avpnm=0x00000000, tlbiel=1, penc=0
[ 0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000,
avpnm=0x00000000, tlbiel=1, penc=7
[ 0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000,
avpnm=0x00000000, tlbiel=1, penc=56
[ 0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110,
avpnm=0x00000000, tlbiel=1, penc=1
[ 0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110,
avpnm=0x00000000, tlbiel=1, penc=8
[ 0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100,
avpnm=0x00000001, tlbiel=0, penc=0
[ 0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120,
avpnm=0x000007ff, tlbiel=0, penc=3
[ 0.000000] hash-mmu: Initializing hash mmu with SLB
[ 0.000000] mmu_features = 0xfc006e01
[ 0.000000] hash-mmu: ppc64_pft_size = 0x1b
[ 0.000000] hash-mmu: htab_hash_mask = 0xfffff
This seems to using Hash MMU, hence the error doesn't come up.
Since vmemmap_list is NOT empty in case of Hash MMU, so crash works as expected.
Can you try it on a system with Radix MMU ? (Rainier/Denali systems might have
that by default)
The 'dmesg | grep mmu' you did is a good way to check if the system is using
'radix-mmu'.
> I guess, the system that was crashed might be using 'hash-mmu'.
>
> > also fails in absence of 'vmemmap_list' in upstream linux
>
> Yes, it will fail in Hash MMU case, as we depend on 'vmemmap_list' in that
> case,
> as the virtual to physical address mapping is not available in page table,
> in
> case of Hash-MMU.
>
> Only in radix MMU case, it will still work, even if 'vmemmap_list' is
> removed,
> since we have the mappings in kernel page table, which is used by this
> patch.
>
> Let me know if the issue still doesn't reproduce even after using a system
> with
> Radix MMU.
>
>
Yes, still not reproduce on my side. But, looks like we have the same
system with Radix MMU, it's strange.
Actually I meant the current MMU should be Radix MMU, according to the above
system logs, the system is using Hash MMU.
On a system with current MMU as Radix MMU, the error should occur. Since with
the below commit in upstream kernel:
368a0590d954 ("powerpc/book3s64/vmemmap: switch radix to use a different vmemmap
handling function")
the way address mapping was stored for vmemmap has changed, for Radix MMU.
In case of Radix MMU, now we have the vmemmap address mapping in kernel page
tables only. Hence 'vmemmap_list' is empty.
In case of Hash MMU, vmemmap address mapping is still stored in
'vmemmap_list', which crash uses, hence the error will not occur.
Also, due to this reason, if we crash a system which is using Hash MMU,
kernel still populates it in 'vmemmap_list' so we need that symbol.
While, in Radix MMU, even if the symbol is there or is missing, crash will
still work after this patch.
Thanks
- Aditya Gupta
10:36 p.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Wed, Sep 20, 2023 at 7:17 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hello lijiang,
On Wed, Sep 20, 2023 at 10:21:18AM +0800, lijiang wrote:
> On Tue, Sep 19, 2023 at 2:23 PM Aditya Gupta <adityag(a)linux.ibm.com>
wrote:
>
> > Hello lijiang,
> >
> > On Mon, Sep 18, 2023 at 07:34:04PM +0800, lijiang wrote:
> > > Hi, Aditya
> > > Thank you for the patch.
> > >
> > > ...
> > >
> > > Test kernel commit: ce9ecca0238b ("Linux 6.6-rc2")
> > >
> > > # ./crash /home/linux/vmlinux
> >
> > Thanks for testing it.
> >
> > This issue occurs only in case of Radix MMU.
> >
> > Overall, these are all the requirements:
> > 1. Upstream linux (master branch) (your commit will also work,
> > ce9ecca0238b)
> > 2. 'CONFIG_PPC_BOOK3S_64' should be 'y' in kernel config (this
should
be
> > there
> > in default configs)
> >
>
> # grep "CONFIG_PPC_BOOK3S_64" /home/linux/.config
> CONFIG_PPC_BOOK3S_64=y
>
> 3. Check in dmesg of the crashed kernel, if it prints 'hash-mmu' or
> > 'radix-mmu'. It should be 'radix-mmu'.
> >
> >
> # dmesg|grep mmu
> [ 0.000000] hash-mmu: Page sizes from device-tree:
> [ 0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000,
> avpnm=0x00000000, tlbiel=1, penc=0
> [ 0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000,
> avpnm=0x00000000, tlbiel=1, penc=7
> [ 0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000,
> avpnm=0x00000000, tlbiel=1, penc=56
> [ 0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110,
> avpnm=0x00000000, tlbiel=1, penc=1
> [ 0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110,
> avpnm=0x00000000, tlbiel=1, penc=8
> [ 0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100,
> avpnm=0x00000001, tlbiel=0, penc=0
> [ 0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120,
> avpnm=0x000007ff, tlbiel=0, penc=3
> [ 0.000000] hash-mmu: Initializing hash mmu with SLB
> [ 0.000000] mmu_features = 0xfc006e01
> [ 0.000000] hash-mmu: ppc64_pft_size = 0x1b
> [ 0.000000] hash-mmu: htab_hash_mask = 0xfffff
>
This seems to using Hash MMU, hence the error doesn't come up.
Since vmemmap_list is NOT empty in case of Hash MMU, so crash works as
expected.
Thank you for the explanation, Aditya.
Can you try it on a system with Radix MMU ? (Rainier/Denali systems
might
have
that by default)
I'm afraid I don't have such machines with Radix MMU. Looks like I can not
test it, let's reback to the discussion of patches.
The 'dmesg | grep mmu' you did is a good way to check if the system is using
'radix-mmu'.
>
> > I guess, the system that was crashed might be using 'hash-mmu'.
> >
> > > also fails in absence of 'vmemmap_list' in upstream linux
> >
> > Yes, it will fail in Hash MMU case, as we depend on 'vmemmap_list' in
that
> > case,
> > as the virtual to physical address mapping is not available in page
table,
> > in
> > case of Hash-MMU.
> >
> > Only in radix MMU case, it will still work, even if 'vmemmap_list' is
> > removed,
> > since we have the mappings in kernel page table, which is used by this
> > patch.
> >
> > Let me know if the issue still doesn't reproduce even after using a
system
> > with
> > Radix MMU.
> >
> >
> Yes, still not reproduce on my side. But, looks like we have the same
> system with Radix MMU, it's strange.
Actually I meant the current MMU should be Radix MMU, according to the
above
system logs, the system is using Hash MMU.
On a system with current MMU as Radix MMU, the error should occur. Since
with
the below commit in upstream kernel:
368a0590d954 ("powerpc/book3s64/vmemmap: switch radix to use a
different vmemmap
handling function")
the way address mapping was stored for vmemmap has changed, for Radix MMU.
In case of Radix MMU, now we have the vmemmap address mapping in kernel
page
tables only. Hence 'vmemmap_list' is empty.
In case of Hash MMU, vmemmap address mapping is still stored in
'vmemmap_list', which crash uses, hence the error will not occur.
Also, due to this reason, if we crash a system which is using Hash MMU,
kernel still populates it in 'vmemmap_list' so we need that symbol.
While, in Radix MMU, even if the symbol is there or is missing, crash will
still work after this patch.
OK, got it. Thank you for explaining the details.
Thanks.
Lianbo
Thanks
- Aditya Gupta
2:01 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Thu, Sep 21, 2023 at 11:36:10AM +0800, lijiang wrote:
On Wed, Sep 20, 2023 at 7:17 PM Aditya Gupta
<adityag(a)linux.ibm.com> wrote:
> Hello lijiang,
>
> On Wed, Sep 20, 2023 at 10:21:18AM +0800, lijiang wrote:
> > On Tue, Sep 19, 2023 at 2:23 PM Aditya Gupta <adityag(a)linux.ibm.com>
> wrote:
> >
> > > Hello lijiang,
> > >
> > > On Mon, Sep 18, 2023 at 07:34:04PM +0800, lijiang wrote:
> > > > Hi, Aditya
> > > > Thank you for the patch.
> > > >
> > > > ...
> > > >
> > > > Test kernel commit: ce9ecca0238b ("Linux 6.6-rc2")
> > > >
> > > > # ./crash /home/linux/vmlinux
> > >
> > > Thanks for testing it.
> > >
> > > This issue occurs only in case of Radix MMU.
> > >
> > > Overall, these are all the requirements:
> > > 1. Upstream linux (master branch) (your commit will also work,
> > > ce9ecca0238b)
> > > 2. 'CONFIG_PPC_BOOK3S_64' should be 'y' in kernel config
(this should
> be
> > > there
> > > in default configs)
> > >
> >
> > # grep "CONFIG_PPC_BOOK3S_64" /home/linux/.config
> > CONFIG_PPC_BOOK3S_64=y
> >
> > 3. Check in dmesg of the crashed kernel, if it prints 'hash-mmu' or
> > > 'radix-mmu'. It should be 'radix-mmu'.
> > >
> > >
> > # dmesg|grep mmu
> > [ 0.000000] hash-mmu: Page sizes from device-tree:
> > [ 0.000000] hash-mmu: base_shift=12: shift=12, sllp=0x0000,
> > avpnm=0x00000000, tlbiel=1, penc=0
> > [ 0.000000] hash-mmu: base_shift=12: shift=16, sllp=0x0000,
> > avpnm=0x00000000, tlbiel=1, penc=7
> > [ 0.000000] hash-mmu: base_shift=12: shift=24, sllp=0x0000,
> > avpnm=0x00000000, tlbiel=1, penc=56
> > [ 0.000000] hash-mmu: base_shift=16: shift=16, sllp=0x0110,
> > avpnm=0x00000000, tlbiel=1, penc=1
> > [ 0.000000] hash-mmu: base_shift=16: shift=24, sllp=0x0110,
> > avpnm=0x00000000, tlbiel=1, penc=8
> > [ 0.000000] hash-mmu: base_shift=24: shift=24, sllp=0x0100,
> > avpnm=0x00000001, tlbiel=0, penc=0
> > [ 0.000000] hash-mmu: base_shift=34: shift=34, sllp=0x0120,
> > avpnm=0x000007ff, tlbiel=0, penc=3
> > [ 0.000000] hash-mmu: Initializing hash mmu with SLB
> > [ 0.000000] mmu_features = 0xfc006e01
> > [ 0.000000] hash-mmu: ppc64_pft_size = 0x1b
> > [ 0.000000] hash-mmu: htab_hash_mask = 0xfffff
> >
>
> This seems to using Hash MMU, hence the error doesn't come up.
> Since vmemmap_list is NOT empty in case of Hash MMU, so crash works as
> expected.
>
>
Thank you for the explanation, Aditya.
> Can you try it on a system with Radix MMU ? (Rainier/Denali systems might
> have
> that by default)
>
>
I'm afraid I don't have such machines with Radix MMU. Looks like I can not
test it, let's reback to the discussion of patches.
Sure, thanks for trying.
Thanks
- Aditya Gupta
12:51 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
Hi, Aditya
Thank you for the patch.
On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
> From: Aditya Gupta <adityag(a)linux.ibm.com>
> To: crash-utility(a)redhat.com
> Cc: Hari Bathini <hbathini(a)linux.ibm.com>, Mahesh J Salgaonkar
> <mahesh(a)linux.ibm.com>, Sourabh Jain
<sourabhjain(a)linux.ibm.com>,
> "Aneesh Kumar K.V" <aneesh.kumar(a)linux.ibm.com>
> Subject: [Crash-utility] [PATCH] ppc64: do page traversal if
> vmemmap_list not populated
> Message-ID: <20230911093032.419388-1-adityag(a)linux.ibm.com>
> Content-Type: text/plain; charset="US-ASCII"; x-default=true
>
> Currently 'crash-tool' fails on vmcore collected on upstream kernel on
> PowerPC64 with the error:
>
> crash: invalid kernel virtual address: 0 type: "first list entry
>
> Presently the address translation for vmemmap addresses is done using
> the vmemmap_list. But with the below commit in Linux, vmemmap_list can
> be empty, in case of Radix MMU on PowerPC64
>
> 368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a
> different vmemmap handling function)
>
> In case vmemmap_list is empty, then it's head is NULL, which crash tries
> to access and fails due to accessing NULL.
>
> Instead of depending on 'vmemmap_list' for address translation for
> vmemmap addresses, do a kernel pagetable walk to get the physical
> address associated with given virtual address
>
> Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com>
> Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
>
> ---
>
> Testing
> =======
>
> Git tree with patch applied:
> https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1
>
> This can be tested with '/proc/vmcore' as the vmcore, since makedumpfile
also fails in absence of 'vmemmap_list' in upstream linux
>
> The fix for makedumpfile will also been sent to upstream
>
> ---
> ---
> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
> 1 file changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/ppc64.c b/ppc64.c
> index fc34006f4863..1e84c5f56773 100644
> --- a/ppc64.c
> +++ b/ppc64.c
> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
> long backing_size, virt_addr_offset, phys_offset, list_offset;
> ulong *vmemmap_list;
> char *vmemmap_buf;
> - struct machine_specific *ms;
> -
> + struct machine_specific *ms = machdep->machspec;
> +
> + ld = &list_data;
> + BZERO(ld, sizeof(struct list_data));
> +
> + /*
> + * vmemmap_list is missing or set to 0 in the kernel would imply
> + * vmemmap region is mapped in the kernel pagetable. So, read
> vmemmap_list
> + * anyway and use the translation method accordingly.
> + */
> + readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
> sizeof(void *),
> + "vmemmap_list", RETURN_ON_ERROR);
> + if (!ld->start) {
> + /*
> + * vmemmap_list is set to 0 or missing. Do kernel
> pagetable walk
> + * for vmemmamp address translation.
> + */
> + ms->vmemmap_list = NULL;
> + ms->vmemmap_cnt = 0;
> +
> + machdep->flags |= VMEMMAP_AWARE;
> + return;
> + }
> +
>
I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
after the 'kernel_symbol_exists("vmemmap_list")', and also check the
returned value of readmem().
And other changes are fine to me.
Thanks
Lianbo
if (!(kernel_symbol_exists("vmemmap_list")) ||
> !(kernel_symbol_exists("mmu_psize_defs"))
||
> !(kernel_symbol_exists("mmu_vmemmap_psize")) ||
> @@ -1293,8 +1315,6 @@ void ppc64_vmemmap_init(void)
> !MEMBER_EXISTS("vmemmap_backing", "list"))
> return;
>
> - ms = machdep->machspec;
> -
> backing_size = STRUCT_SIZE("vmemmap_backing");
> virt_addr_offset = MEMBER_OFFSET("vmemmap_backing",
"virt_addr");
> phys_offset = MEMBER_OFFSET("vmemmap_backing", "phys");
> @@ -1313,14 +1333,8 @@ void ppc64_vmemmap_init(void)
>
> ms->vmemmap_psize = 1 << shift;
>
> - ld = &list_data;
> - BZERO(ld, sizeof(struct list_data));
> - if (!readmem(symbol_value("vmemmap_list"),
> - KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
> - RETURN_ON_ERROR))
> - return;
> - ld->end = symbol_value("vmemmap_list");
> - ld->list_head_offset = list_offset;
> + ld->end = symbol_value("vmemmap_list");
> + ld->list_head_offset = list_offset;
>
> hq_open();
> cnt = do_list(ld);
> @@ -1366,7 +1380,7 @@ ppc64_vmemmap_to_phys(ulong kvaddr, physaddr_t
> *paddr, int verbose)
> {
> int i;
> ulong offset;
> - struct machine_specific *ms;
> + struct machine_specific *ms = machdep->machspec;
>
> if (!(machdep->flags & VMEMMAP_AWARE)) {
> /*
> @@ -1386,7 +1400,12 @@ ppc64_vmemmap_to_phys(ulong kvaddr, physaddr_t
> *paddr, int verbose)
> return FALSE;
> }
>
> - ms = machdep->machspec;
> + /**
> + * When vmemmap_list is not populated, kernel does the mapping in
> init_mm
> + * page table, so do a pagetable walk in kernel page table
> + */
> + if (!ms->vmemmap_list)
> + return ppc64_vtop_level4(kvaddr, (ulong
> *)vt->kernel_pgd[0], paddr, verbose);
>
> for (i = 0; i < ms->vmemmap_cnt; i++) {
> if ((kvaddr >= ms->vmemmap_list[i].virt) &&
> --
> 2.41.0
>
2:30 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On 21/09/23 11:21, lijiang wrote:
On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang(a) redhat. com>
wrote:
Hi, Aditya Thank you for the patch. On Mon, Sep 11, 2023 at 8: 00 PM
<crash-utility-request(a) redhat. com> wrote: From: Aditya Gupta
<adityag(a) linux. ibm. com> To:
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
Report Suspicious
<https://us-phishalarm-ewt.proofpoint.com/EWT/v1/PjiDSg!12-vrJGz6dTTkKy6di...
ZjQcmQRYFpfptBannerEnd
On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
Hi, Aditya
Thank you for the patch.
On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com>
wrote:
From: Aditya Gupta <adityag(a)linux.ibm.com>
To: crash-utility(a)redhat.com
Cc: Hari Bathini <hbathini(a)linux.ibm.com>, Mahesh J Salgaonkar
<mahesh(a)linux.ibm.com>, Sourabh Jain
<sourabhjain(a)linux.ibm.com>,
"Aneesh Kumar K.V" <aneesh.kumar(a)linux.ibm.com>
Subject: [Crash-utility] [PATCH] ppc64: do page traversal if
vmemmap_list not populated
Message-ID: <20230911093032.419388-1-adityag(a)linux.ibm.com>
Content-Type: text/plain; charset="US-ASCII"; x-default=true
Currently 'crash-tool' fails on vmcore collected on upstream
kernel on
PowerPC64 with the error:
crash: invalid kernel virtual address: 0 type: "first
list entry
Presently the address translation for vmemmap addresses is
done using
the vmemmap_list. But with the below commit in Linux,
vmemmap_list can
be empty, in case of Radix MMU on PowerPC64
368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a
different vmemmap handling function)
In case vmemmap_list is empty, then it's head is NULL, which
crash tries
to access and fails due to accessing NULL.
Instead of depending on 'vmemmap_list' for address translation for
vmemmap addresses, do a kernel pagetable walk to get the physical
address associated with given virtual address
Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com>
Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
---
Testing
=======
Git tree with patch applied:
https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1
This can be tested with '/proc/vmcore' as the vmcore, since
makedumpfile
also fails in absence of 'vmemmap_list' in upstream linux
The fix for makedumpfile will also been sent to upstream
---
---
ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
1 file changed, 33 insertions(+), 14 deletions(-)
diff --git a/ppc64.c b/ppc64.c
index fc34006f4863..1e84c5f56773 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
long backing_size, virt_addr_offset, phys_offset,
list_offset;
ulong *vmemmap_list;
char *vmemmap_buf;
- struct machine_specific *ms;
-
+ struct machine_specific *ms = machdep->machspec;
+
+ ld = &list_data;
+ BZERO(ld, sizeof(struct list_data));
+
+ /*
+ * vmemmap_list is missing or set to 0 in the kernel
would imply
+ * vmemmap region is mapped in the kernel pagetable.
So, read vmemmap_list
+ * anyway and use the translation method accordingly.
+ */
+ readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start, sizeof(void *),
+ "vmemmap_list", RETURN_ON_ERROR);
+ if (!ld->start) {
+ /*
+ * vmemmap_list is set to 0 or missing. Do
kernel pagetable walk
+ * for vmemmamp address translation.
+ */
+ ms->vmemmap_list = NULL;
+ ms->vmemmap_cnt = 0;
+
+ machdep->flags |= VMEMMAP_AWARE;
+ return;
+ }
+
I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)' after the
'kernel_symbol_exists("vmemmap_list")', and also check the returned
value of readmem().
Yes, I considered that, but the if condition checking for
'kernel_symbol_exists("vmemmap_list")' returns from the function if the
symbol is not found, same with the readmem return value check earlier,
we wanted to initialise the ms->vmemmap_list and machdep->flags before that.
The readmem used to be like this, which returns from function if readmem
failed:
- if (!readmem(symbol_value("vmemmap_list"),
- KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
- RETURN_ON_ERROR))
- return;
So, intentionally I put it above the if and removed the return value
check, since we don't want to depend on vmemmap_list symbol being there
for newer kernels anymore.
And to be future proof in case the address mapping moves to page table
for Hash MMU case also, and since vmemmap_list is PowerPC specific, the
symbol might also be removed.
But, if it's a coding guidelines requirement to handle the return
values, I can think of how to handle the return value of readmem, any
suggestions ?
And other changes are fine to me.
Thank you for your reviews Lianbo :)
Thanks,
- Aditya Gupta
4:46 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
On 21/09/23 11:21, lijiang wrote:
On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com>
<lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote: Hi, Aditya Thank you for
the patch. On Mon, Sep 11, 2023 at 8: 00 PM <crash-utility-request@
redhat. com> <crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote:
From: Aditya Gupta <adityag@ linux. ibm. com>
<adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
> Hi, Aditya
> Thank you for the patch.
>
> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
>
>> From: Aditya Gupta <adityag(a)linux.ibm.com>
>> To: crash-utility(a)redhat.com
>> Cc: Hari Bathini <hbathini(a)linux.ibm.com>, Mahesh J Salgaonkar
>> <mahesh(a)linux.ibm.com>, Sourabh Jain <sourabhjain(a)linux.ibm.com
>> >,
>> "Aneesh Kumar K.V" <aneesh.kumar(a)linux.ibm.com>
>> Subject: [Crash-utility] [PATCH] ppc64: do page traversal if
>> vmemmap_list not populated
>> Message-ID: <20230911093032.419388-1-adityag(a)linux.ibm.com>
>> Content-Type: text/plain; charset="US-ASCII"; x-default=true
>>
>> Currently 'crash-tool' fails on vmcore collected on upstream kernel on
>> PowerPC64 with the error:
>>
>> crash: invalid kernel virtual address: 0 type: "first list entry
>>
>> Presently the address translation for vmemmap addresses is done using
>> the vmemmap_list. But with the below commit in Linux, vmemmap_list can
>> be empty, in case of Radix MMU on PowerPC64
>>
>> 368a0590d954: (powerpc/book3s64/vmemmap: switch radix to use a
>> different vmemmap handling function)
>>
>> In case vmemmap_list is empty, then it's head is NULL, which crash tries
>> to access and fails due to accessing NULL.
>>
>> Instead of depending on 'vmemmap_list' for address translation for
>> vmemmap addresses, do a kernel pagetable walk to get the physical
>> address associated with given virtual address
>>
>> Reviewed-by: Hari Bathini <hbathini(a)linux.ibm.com>
>> Signed-off-by: Aditya Gupta <adityag(a)linux.ibm.com>
>>
>> ---
>>
>> Testing
>> =======
>>
>> Git tree with patch applied:
>> https://github.com/adi-g15-ibm/crash/tree/bugzilla-203296-list-v1
>>
>> This can be tested with '/proc/vmcore' as the vmcore, since makedumpfile
>
> also fails in absence of 'vmemmap_list' in upstream linux
>>
>> The fix for makedumpfile will also been sent to upstream
>>
>> ---
>> ---
>> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/ppc64.c b/ppc64.c
>> index fc34006f4863..1e84c5f56773 100644
>> --- a/ppc64.c
>> +++ b/ppc64.c
>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
>> long backing_size, virt_addr_offset, phys_offset, list_offset;
>> ulong *vmemmap_list;
>> char *vmemmap_buf;
>> - struct machine_specific *ms;
>> -
>> + struct machine_specific *ms = machdep->machspec;
>> +
>> + ld = &list_data;
>> + BZERO(ld, sizeof(struct list_data));
>> +
>> + /*
>> + * vmemmap_list is missing or set to 0 in the kernel would imply
>> + * vmemmap region is mapped in the kernel pagetable. So, read
>> vmemmap_list
>> + * anyway and use the translation method accordingly.
>> + */
>> + readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
>> sizeof(void *),
>> + "vmemmap_list", RETURN_ON_ERROR);
>> + if (!ld->start) {
>> + /*
>> + * vmemmap_list is set to 0 or missing. Do kernel
>> pagetable walk
>> + * for vmemmamp address translation.
>> + */
>> + ms->vmemmap_list = NULL;
>> + ms->vmemmap_cnt = 0;
>> +
>> + machdep->flags |= VMEMMAP_AWARE;
>> + return;
>> + }
>> +
>>
>
I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
after the 'kernel_symbol_exists("vmemmap_list")', and also check the
returned value of readmem().
Yes, I considered that, but the if condition checking for
'kernel_symbol_exists("vmemmap_list")' returns from the function if
the
symbol is not found, same with the readmem return value check earlier, we
wanted to initialise the ms->vmemmap_list and machdep->flags before that.
It's true.
For the current patch, if the symbol 'vmemmap_list' is not found, the
symbol_value() will invoke the error(FATAL, ...) and ultimately exit from
this function. It doesn't have any chance to execute the initialising code.
And, the initialization code relies on the result of ld->start, once the
readmem() fails due to some reasons, it may also enter the
initialization code. I'm not sure if that is expected behavior(if yes, the
"RETURN_ON_ERROR|QUIET " would be good).
If so, the kernel_symbol_exists("vmemmap_list") may be redundant. That can
be removed this time. What do you think?
The readmem used to be like this, which returns from function if readmem
failed:
- if (!readmem(symbol_value("vmemmap_list"),
- KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
- RETURN_ON_ERROR))
- return;
So, intentionally I put it above the if and removed the return value
check, since we don't want to depend on vmemmap_list symbol being there for
newer kernels anymore.
And to be future proof in case the address mapping moves to page table for
Hash MMU case also, and since vmemmap_list is PowerPC specific, the symbol
might also be removed.
Ok, got it, thanks for the information.
But, if it's a coding guidelines requirement to handle the return
values,
I can think of how to handle the return value of readmem, any suggestions ?
No, it's not a coding guidelines requirement. :-)
And other changes are fine to me.
Thank you for your reviews Lianbo :)
No problem.
Thanks.
Lianbo
Thanks,
- Aditya Gupta
6:19 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Thu, Sep 21, 2023 at 05:46:32PM +0800, lijiang wrote:
On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta
<adityag(a)linux.ibm.com> wrote:
> On 21/09/23 11:21, lijiang wrote:
>
> On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com>
> <lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote: Hi, Aditya Thank you for
> the patch. On Mon, Sep 11, 2023 at 8: 00 PM <crash-utility-request@
> redhat. com> <crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote:
> From: Aditya Gupta <adityag@ linux. ibm. com>
> <adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
> On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
>
>> Hi, Aditya
>> Thank you for the patch.
>>
>> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
>>
>>
>> ...
>>
>>> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
>>> 1 file changed, 33 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/ppc64.c b/ppc64.c
>>> index fc34006f4863..1e84c5f56773 100644
>>> --- a/ppc64.c
>>> +++ b/ppc64.c
>>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
>>> long backing_size, virt_addr_offset, phys_offset, list_offset;
>>> ulong *vmemmap_list;
>>> char *vmemmap_buf;
>>> - struct machine_specific *ms;
>>> -
>>> + struct machine_specific *ms = machdep->machspec;
>>> +
>>> + ld = &list_data;
>>> + BZERO(ld, sizeof(struct list_data));
>>> +
>>> + /*
>>> + * vmemmap_list is missing or set to 0 in the kernel would imply
>>> + * vmemmap region is mapped in the kernel pagetable. So, read
>>> vmemmap_list
>>> + * anyway and use the translation method accordingly.
>>> + */
>>> + readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
>>> sizeof(void *),
>>> + "vmemmap_list", RETURN_ON_ERROR);
>>> + if (!ld->start) {
>>> + /*
>>> + * vmemmap_list is set to 0 or missing. Do kernel
>>> pagetable walk
>>> + * for vmemmamp address translation.
>>> + */
>>> + ms->vmemmap_list = NULL;
>>> + ms->vmemmap_cnt = 0;
>>> +
>>> + machdep->flags |= VMEMMAP_AWARE;
>>> + return;
>>> + }
>>> +
>>>
>>
> I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
> after the 'kernel_symbol_exists("vmemmap_list")', and also check
the
> returned value of readmem().
>
> Yes, I considered that, but the if condition checking for
> 'kernel_symbol_exists("vmemmap_list")' returns from the function
if the
> symbol is not found, same with the readmem return value check earlier, we
> wanted to initialise the ms->vmemmap_list and machdep->flags before that.
>
>
It's true.
For the current patch, if the symbol 'vmemmap_list' is not found, the
symbol_value() will invoke the error(FATAL, ...) and ultimately exit from
this function. It doesn't have any chance to execute the initialising code.
Got it, this would be an issue when 'vmemmap_list' is missing.
Made a mistake here, Thanks, will add an if around the readmem, that should
solve it, like this ?
if (kernel_symbol_exists("vmemmap_list"))
readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
sizeof(void *),
"vmemmap_list", RETURN_ON_ERROR | QUIET);
Will fix this in V2.
And, the initialization code relies on the result of ld->start,
once the
readmem() fails due to some reasons, it may also enter the
initialization code. I'm not sure if that is expected behavior(if yes, the
"RETURN_ON_ERROR|QUIET " would be good).
Okay, even if readmem fails, we still go ahead and since ld was all zeros,
'if (!ld->start)' should remain true, for both cases when vmemmap_list is
empty
(head is NULL, hence ld->start=0), vmemmap_list symbol is not found or readmem
fails (ld->start=0 since ld was all zeros due to BZERO).
Will add "RETURN_ON_ERROR|QUIET" also in V2.
If so, the kernel_symbol_exists("vmemmap_list") may be redundant. That can
be removed this time. What do you think?
True. Since the case of 'vmemmap_list' being empty, or symbol not being there,
both get handled before this, that can be removed.
Will do it in V2.
Thanks,
- Aditya Gupta
> The readmem used to be like this, which returns from function if
readmem
> failed:
>
> - if (!readmem(symbol_value("vmemmap_list"),
> - KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
> - RETURN_ON_ERROR))
> - return;
>
> So, intentionally I put it above the if and removed the return value
> check, since we don't want to depend on vmemmap_list symbol being there for
> newer kernels anymore.
> And to be future proof in case the address mapping moves to page table for
> Hash MMU case also, and since vmemmap_list is PowerPC specific, the symbol
> might also be removed.
>
>
Ok, got it, thanks for the information.
> But, if it's a coding guidelines requirement to handle the return values,
> I can think of how to handle the return value of readmem, any suggestions ?
>
>
No, it's not a coding guidelines requirement. :-)
>
> And other changes are fine to me.
>
> Thank you for your reviews Lianbo :)
>
>
No problem.
Thanks.
Lianbo
> Thanks,
> - Aditya Gupta
>
6:26 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
Hi Lianbo,
Resending this, since skipped your mail from "To:" field by mistake.
On Thu, Sep 21, 2023 at 05:46:32PM +0800, lijiang wrote:
On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta
<adityag(a)linux.ibm.com> wrote:
> On 21/09/23 11:21, lijiang wrote:
>
> On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com>
> <lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote: Hi, Aditya Thank you for
> the patch. On Mon, Sep 11, 2023 at 8: 00 PM <crash-utility-request@
> redhat. com> <crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote:
> From: Aditya Gupta <adityag@ linux. ibm. com>
> <adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
> On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
>
>> Hi, Aditya
>> Thank you for the patch.
>>
>> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com> wrote:
>>
>>
>> ...
>>
>>> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
>>> 1 file changed, 33 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/ppc64.c b/ppc64.c
>>> index fc34006f4863..1e84c5f56773 100644
>>> --- a/ppc64.c
>>> +++ b/ppc64.c
>>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
>>> long backing_size, virt_addr_offset, phys_offset, list_offset;
>>> ulong *vmemmap_list;
>>> char *vmemmap_buf;
>>> - struct machine_specific *ms;
>>> -
>>> + struct machine_specific *ms = machdep->machspec;
>>> +
>>> + ld = &list_data;
>>> + BZERO(ld, sizeof(struct list_data));
>>> +
>>> + /*
>>> + * vmemmap_list is missing or set to 0 in the kernel would imply
>>> + * vmemmap region is mapped in the kernel pagetable. So, read
>>> vmemmap_list
>>> + * anyway and use the translation method accordingly.
>>> + */
>>> + readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
>>> sizeof(void *),
>>> + "vmemmap_list", RETURN_ON_ERROR);
>>> + if (!ld->start) {
>>> + /*
>>> + * vmemmap_list is set to 0 or missing. Do kernel
>>> pagetable walk
>>> + * for vmemmamp address translation.
>>> + */
>>> + ms->vmemmap_list = NULL;
>>> + ms->vmemmap_cnt = 0;
>>> +
>>> + machdep->flags |= VMEMMAP_AWARE;
>>> + return;
>>> + }
>>> +
>>>
>>
> I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
> after the 'kernel_symbol_exists("vmemmap_list")', and also check
the
> returned value of readmem().
>
> Yes, I considered that, but the if condition checking for
> 'kernel_symbol_exists("vmemmap_list")' returns from the function
if the
> symbol is not found, same with the readmem return value check earlier, we
> wanted to initialise the ms->vmemmap_list and machdep->flags before that.
>
>
It's true.
For the current patch, if the symbol 'vmemmap_list' is not found, the
symbol_value() will invoke the error(FATAL, ...) and ultimately exit from
this function. It doesn't have any chance to execute the initialising code.
Got it, this would be an issue when 'vmemmap_list' is missing.
Made a mistake here, Thanks, will add an if around the readmem, that should
solve it, like this ?
if (kernel_symbol_exists("vmemmap_list"))
readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
sizeof(void *),
"vmemmap_list", RETURN_ON_ERROR | QUIET);
Will fix this in V2.
And, the initialization code relies on the result of ld->start,
once the
readmem() fails due to some reasons, it may also enter the
initialization code. I'm not sure if that is expected behavior(if yes, the
"RETURN_ON_ERROR|QUIET " would be good).
Okay, even if readmem fails, we still go ahead and since ld was all zeros,
'if (!ld->start)' should remain true, for both cases when vmemmap_list is
empty
(head is NULL, hence ld->start=0), vmemmap_list symbol is not found or readmem
fails (ld->start=0 since ld was all zeros due to BZERO).
Will add "RETURN_ON_ERROR|QUIET" also in V2.
If so, the kernel_symbol_exists("vmemmap_list") may be redundant. That can
be removed this time. What do you think?
True. Since the case of 'vmemmap_list' being empty, or symbol not being there,
both get handled before this, that can be removed.
Will do it in V2.
Thanks,
- Aditya Gupta
> The readmem used to be like this, which returns from function if
readmem
> failed:
>
> - if (!readmem(symbol_value("vmemmap_list"),
> - KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
> - RETURN_ON_ERROR))
> - return;
>
> So, intentionally I put it above the if and removed the return value
> check, since we don't want to depend on vmemmap_list symbol being there for
> newer kernels anymore.
> And to be future proof in case the address mapping moves to page table for
> Hash MMU case also, and since vmemmap_list is PowerPC specific, the symbol
> might also be removed.
>
>
Ok, got it, thanks for the information.
> But, if it's a coding guidelines requirement to handle the return values,
> I can think of how to handle the return value of readmem, any suggestions ?
>
>
No, it's not a coding guidelines requirement. :-)
>
> And other changes are fine to me.
>
> Thank you for your reviews Lianbo :)
>
>
No problem.
Thanks.
Lianbo
> Thanks,
> - Aditya Gupta
>
1:13 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Thu, Sep 21, 2023 at 7:26 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
Hi Lianbo,
Resending this, since skipped your mail from "To:" field by mistake.
No worries. I can also get your reply from the mail list.
On Thu, Sep 21, 2023 at 05:46:32PM +0800, lijiang wrote:
> On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta <adityag(a)linux.ibm.com>
wrote:
>
> > On 21/09/23 11:21, lijiang wrote:
> >
> > On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com>
> > <lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote: Hi, Aditya Thank you for
> > the patch. On Mon, Sep 11, 2023 at 8: 00 PM <crash-utility-request@
> > redhat. com> <crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom>
wrote:
> > From: Aditya Gupta <adityag@ linux. ibm. com>
> > <adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
> > On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
> >
> >> Hi, Aditya
> >> Thank you for the patch.
> >>
> >> On Mon, Sep 11, 2023 at 8:00 PM <crash-utility-request(a)redhat.com>
wrote:
> >>
> >>
> >> ...
> >>
> >>> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
> >>> 1 file changed, 33 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/ppc64.c b/ppc64.c
> >>> index fc34006f4863..1e84c5f56773 100644
> >>> --- a/ppc64.c
> >>> +++ b/ppc64.c
> >>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
> >>> long backing_size, virt_addr_offset, phys_offset,
list_offset;
> >>> ulong *vmemmap_list;
> >>> char *vmemmap_buf;
> >>> - struct machine_specific *ms;
> >>> -
> >>> + struct machine_specific *ms = machdep->machspec;
> >>> +
> >>> + ld = &list_data;
> >>> + BZERO(ld, sizeof(struct list_data));
> >>> +
> >>> + /*
> >>> + * vmemmap_list is missing or set to 0 in the kernel would
imply
> >>> + * vmemmap region is mapped in the kernel pagetable. So, read
> >>> vmemmap_list
> >>> + * anyway and use the translation method accordingly.
> >>> + */
> >>> + readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
> >>> sizeof(void *),
> >>> + "vmemmap_list", RETURN_ON_ERROR);
> >>> + if (!ld->start) {
> >>> + /*
> >>> + * vmemmap_list is set to 0 or missing. Do kernel
> >>> pagetable walk
> >>> + * for vmemmamp address translation.
> >>> + */
> >>> + ms->vmemmap_list = NULL;
> >>> + ms->vmemmap_cnt = 0;
> >>> +
> >>> + machdep->flags |= VMEMMAP_AWARE;
> >>> + return;
> >>> + }
> >>> +
> >>>
> >>
> > I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
> > after the 'kernel_symbol_exists("vmemmap_list")', and also
check the
> > returned value of readmem().
> >
> > Yes, I considered that, but the if condition checking for
> > 'kernel_symbol_exists("vmemmap_list")' returns from the
function if the
> > symbol is not found, same with the readmem return value check earlier,
we
> > wanted to initialise the ms->vmemmap_list and machdep->flags before
that.
> >
> >
> It's true.
>
> For the current patch, if the symbol 'vmemmap_list' is not found, the
> symbol_value() will invoke the error(FATAL, ...) and ultimately exit from
> this function. It doesn't have any chance to execute the initialising
code.
>
Got it, this would be an issue when 'vmemmap_list' is missing.
Made a mistake here, Thanks, will add an if around the readmem, that should
solve it, like this ?
if (kernel_symbol_exists("vmemmap_list"))
readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
sizeof(void *),
"vmemmap_list", RETURN_ON_ERROR | QUIET);
Will fix this in V2.
Ok, Thanks. For the ppc64_vmemmap_init() changes, does this work for you?
diff --git a/ppc64.c b/ppc64.c
index fc34006..3e155a0 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -1280,10 +1280,34 @@ void ppc64_vmemmap_init(void)
long backing_size, virt_addr_offset, phys_offset, list_offset;
ulong *vmemmap_list;
char *vmemmap_buf;
- struct machine_specific *ms;
-
- if (!(kernel_symbol_exists("vmemmap_list")) ||
- !(kernel_symbol_exists("mmu_psize_defs")) ||
+ struct machine_specific *ms = machdep->machspec;
+
+ if (!kernel_symbol_exists("vmemmap_list"))
+ return;
+
+ ld = &list_data;
+ BZERO(ld, sizeof(struct list_data));
+
+ /*
+ * vmemmap_list is missing or set to 0 in the kernel would imply
+ * vmemmap region is mapped in the kernel pagetable. So, read vmemmap_list
+ * anyway and use the translation method accordingly.
+ */
+ readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start, sizeof(void
*),
+ "vmemmap_list", RETURN_ON_ERROR|QUIET);
+ if (!ld->start) {
+ /*
+ * vmemmap_list is set to 0 or missing. Do kernel pagetable walk
+ * for vmemmamp address translation.
+ */
+ ms->vmemmap_list = NULL;
+ ms->vmemmap_cnt = 0;
+
+ machdep->flags |= VMEMMAP_AWARE;
+ return;
+ }
+
+ if (!(kernel_symbol_exists("mmu_psize_defs")) ||
!(kernel_symbol_exists("mmu_vmemmap_psize")) ||
!STRUCT_EXISTS("vmemmap_backing") ||
!STRUCT_EXISTS("mmu_psize_def") ||
@@ -1293,8 +1317,6 @@ void ppc64_vmemmap_init(void)
!MEMBER_EXISTS("vmemmap_backing", "list"))
return;
- ms = machdep->machspec;
-
backing_size = STRUCT_SIZE("vmemmap_backing");
virt_addr_offset = MEMBER_OFFSET("vmemmap_backing", "virt_addr");
phys_offset = MEMBER_OFFSET("vmemmap_backing", "phys");
@@ -1313,12 +1335,6 @@ void ppc64_vmemmap_init(void)
ms->vmemmap_psize = 1 << shift;
- ld = &list_data;
- BZERO(ld, sizeof(struct list_data));
- if (!readmem(symbol_value("vmemmap_list"),
- KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
- RETURN_ON_ERROR))
- return;
ld->end = symbol_value("vmemmap_list");
ld->list_head_offset = list_offset;
BTW: I just got a system with the radix-mmu, and reproduced this issue.
# dmesg|grep mmu
[ 0.000000] dt-cpu-ftrs: final cpu/mmu features = 0x0001b8eb8f5fb187
0x3c007041
[ 0.000000] radix-mmu: Page sizes from device-tree:
[ 0.000000] radix-mmu: Page size shift = 12 AP=0x0
[ 0.000000] radix-mmu: Page size shift = 16 AP=0x5
[ 0.000000] radix-mmu: Page size shift = 21 AP=0x1
[ 0.000000] radix-mmu: Page size shift = 30 AP=0x2
[ 0.000000] radix-mmu: Mapped 0x0000000000000000-0x0000000002800000 with
2.00 MiB pages (exec)
[ 0.000000] radix-mmu: Mapped 0x0000000002800000-0x0000000040000000 with
2.00 MiB pages
...
Thanks
Lianbo
And, the initialization code relies on the result of ld->start,
once the
> readmem() fails due to some reasons, it may also enter the
> initialization code. I'm not sure if that is expected behavior(if yes,
the
> "RETURN_ON_ERROR|QUIET " would be good).
Okay, even if readmem fails, we still go ahead and since ld was all zeros,
'if (!ld->start)' should remain true, for both cases when vmemmap_list is
empty
(head is NULL, hence ld->start=0), vmemmap_list symbol is not found or
readmem
fails (ld->start=0 since ld was all zeros due to BZERO).
Will add "RETURN_ON_ERROR|QUIET" also in V2.
>
> If so, the kernel_symbol_exists("vmemmap_list") may be redundant. That
can
> be removed this time. What do you think?
>
True. Since the case of 'vmemmap_list' being empty, or symbol not being
there,
both get handled before this, that can be removed.
Will do it in V2.
Thanks,
- Aditya Gupta
> > The readmem used to be like this, which returns from function if
readmem
> > failed:
> >
> > - if (!readmem(symbol_value("vmemmap_list"),
> > - KVADDR, &ld->start, sizeof(void *),
"vmemmap_list",
> > - RETURN_ON_ERROR))
> > - return;
> >
> > So, intentionally I put it above the if and removed the return value
> > check, since we don't want to depend on vmemmap_list symbol being
there for
> > newer kernels anymore.
> > And to be future proof in case the address mapping moves to page table
for
> > Hash MMU case also, and since vmemmap_list is PowerPC specific, the
symbol
> > might also be removed.
> >
> >
> Ok, got it, thanks for the information.
>
>
> > But, if it's a coding guidelines requirement to handle the return
values,
> > I can think of how to handle the return value of readmem, any
suggestions ?
> >
> >
> No, it's not a coding guidelines requirement. :-)
>
> >
> > And other changes are fine to me.
> >
> > Thank you for your reviews Lianbo :)
> >
> >
> No problem.
>
> Thanks.
> Lianbo
>
>
> > Thanks,
> > - Aditya Gupta
> >
4:26 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Fri, Sep 22, 2023 at 02:13:31PM +0800, lijiang wrote:
On Thu, Sep 21, 2023 at 7:26 PM Aditya Gupta
<adityag(a)linux.ibm.com> wrote:
> Hi Lianbo,
> Resending this, since skipped your mail from "To:" field by mistake.
>
>
No worries. I can also get your reply from the mail list.
> On Thu, Sep 21, 2023 at 05:46:32PM +0800, lijiang wrote:
> > On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta <adityag(a)linux.ibm.com>
> wrote:
> >
> > > On 21/09/23 11:21, lijiang wrote:
> > >
> > > On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat. com>
> > > <lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote: Hi, Aditya Thank you
for
> > > the patch. On Mon, Sep 11, 2023 at 8: 00 PM <crash-utility-request@
> > > redhat. com>
<crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom>
> wrote:
> > > From: Aditya Gupta <adityag@ linux. ibm. com>
> > > <adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
> > > On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com> wrote:
> > >
> > >> Hi, Aditya
> > >> Thank you for the patch.
> > >>
> > >> On Mon, Sep 11, 2023 at 8:00 PM
<crash-utility-request(a)redhat.com>
> wrote:
> > >>
> > >>
> > >> ...
> > >>
> > >>> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
> > >>> 1 file changed, 33 insertions(+), 14 deletions(-)
> > >>>
> > >>> diff --git a/ppc64.c b/ppc64.c
> > >>> index fc34006f4863..1e84c5f56773 100644
> > >>> --- a/ppc64.c
> > >>> +++ b/ppc64.c
> > >>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
> > >>> long backing_size, virt_addr_offset, phys_offset,
> list_offset;
> > >>> ulong *vmemmap_list;
> > >>> char *vmemmap_buf;
> > >>> - struct machine_specific *ms;
> > >>> -
> > >>> + struct machine_specific *ms = machdep->machspec;
> > >>> +
> > >>> + ld = &list_data;
> > >>> + BZERO(ld, sizeof(struct list_data));
> > >>> +
> > >>> + /*
> > >>> + * vmemmap_list is missing or set to 0 in the kernel
would
> imply
> > >>> + * vmemmap region is mapped in the kernel pagetable. So,
read
> > >>> vmemmap_list
> > >>> + * anyway and use the translation method accordingly.
> > >>> + */
> > >>> + readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
> > >>> sizeof(void *),
> > >>> + "vmemmap_list", RETURN_ON_ERROR);
> > >>> + if (!ld->start) {
> > >>> + /*
> > >>> + * vmemmap_list is set to 0 or missing. Do kernel
> > >>> pagetable walk
> > >>> + * for vmemmamp address translation.
> > >>> + */
> > >>> + ms->vmemmap_list = NULL;
> > >>> + ms->vmemmap_cnt = 0;
> > >>> +
> > >>> + machdep->flags |= VMEMMAP_AWARE;
> > >>> + return;
> > >>> + }
> > >>> +
> > >>>
> > >>
> > > I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
> > > after the 'kernel_symbol_exists("vmemmap_list")', and
also check the
> > > returned value of readmem().
> > >
> > > Yes, I considered that, but the if condition checking for
> > > 'kernel_symbol_exists("vmemmap_list")' returns from the
function if the
> > > symbol is not found, same with the readmem return value check earlier,
> we
> > > wanted to initialise the ms->vmemmap_list and machdep->flags before
> that.
> > >
> > >
> > It's true.
> >
> > For the current patch, if the symbol 'vmemmap_list' is not found, the
> > symbol_value() will invoke the error(FATAL, ...) and ultimately exit from
> > this function. It doesn't have any chance to execute the initialising
> code.
> >
>
> Got it, this would be an issue when 'vmemmap_list' is missing.
> Made a mistake here, Thanks, will add an if around the readmem, that should
> solve it, like this ?
>
> if (kernel_symbol_exists("vmemmap_list"))
> readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
> sizeof(void *),
> "vmemmap_list", RETURN_ON_ERROR | QUIET);
>
> Will fix this in V2.
>
>
Ok, Thanks. For the ppc64_vmemmap_init() changes, does this work for you?
diff --git a/ppc64.c b/ppc64.c
index fc34006..3e155a0 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -1280,10 +1280,34 @@ void ppc64_vmemmap_init(void)
long backing_size, virt_addr_offset, phys_offset, list_offset;
ulong *vmemmap_list;
char *vmemmap_buf;
- struct machine_specific *ms;
-
- if (!(kernel_symbol_exists("vmemmap_list")) ||
- !(kernel_symbol_exists("mmu_psize_defs")) ||
+ struct machine_specific *ms = machdep->machspec;
+
+ if (!kernel_symbol_exists("vmemmap_list"))
+ return;
Returning here is an issue, since 'machdep->flags |= VMEMMAP_AWARE' won't
be
run, and since 'ppc64_vmemmap_to_phys' checks for the flag before calling
'ppc64_vtop_level4' for page traversal.
So page traversal will not happen if the flag is not set. Ideally I will want
the if (!ld->start) codeblock to run if vmemmap_list symbol is missing or the
list is empty
Currently I am planning this diff for V2:
diff --git a/ppc64.c b/ppc64.c
index fc34006f4863..cd98a679c45e 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -1280,10 +1280,32 @@ void ppc64_vmemmap_init(void)
long backing_size, virt_addr_offset, phys_offset, list_offset;
ulong *vmemmap_list;
char *vmemmap_buf;
- struct machine_specific *ms;
-
- if (!(kernel_symbol_exists("vmemmap_list")) ||
- !(kernel_symbol_exists("mmu_psize_defs")) ||
+ struct machine_specific *ms = machdep->machspec;
+
+ ld = &list_data;
+ BZERO(ld, sizeof(struct list_data));
+
+ /*
+ * vmemmap_list is missing or set to 0 in the kernel would imply
+ * vmemmap region is mapped in the kernel pagetable. So, read vmemmap_list
+ * anyway and use the translation method accordingly.
+ */
+ if (kernel_symbol_exists("vmemmap_list"))
+ readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
+ sizeof(void *), "vmemmap_list",
RETURN_ON_ERROR|QUIET);
Here I add a check for 'kernel_symbol_exists' before
'symbol_value("vmemmap_list") is done
+ if (!ld->start) {
+ /*
+ * vmemmap_list is set to 0 or missing. Do kernel pagetable walk
+ * for vmemmap address translation.
+ */
+ ms->vmemmap_list = NULL;
+ ms->vmemmap_cnt = 0;
+
+ machdep->flags |= VMEMMAP_AWARE;
+ return;
+ }
+
+ if (!(kernel_symbol_exists("mmu_psize_defs")) ||
!(kernel_symbol_exists("mmu_vmemmap_psize")) ||
...
Here I removed the kernel_symbol_exists("vmemmap_list")
+
+ ld = &list_data;
+ BZERO(ld, sizeof(struct list_data));
+
+ /*
+ * vmemmap_list is missing or set to 0 in the kernel would imply
+ * vmemmap region is mapped in the kernel pagetable. So, read vmemmap_list
+ * anyway and use the translation method accordingly.
+ */
+ readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start, sizeof(void
*),
+ "vmemmap_list", RETURN_ON_ERROR|QUIET);
+ if (!ld->start) {
+ /*
+ * vmemmap_list is set to 0 or missing. Do kernel pagetable walk
+ * for vmemmamp address translation.
+ */
+ ms->vmemmap_list = NULL;
+ ms->vmemmap_cnt = 0;
+
+ machdep->flags |= VMEMMAP_AWARE;
+ return;
+ }
+
+ if (!(kernel_symbol_exists("mmu_psize_defs")) ||
!(kernel_symbol_exists("mmu_vmemmap_psize")) ||
!STRUCT_EXISTS("vmemmap_backing") ||
!STRUCT_EXISTS("mmu_psize_def") ||
@@ -1293,8 +1317,6 @@ void ppc64_vmemmap_init(void)
!MEMBER_EXISTS("vmemmap_backing", "list"))
return;
Yup, this is good, I will remove the kernel_symbol_exists check for
vmemmap_list from if in next version, since it is handled earlier
Thanks,
Aditya Gupta
BTW: I just got a system with the radix-mmu, and reproduced this issue.
# dmesg|grep mmu
[ 0.000000] dt-cpu-ftrs: final cpu/mmu features = 0x0001b8eb8f5fb187
0x3c007041
[ 0.000000] radix-mmu: Page sizes from device-tree:
[ 0.000000] radix-mmu: Page size shift = 12 AP=0x0
[ 0.000000] radix-mmu: Page size shift = 16 AP=0x5
[ 0.000000] radix-mmu: Page size shift = 21 AP=0x1
[ 0.000000] radix-mmu: Page size shift = 30 AP=0x2
[ 0.000000] radix-mmu: Mapped 0x0000000000000000-0x0000000002800000 with
2.00 MiB pages (exec)
[ 0.000000] radix-mmu: Mapped 0x0000000002800000-0x0000000040000000 with
2.00 MiB pages
...
Thanks
Lianbo
> And, the initialization code relies on the result of ld->start, once the
> > readmem() fails due to some reasons, it may also enter the
> > initialization code. I'm not sure if that is expected behavior(if yes,
> the
> > "RETURN_ON_ERROR|QUIET " would be good).
>
> Okay, even if readmem fails, we still go ahead and since ld was all zeros,
> 'if (!ld->start)' should remain true, for both cases when vmemmap_list
is
> empty
> (head is NULL, hence ld->start=0), vmemmap_list symbol is not found or
> readmem
> fails (ld->start=0 since ld was all zeros due to BZERO).
>
> Will add "RETURN_ON_ERROR|QUIET" also in V2.
>
> >
> > If so, the kernel_symbol_exists("vmemmap_list") may be redundant.
That
> can
> > be removed this time. What do you think?
> >
>
> True. Since the case of 'vmemmap_list' being empty, or symbol not being
> there,
> both get handled before this, that can be removed.
> Will do it in V2.
>
> Thanks,
> - Aditya Gupta
>
> > > The readmem used to be like this, which returns from function if
> readmem
> > > failed:
> > >
> > > - if (!readmem(symbol_value("vmemmap_list"),
> > > - KVADDR, &ld->start, sizeof(void *),
"vmemmap_list",
> > > - RETURN_ON_ERROR))
> > > - return;
> > >
> > > So, intentionally I put it above the if and removed the return value
> > > check, since we don't want to depend on vmemmap_list symbol being
> there for
> > > newer kernels anymore.
> > > And to be future proof in case the address mapping moves to page table
> for
> > > Hash MMU case also, and since vmemmap_list is PowerPC specific, the
> symbol
> > > might also be removed.
> > >
> > >
> > Ok, got it, thanks for the information.
> >
> >
> > > But, if it's a coding guidelines requirement to handle the return
> values,
> > > I can think of how to handle the return value of readmem, any
> suggestions ?
> > >
> > >
> > No, it's not a coding guidelines requirement. :-)
> >
> > >
> > > And other changes are fine to me.
> > >
> > > Thank you for your reviews Lianbo :)
> > >
> > >
> > No problem.
> >
> > Thanks.
> > Lianbo
> >
> >
> > > Thanks,
> > > - Aditya Gupta
> > >
>
>
>
5:22 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Fri, Sep 22, 2023 at 5:26 PM Aditya Gupta <adityag(a)linux.ibm.com> wrote:
On Fri, Sep 22, 2023 at 02:13:31PM +0800, lijiang wrote:
> On Thu, Sep 21, 2023 at 7:26 PM Aditya Gupta <adityag(a)linux.ibm.com>
wrote:
>
> > Hi Lianbo,
> > Resending this, since skipped your mail from "To:" field by mistake.
> >
> >
> No worries. I can also get your reply from the mail list.
>
>
> > On Thu, Sep 21, 2023 at 05:46:32PM +0800, lijiang wrote:
> > > On Thu, Sep 21, 2023 at 3:59 PM Aditya Gupta
<adityag(a)linux.ibm.com>
> > wrote:
> > >
> > > > On 21/09/23 11:21, lijiang wrote:
> > > >
> > > > On Mon, Sep 18, 2023 at 7: 34 PM lijiang <lijiang@ redhat.
com>
> > > > <lijiang(a)%E2%80%8Aredhat.%E2%80%8Acom> wrote: Hi, Aditya Thank
you for
> > > > the patch. On Mon, Sep 11, 2023 at 8: 00 PM
<crash-utility-request@
> > > > redhat. com>
<crash-utility-request(a)%E2%80%8Aredhat.%E2%80%8Acom>
> > wrote:
> > > > From: Aditya Gupta <adityag@ linux. ibm. com>
> > > > <adityag(a)%E2%80%8Alinux.%E2%80%8Aibm.%E2%80%8Acom> To:
> > > > On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com>
wrote:
> > > >
> > > >> Hi, Aditya
> > > >> Thank you for the patch.
> > > >>
> > > >> On Mon, Sep 11, 2023 at 8:00 PM
<crash-utility-request(a)redhat.com
>
> > wrote:
> > > >>
> > > >>
> > > >> ...
> > > >>
> > > >>> ppc64.c | 47
+++++++++++++++++++++++++++++++++--------------
> > > >>> 1 file changed, 33 insertions(+), 14 deletions(-)
> > > >>>
> > > >>> diff --git a/ppc64.c b/ppc64.c
> > > >>> index fc34006f4863..1e84c5f56773 100644
> > > >>> --- a/ppc64.c
> > > >>> +++ b/ppc64.c
> > > >>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
> > > >>> long backing_size, virt_addr_offset, phys_offset,
> > list_offset;
> > > >>> ulong *vmemmap_list;
> > > >>> char *vmemmap_buf;
> > > >>> - struct machine_specific *ms;
> > > >>> -
> > > >>> + struct machine_specific *ms = machdep->machspec;
> > > >>> +
> > > >>> + ld = &list_data;
> > > >>> + BZERO(ld, sizeof(struct list_data));
> > > >>> +
> > > >>> + /*
> > > >>> + * vmemmap_list is missing or set to 0 in the kernel
would
> > imply
> > > >>> + * vmemmap region is mapped in the kernel pagetable.
So,
read
> > > >>> vmemmap_list
> > > >>> + * anyway and use the translation method
accordingly.
> > > >>> + */
> > > >>> + readmem(symbol_value("vmemmap_list"),
KVADDR, &ld->start,
> > > >>> sizeof(void *),
> > > >>> + "vmemmap_list", RETURN_ON_ERROR);
> > > >>> + if (!ld->start) {
> > > >>> + /*
> > > >>> + * vmemmap_list is set to 0 or missing. Do
kernel
> > > >>> pagetable walk
> > > >>> + * for vmemmamp address translation.
> > > >>> + */
> > > >>> + ms->vmemmap_list = NULL;
> > > >>> + ms->vmemmap_cnt = 0;
> > > >>> +
> > > >>> + machdep->flags |= VMEMMAP_AWARE;
> > > >>> + return;
> > > >>> + }
> > > >>> +
> > > >>>
> > > >>
> > > > I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
> > > > after the 'kernel_symbol_exists("vmemmap_list")',
and also check
the
> > > > returned value of readmem().
> > > >
> > > > Yes, I considered that, but the if condition checking for
> > > > 'kernel_symbol_exists("vmemmap_list")' returns from
the function
if the
> > > > symbol is not found, same with the readmem return value check
earlier,
> > we
> > > > wanted to initialise the ms->vmemmap_list and machdep->flags
before
> > that.
> > > >
> > > >
> > > It's true.
> > >
> > > For the current patch, if the symbol 'vmemmap_list' is not found,
the
> > > symbol_value() will invoke the error(FATAL, ...) and ultimately exit
from
> > > this function. It doesn't have any chance to execute the initialising
> > code.
> > >
> >
> > Got it, this would be an issue when 'vmemmap_list' is missing.
> > Made a mistake here, Thanks, will add an if around the readmem, that
should
> > solve it, like this ?
> >
> > if (kernel_symbol_exists("vmemmap_list"))
> > readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
> > sizeof(void *),
> > "vmemmap_list", RETURN_ON_ERROR | QUIET);
> >
> > Will fix this in V2.
> >
> >
> Ok, Thanks. For the ppc64_vmemmap_init() changes, does this work for you?
>
> diff --git a/ppc64.c b/ppc64.c
> index fc34006..3e155a0 100644
> --- a/ppc64.c
> +++ b/ppc64.c
> @@ -1280,10 +1280,34 @@ void ppc64_vmemmap_init(void)
> long backing_size, virt_addr_offset, phys_offset, list_offset;
> ulong *vmemmap_list;
> char *vmemmap_buf;
> - struct machine_specific *ms;
> -
> - if (!(kernel_symbol_exists("vmemmap_list")) ||
> - !(kernel_symbol_exists("mmu_psize_defs")) ||
> + struct machine_specific *ms = machdep->machspec;
> +
> + if (!kernel_symbol_exists("vmemmap_list"))
> + return;
Returning here is an issue, since 'machdep->flags |= VMEMMAP_AWARE' won't
be
run, and since 'ppc64_vmemmap_to_phys' checks for the flag before calling
'ppc64_vtop_level4' for page traversal.
Understood.
So page traversal will not happen if the flag is not set. Ideally I
will
want
the if (!ld->start) codeblock to run if vmemmap_list symbol is missing or
the
list is empty
Now this is the point. I have no other issues, thank you for the
explanation, Aditya.
Currently I am planning this diff for V2:
diff --git a/ppc64.c b/ppc64.c
index fc34006f4863..cd98a679c45e 100644
--- a/ppc64.c
+++ b/ppc64.c
@@ -1280,10 +1280,32 @@ void ppc64_vmemmap_init(void)
long backing_size, virt_addr_offset, phys_offset, list_offset;
ulong *vmemmap_list;
char *vmemmap_buf;
- struct machine_specific *ms;
-
- if (!(kernel_symbol_exists("vmemmap_list")) ||
- !(kernel_symbol_exists("mmu_psize_defs")) ||
+ struct machine_specific *ms = machdep->machspec;
+
+ ld = &list_data;
+ BZERO(ld, sizeof(struct list_data));
+
+ /*
+ * vmemmap_list is missing or set to 0 in the kernel would
imply
+ * vmemmap region is mapped in the kernel pagetable. So, read
vmemmap_list
+ * anyway and use the translation method accordingly.
+ */
+ if (kernel_symbol_exists("vmemmap_list"))
+ readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
+ sizeof(void *), "vmemmap_list",
RETURN_ON_ERROR|QUIET);
Here I add a check for 'kernel_symbol_exists' before
'symbol_value("vmemmap_list") is done
+ if (!ld->start) {
+ /*
+ * vmemmap_list is set to 0 or missing. Do kernel
pagetable walk
+ * for vmemmap address translation.
+ */
+ ms->vmemmap_list = NULL;
+ ms->vmemmap_cnt = 0;
+
+ machdep->flags |= VMEMMAP_AWARE;
+ return;
+ }
+
+ if (!(kernel_symbol_exists("mmu_psize_defs")) ||
!(kernel_symbol_exists("mmu_vmemmap_psize")) ||
...
Here I removed the kernel_symbol_exists("vmemmap_list")
> +
> + ld = &list_data;
> + BZERO(ld, sizeof(struct list_data));
> +
> + /*
> + * vmemmap_list is missing or set to 0 in the kernel would imply
> + * vmemmap region is mapped in the kernel pagetable. So, read
vmemmap_list
> + * anyway and use the translation method accordingly.
> + */
> + readmem(symbol_value("vmemmap_list"), KVADDR, &ld->start,
sizeof(void
*),
> + "vmemmap_list", RETURN_ON_ERROR|QUIET);
> + if (!ld->start) {
> + /*
> + * vmemmap_list is set to 0 or missing. Do kernel pagetable walk
> + * for vmemmamp address translation.
> + */
> + ms->vmemmap_list = NULL;
> + ms->vmemmap_cnt = 0;
> +
> + machdep->flags |= VMEMMAP_AWARE;
> + return;
> + }
> +
> + if (!(kernel_symbol_exists("mmu_psize_defs")) ||
> !(kernel_symbol_exists("mmu_vmemmap_psize")) ||
> !STRUCT_EXISTS("vmemmap_backing") ||
> !STRUCT_EXISTS("mmu_psize_def") ||
> @@ -1293,8 +1317,6 @@ void ppc64_vmemmap_init(void)
> !MEMBER_EXISTS("vmemmap_backing", "list"))
> return;
>
Yup, this is good, I will remove the kernel_symbol_exists check for
vmemmap_list from if in next version, since it is handled earlier
OK. Looks good to me.
Thanks.
Lianbo
Thanks,
Aditya Gupta
>
> BTW: I just got a system with the radix-mmu, and reproduced this issue.
> # dmesg|grep mmu
> [ 0.000000] dt-cpu-ftrs: final cpu/mmu features = 0x0001b8eb8f5fb187
> 0x3c007041
> [ 0.000000] radix-mmu: Page sizes from device-tree:
> [ 0.000000] radix-mmu: Page size shift = 12 AP=0x0
> [ 0.000000] radix-mmu: Page size shift = 16 AP=0x5
> [ 0.000000] radix-mmu: Page size shift = 21 AP=0x1
> [ 0.000000] radix-mmu: Page size shift = 30 AP=0x2
> [ 0.000000] radix-mmu: Mapped 0x0000000000000000-0x0000000002800000
with
> 2.00 MiB pages (exec)
> [ 0.000000] radix-mmu: Mapped 0x0000000002800000-0x0000000040000000
with
> 2.00 MiB pages
> ...
>
> Thanks
> Lianbo
>
> > And, the initialization code relies on the result of ld->start, once
the
> > > readmem() fails due to some reasons, it may also enter the
> > > initialization code. I'm not sure if that is expected behavior(if
yes,
> > the
> > > "RETURN_ON_ERROR|QUIET " would be good).
> >
> > Okay, even if readmem fails, we still go ahead and since ld was all
zeros,
> > 'if (!ld->start)' should remain true, for both cases when
vmemmap_list
is
> > empty
> > (head is NULL, hence ld->start=0), vmemmap_list symbol is not found or
> > readmem
> > fails (ld->start=0 since ld was all zeros due to BZERO).
> >
> > Will add "RETURN_ON_ERROR|QUIET" also in V2.
> >
> > >
> > > If so, the kernel_symbol_exists("vmemmap_list") may be
redundant.
That
> > can
> > > be removed this time. What do you think?
> > >
> >
> > True. Since the case of 'vmemmap_list' being empty, or symbol not
being
> > there,
> > both get handled before this, that can be removed.
> > Will do it in V2.
> >
> > Thanks,
> > - Aditya Gupta
> >
> > > > The readmem used to be like this, which returns from function if
> > readmem
> > > > failed:
> > > >
> > > > - if (!readmem(symbol_value("vmemmap_list"),
> > > > - KVADDR, &ld->start, sizeof(void *),
"vmemmap_list",
> > > > - RETURN_ON_ERROR))
> > > > - return;
> > > >
> > > > So, intentionally I put it above the if and removed the return
value
> > > > check, since we don't want to depend on vmemmap_list symbol
being
> > there for
> > > > newer kernels anymore.
> > > > And to be future proof in case the address mapping moves to page
table
> > for
> > > > Hash MMU case also, and since vmemmap_list is PowerPC specific, the
> > symbol
> > > > might also be removed.
> > > >
> > > >
> > > Ok, got it, thanks for the information.
> > >
> > >
> > > > But, if it's a coding guidelines requirement to handle the
return
> > values,
> > > > I can think of how to handle the return value of readmem, any
> > suggestions ?
> > > >
> > > >
> > > No, it's not a coding guidelines requirement. :-)
> > >
> > > >
> > > > And other changes are fine to me.
> > > >
> > > > Thank you for your reviews Lianbo :)
> > > >
> > > >
> > > No problem.
> > >
> > > Thanks.
> > > Lianbo
> > >
> > >
> > > > Thanks,
> > > > - Aditya Gupta
> > > >
> >
> >
> >
4:15 a.m.
New subject: [PATCH] ppc64: do page traversal if vmemmap_list not populated
On Thu, Sep 21, 2023 at 01:51:27PM +0800, lijiang wrote:
On Mon, Sep 18, 2023 at 7:34 PM lijiang <lijiang(a)redhat.com>
wrote:
>>
>> ---
>> ---
>> ppc64.c | 47 +++++++++++++++++++++++++++++++++--------------
>> 1 file changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/ppc64.c b/ppc64.c
>> index fc34006f4863..1e84c5f56773 100644
>> --- a/ppc64.c
>> +++ b/ppc64.c
>> @@ -1280,8 +1280,30 @@ void ppc64_vmemmap_init(void)
>> long backing_size, virt_addr_offset, phys_offset, list_offset;
>> ulong *vmemmap_list;
>> char *vmemmap_buf;
>> - struct machine_specific *ms;
>> -
>> + struct machine_specific *ms = machdep->machspec;
>> +
>> + ld = &list_data;
>> + BZERO(ld, sizeof(struct list_data));
>> +
>> + /*
>> + * vmemmap_list is missing or set to 0 in the kernel would imply
>> + * vmemmap region is mapped in the kernel pagetable. So, read
>> vmemmap_list
>> + * anyway and use the translation method accordingly.
>> + */
>> + readmem(symbol_value("vmemmap_list"), KVADDR,
&ld->start,
>> sizeof(void *),
>> + "vmemmap_list", RETURN_ON_ERROR);
>> + if (!ld->start) {
>> + /*
>> + * vmemmap_list is set to 0 or missing. Do kernel
>> pagetable walk
>> + * for vmemmamp address translation.
>> + */
>> + ms->vmemmap_list = NULL;
>> + ms->vmemmap_cnt = 0;
>> +
>> + machdep->flags |= VMEMMAP_AWARE;
>> + return;
>> + }
>> +
>>
>
I would suggest putting the
'readmem(symbol_value("vmemmap_list"),...)'
after the 'kernel_symbol_exists("vmemmap_list")', and also check the
returned value of readmem().
Yes, I considered that, but the if condition checking for
'kernel_symbol_exists("vmemmap_list")' returns from the function if the
symbol
is not found, same with the readmem return value check earlier, we wanted to
initialise the ms->vmemmap_list and machdep->flags before that.
The readmem used to be like this, which returns from function if readmem failed:
- if (!readmem(symbol_value("vmemmap_list"),
- KVADDR, &ld->start, sizeof(void *), "vmemmap_list",
- RETURN_ON_ERROR))
- return;
So, intentionally I put it above the if and removed the return value check,
since we don't want to depend on vmemmap_list symbol being there for newer
kernels anymore.
And to be future proof in case the address mapping moves to page table for
Hash MMU case also, and since vmemmap_list is PowerPC specific, the symbol
might also be removed.
But, if it's a coding guidelines requirement to handle the return values, I can
think of how to handle the return value of readmem, any suggestions ?
And other changes are fine to me.
Thank you for your reviews Lianbo :)
Thanks,
- Aditya Gupta
360
days inactive
364
days old
devel@lists.crash-utility.osci.io
14 comments
2 participants
participants (2)
-
Aditya Gupta -
lijiang