NetBSD Problem Report #51663

From www@NetBSD.org  Sun Nov 27 08:47:01 2016
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
	(Client CN "mail.netbsd.org", Issuer "Postmaster NetBSD.org" (verified OK))
	by mollari.NetBSD.org (Postfix) with ESMTPS id 879017A316
	for <gnats-bugs@gnats.NetBSD.org>; Sun, 27 Nov 2016 08:47:01 +0000 (UTC)
Message-Id: <20161127084700.309167A33D@mollari.NetBSD.org>
Date: Sun, 27 Nov 2016 08:47:00 +0000 (UTC)
From: rokuyama@rk.phys.keio.ac.jp
Reply-To: rokuyama@rk.phys.keio.ac.jp
To: gnats-bugs@NetBSD.org
Subject: crash dump related bugs in x68k/machdep.c
X-Send-Pr-Version: www-1.0

>Number:         51663
>Category:       port-x68k
>Synopsis:       crash dump related bugs in x68k/machdep.c
>Confidential:   no
>Severity:       serious
>Priority:       high
>Responsible:    tsutsui
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Sun Nov 27 08:50:00 +0000 2016
>Closed-Date:    Sun Feb 05 10:24:48 +0000 2017
>Last-Modified:  Sun Feb 05 10:24:48 +0000 2017
>Originator:     Rin Okuyama
>Release:        7.99.42
>Organization:
Faculty of Science and Technology, Keio University
>Environment:
NetBSD x68k 7.99.42 NetBSD 7.99.42 (GENERIC) #4: Sat Nov 26 17:03:01 JST 2016  rin@xxx:xxx x68k
>Description:
On x68k, savecore(8) complains "no core dump (invalid dumplo)" even
though there is enough room for crash dump in the dump device. This is
because of a bug in x68k/machdep.c:

https://nxr.netbsd.org/source/xref/src/sys/arch/x68k/x68k/machdep.c#602
  602          /*
  603           * X68k has multiple RAM segments on some models.
  604           */
  605          m->ram_segs[0].start = lowram;
  606          m->ram_segs[0].size = mem_size - lowram;
  607          for (i = 1; i < vm_nphysseg; i++) {
  608                  m->ram_segs[i].start =
  609                      ctob(VM_PHYSMEM_PTR(i)->start);
  610                  m->ram_segs[i].size  =
  611                      ctob(VM_PHYSMEM_PTR(i)->end - VM_PHYSMEM_PTR(i)->start);
  612          }

Since mem_size is sum of the base and extended RAM segments, the amount
of extended RAM segments is double-counted. In addition, this code
assumes that vm_physmem[0], [1], and [2] correspond to the base, 1st,
and 2nd extended RAM segments, respectively. But this is not correct;
the order is inverted actually. In the attached patch, this part is
modified as follows:

  604          /*
  605           * X68k has multiple RAM segments on some models.
  606           */
  607          m->ram_segs[0].start = phys_basemem_seg.start;
  608          m->ram_segs[0].size = phys_basemem_seg.end;
  609  #ifdef EXTENDED_MEMORY
  610          seg = 1;
  611          for (i = 0; i < EXTMEM_SEGS; i++) {
  612                  size = phys_extmem_seg[i].end - phys_extmem_seg[i].start;
  613                  if (size == 0)
  614                          continue;
  615                  m->ram_segs[seg].start = phys_extmem_seg[i].start;
  616                  m->ram_segs[seg].size  = size;
  617                  seg++;
  618          }
  619  #endif

Here, ram_segs[0] correctly describes the base RAM segment. ram_segs[1]
and [2] includes area not used by VM subsystem. However, it does not
cause any problems except for small increase in crash dump, and more
importantly, it is free from unnecessary dependent on the internal
structure of VM subsystem.

With this patch, crash dump is successfully generated, and saved by
savecore(8). I have checked ps(1), vmstat(1), and gdb(1) work well with
saved crash dumps.
>How-To-Repeat:
Use savecore(8) or sync in ddb(4) on x68k.
>Fix:
--- src/sys/arch/x68k/x68k/machdep.c.orig	2016-11-25 16:03:30.017435756 +0900
+++ src/sys/arch/x68k/x68k/machdep.c	2016-11-26 17:02:22.570386277 +0900
@@ -553,7 +553,9 @@
 {
 	cpu_kcore_hdr_t *h = &cpu_kcore_hdr;
 	struct m68k_kcore_hdr *m = &h->un._m68k;
-	int i;
+#ifdef EXTENDED_MEMORY
+	int i, seg, size;
+#endif

 	memset(&cpu_kcore_hdr, 0, sizeof(cpu_kcore_hdr));

@@ -602,14 +604,19 @@
 	/*
 	 * X68k has multiple RAM segments on some models.
 	 */
-	m->ram_segs[0].start = lowram;
-	m->ram_segs[0].size = mem_size - lowram;
-	for (i = 1; i < vm_nphysseg; i++) {
-		m->ram_segs[i].start =
-		    ctob(VM_PHYSMEM_PTR(i)->start);
-		m->ram_segs[i].size  =
-		    ctob(VM_PHYSMEM_PTR(i)->end - VM_PHYSMEM_PTR(i)->start);
+	m->ram_segs[0].start = phys_basemem_seg.start;
+	m->ram_segs[0].size = phys_basemem_seg.end;
+#ifdef EXTENDED_MEMORY
+	seg = 1;
+	for (i = 0; i < EXTMEM_SEGS; i++) {
+		size = phys_extmem_seg[i].end - phys_extmem_seg[i].start;
+		if (size == 0)
+			continue;
+		m->ram_segs[seg].start = phys_extmem_seg[i].start;
+		m->ram_segs[seg].size  = size;
+		seg++;
 	}
+#endif
 }

 /*
@@ -806,6 +813,7 @@
 		}
 	}
 	printf("succeeded\n");
+	delay(5000000);		/* 5 seconds */
 }

 void

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: port-x68k-maintainer->tsutsui
Responsible-Changed-By: tsutsui@NetBSD.org
Responsible-Changed-When: Mon, 28 Nov 2016 06:38:23 +0000
Responsible-Changed-Why:
I'll take a look later.


From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: gnats-bugs@NetBSD.org, tsutsui@NetBSD.org
Cc: 
Subject: Re: port-x68k/51663 (crash dump related bugs in x68k/machdep.c)
Date: Mon, 28 Nov 2016 22:38:39 +0900

 Let me explain a little more.

 cpu_kcore_hdr.un._m68k.ram_segs[i].{start,size} describe start address
 and size of the i-th segment of physical memory.

 On the kernel side, dumpsys(9) dumps their contents:

 https://nxr.netbsd.org/source/xref/src/sys/arch/x68k/x68k/machdep.c#702
    702  void
    703  dumpsys(void)
    704  {
    ...
    717          seg = 0;
    ...
    750          for (pg = 0; pg < dumpsize; pg++) {
    ...
    762                  while (maddr >=
    763                      (m->ram_segs[seg].start + m->ram_segs[seg].size)) {
    764                          if (++seg >= M68K_NPHYS_RAM_SEGS ||
    765                              m->ram_segs[seg].size == 0) {
    766                                  error = EINVAL;         /* XXX ?? */
    767                                  goto bad;
    768                          }
    769                          maddr = m->ram_segs[seg].start;
    770                  }
    771                  pmap_enter(pmap_kernel(), (vaddr_t)vmmap, maddr,
    772                      VM_PROT_READ, VM_PROT_READ|PMAP_WIRED);
    773                  pmap_update(pmap_kernel());
    774
    775                  error = (*dump)(dumpdev, blkno, vmmap, PAGE_SIZE);
    776   bad:
    777                  switch (error) {
    778                  case 0:
    779                          maddr += PAGE_SIZE;
    780                          blkno += btodb(PAGE_SIZE);
    781                          break;
    ...
    806                  }
    807          }
    808          printf("succeeded\n");
    809  }

 On the userland side, libkvm converts physical address into offset of
 a crash dump:

 https://nxr.netbsd.org/source/xref/src/lib/libkvm/kvm_m68k_cmn.c#158
    158  /*
    159   * Translate a physical address to a file-offset in the crash dump.
    160   */
    161  off_t
    162  _kvm_cmn_pa2off(kvm_t *kd, u_long pa)
    163  {
    164          cpu_kcore_hdr_t *h = kd->cpu_data;
    165          struct m68k_kcore_hdr *m = &h->un._m68k;
    166          phys_ram_seg_t *rsp;
    167          off_t off;
    168          int i;
    169
    170          off = 0;
    171          rsp = m->ram_segs;
    172          for (i = 0; i < M68K_NPHYS_RAM_SEGS && rsp[i].size != 0; i++) {
    173                  if (pa >= rsp[i].start &&
    174                      pa < (rsp[i].start + rsp[i].size)) {
    175                          pa -= rsp[i].start;
    176                          break;
    177                  }
    178                  off += rsp[i].size;
    179          }
    180          return (kd->dump_off + off + pa);
    181  }

 (This function silently returns bogus offset when invalid address is
 passed. This is a dangerous behavior and should be fixed later.)

 cpu_kcore_hdr.un._m68k.ram_segs[0] must cover whole the base memory,
 including areas not registered into the free list of UVM. This is
 because there are global variables and msgbuf below and above the region
 managed by UVM.

 On the other hand, regions in extended memory not managed by UVM can
 probably be excluded from ram_segs[i] (i >= 1). However, in my patch,
 such regions are also included for the reasons explained before.

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: rokuyama@rk.phys.keio.ac.jp
Cc: gnats-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-x68k/51663 (crash dump related bugs in x68k/machdep.c)
Date: Wed, 30 Nov 2016 00:56:45 +0900

 > Let me explain a little more.

 Thank you for your explanation. Your analysis is correct.

 > cpu_kcore_hdr.un._m68k.ram_segs[i].{start,size} describe start address
 > and size of the i-th segment of physical memory.
  :

 I see.  Actually I didn't understand what values ram_segs[] should have,
 and I should have checked cpu_init_kcore_hdr() more carefully
 when I changed mem_exists() for PR/45915:
  http://mail-index.netbsd.org/port-x68k/2013/10/19/msg000039.html

 > cpu_kcore_hdr.un._m68k.ram_segs[0] must cover whole the base memory,
 > including areas not registered into the free list of UVM. This is
 > because there are global variables and msgbuf below and above the region
 > managed by UVM.

 (BTW, mem_size for ram_segs[0] seems incorrect even before
  my mem_exist() changes..)

 > On the other hand, regions in extended memory not managed by UVM can
 > probably be excluded from ram_segs[i] (i >= 1). However, in my patch,
 > such regions are also included for the reasons explained before.

 Maybe we should clarify the definitions of the following regions
 in some m68k docs:

  a) VM_PHYSMEM_PTR() (formerly vm_physmem[]) registered by
     uvm_page_physload(9)
  b) phys_ram_seg_t ram_segs[] for crashdump

 The region a) includes memories that are managed by uvm(9).
 The region b) includes memories that should be accessed
 on gdb "target kvm" debug using crashdump via savecore(8).

 The main difference is that b) includes kernel itself and msgbuf
 but a) should not have them.

 b) would exclude memory regions that are not managed by kernel
 (for example, PROM reserved area on news68k and sun3x etc.)
 as you mentioned, but IIRC x68k uses whole extended memory regions.

 Forthermore, it would be harmless to register unmanaged regions
 into b) (unless the region includes some sensitive data),
 so we don't have to consider it strictly if the port doesn't
 have useful info (variables) about the "unmanaged regions."

 (Note news68k PROM excludes its reserved area from "memsize" info)

 Your patch seems fine (maybe "size" should be psize_t rather than int).
 I'll commit it after I also confirm it on my x68k.

 Thanks,

 ---
 Izumi Tsutsui

State-Changed-From-To: open->analyzed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Tue, 29 Nov 2016 16:02:48 +0000
State-Changed-Why:


From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org
Subject: Re: port-x68k/51663 (crash dump related bugs in x68k/machdep.c)
Date: Wed, 30 Nov 2016 21:16:26 +0900

 Thank you very much for your careful analysis and review of my patch.
 Also, I appreciate your detailed explanation. I learned a lot from you.

 Rin

From: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
To: rokuyama@rk.phys.keio.ac.jp
Cc: gnats-bugs@NetBSD.org, tsutsui@ceres.dti.ne.jp
Subject: Re: port-x68k/51663 (crash dump related bugs in x68k/machdep.c)
Date: Thu, 1 Dec 2016 06:49:02 +0900

 I slightly modified your patch and have confirmed it on my X68030
 (which has no highmem, so it just means "no bad side effect").

 Changes are:
 - use psize_t (instead of int) for "size" of memory regions
 - explicitly calculate size of basemem_seg (for future readers)
   even though phys_basemem_seg.start (== lowram) is zero on x68k

 Could you also confirm? (sorry I'm so lazy not to setup XM6i)

 ---
 Index: x68k/machdep.c
 ===================================================================
 RCS file: /cvsroot/src/sys/arch/x68k/x68k/machdep.c,v
 retrieving revision 1.193
 diff -u -p -d -r1.193 machdep.c
 --- x68k/machdep.c	14 Jun 2016 07:51:10 -0000	1.193
 +++ x68k/machdep.c	30 Nov 2016 21:47:54 -0000
 @@ -553,7 +553,10 @@ cpu_init_kcore_hdr(void)
  {
  	cpu_kcore_hdr_t *h = &cpu_kcore_hdr;
  	struct m68k_kcore_hdr *m = &h->un._m68k;
 -	int i;
 +	psize_t size;
 +#ifdef EXTENDED_MEMORY
 +	int i, seg;
 +#endif

  	memset(&cpu_kcore_hdr, 0, sizeof(cpu_kcore_hdr));

 @@ -602,14 +605,20 @@ cpu_init_kcore_hdr(void)
  	/*
  	 * X68k has multiple RAM segments on some models.
  	 */
 -	m->ram_segs[0].start = lowram;
 -	m->ram_segs[0].size = mem_size - lowram;
 -	for (i = 1; i < vm_nphysseg; i++) {
 -		m->ram_segs[i].start =
 -		    ctob(VM_PHYSMEM_PTR(i)->start);
 -		m->ram_segs[i].size  =
 -		    ctob(VM_PHYSMEM_PTR(i)->end - VM_PHYSMEM_PTR(i)->start);
 +	size = phys_basemem_seg.end - phys_basemem_seg.start;
 +	m->ram_segs[0].start = phys_basemem_seg.start;
 +	m->ram_segs[0].size  = size;
 +#ifdef EXTENDED_MEMORY
 +	seg = 1;
 +	for (i = 0; i < EXTMEM_SEGS; i++) {
 +		size = phys_extmem_seg[i].end - phys_extmem_seg[i].start;
 +		if (size == 0)
 +			continue;
 +		m->ram_segs[seg].start = phys_extmem_seg[i].start;
 +		m->ram_segs[seg].size  = size;
 +		seg++;
  	}
 +#endif
  }

  /*

 ---
 Izumi Tsutsui

From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org
Subject: Re: port-x68k/51663 (crash dump related bugs in x68k/machdep.c)
Date: Thu, 1 Dec 2016 12:38:27 +0900

 On 2016/12/01 6:49, Izumi Tsutsui wrote:
 > I slightly modified your patch and have confirmed it on my X68030
 > (which has no highmem, so it just means "no bad side effect").
 >
 > Changes are:
 > - use psize_t (instead of int) for "size" of memory regions
 > - explicitly calculate size of basemem_seg (for future readers)
 >   even though phys_basemem_seg.start (== lowram) is zero on x68k
 >
 > Could you also confirm? (sorry I'm so lazy not to setup XM6i)

 Thank you for your revision. It seems very reasonable to me. I've
 tested your patch on XM6i with 12MB of base memory and the following
 layouts of extended memory:

 (1)  16MB at 0x01000000 (TS-6BE16 mode)
 (2) 128MB at 0x10000000 (060turbo mode)
 (3) 240MB at 0x01000000 and 768MB at 0x01000000

 dmesg, ps, vmstat, and gdb work just fine for the all layouts.

 Thanks,
 Rin

From: Rin Okuyama <rokuyama@rk.phys.keio.ac.jp>
To: Izumi Tsutsui <tsutsui@ceres.dti.ne.jp>
Cc: gnats-bugs@NetBSD.org
Subject: Re: port-x68k/51663 (crash dump related bugs in x68k/machdep.c)
Date: Thu, 1 Dec 2016 12:42:09 +0900

 Sorry, I made typo.

 On 2016/12/01 12:38, Rin Okuyama wrote:
 > (3) 240MB at 0x01000000 and 768MB at 0x01000000

 This one should be

 > (3) 240MB at 0x01000000 and 768MB at 0x10000000

From: "Izumi Tsutsui" <tsutsui@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51663 CVS commit: src/sys/arch/x68k/x68k
Date: Fri, 2 Dec 2016 12:43:07 +0000

 Module Name:	src
 Committed By:	tsutsui
 Date:		Fri Dec  2 12:43:07 UTC 2016

 Modified Files:
 	src/sys/arch/x68k/x68k: machdep.c

 Log Message:
 Fix crashdump on machines with EXTENDED_MEMORY.

 PR port-x68k/51663 from Rin Okuyama.


 To generate a diff of this commit:
 cvs rdiff -u -r1.193 -r1.194 src/sys/arch/x68k/x68k/machdep.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Soren Jacobsen" <snj@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/51663 CVS commit: [netbsd-7] src/sys/arch/x68k/x68k
Date: Tue, 6 Dec 2016 06:46:02 +0000

 Module Name:	src
 Committed By:	snj
 Date:		Tue Dec  6 06:46:02 UTC 2016

 Modified Files:
 	src/sys/arch/x68k/x68k [netbsd-7]: machdep.c

 Log Message:
 Pull up following revision(s) (requested by tsutsui in ticket #1282):
 	sys/arch/x68k/x68k/machdep.c: revision 1.194
 Fix crashdump on machines with EXTENDED_MEMORY.
 PR port-x68k/51663 from Rin Okuyama.


 To generate a diff of this commit:
 cvs rdiff -u -r1.191.4.1 -r1.191.4.2 src/sys/arch/x68k/x68k/machdep.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: analyzed->closed
State-Changed-By: tsutsui@NetBSD.org
State-Changed-When: Sun, 05 Feb 2017 10:24:48 +0000
State-Changed-Why:
Committed and pulled up to netbsd-7.


>Unformatted:

NetBSD Home
NetBSD PR Database Search

(Contact us) $NetBSD: query-full-pr,v 1.39 2013/11/01 18:47:49 spz Exp $
$NetBSD: gnats_config.sh,v 1.8 2006/05/07 09:23:38 tsutsui Exp $
Copyright © 1994-2014 The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.