Date: Tue, 5 Feb 2019 09:38:54 -0500 From: Ed Maste <emaste@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: Will Andrews <will@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r325728 - head/lib/libkvm Message-ID: <CAPyFy2C%2BZ0aOGUBbgpLZ8sJbaN2YhEbR1YkV9Ya7POTSsbLqGQ@mail.gmail.com> In-Reply-To: <20190205202145.A1080@besplex.bde.org> References: <201711112330.vABNUwXC077395@repo.freebsd.org> <CAPyFy2BwNGHkMjj2rG5N5S=7E8N=9jfAUBki1L8eCY3kMeM8fw@mail.gmail.com> <20190205202145.A1080@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 5 Feb 2019 at 05:17, Bruce Evans <brde@optusnet.com.au> wrote: > > On Mon, 4 Feb 2019, Ed Maste wrote: > > > On Sat, 11 Nov 2017 at 18:31, Will Andrews <will@freebsd.org> wrote: > >> > >> Author: will > >> Date: Sat Nov 11 23:30:58 2017 > >> New Revision: 325728 > >> URL: https://svnweb.freebsd.org/changeset/base/325728 > >> > >> Log: > >> libkvm: add kvm_walk_pages API. (Replying here only to the comments on the issues I brought up.) >>> + u_long paddr; > > Further pollution in the type and struct member names. Further misformatting > > The include of sys/_types.h was apparently changed to sys/types.h to support > using u_long. If we change the types then we can presumably revert that part. > > This should probably be uin64_t to support cross-debugging cores from > > 64-bit machines on 32-bit hosts; also for i386 PAE. Or, on IRC jhb > > suggested we introduce a kpaddr_t typedef akin to kvaddr_t. > > The correct type is vm_paddr_t or maybe a kvm wrapper of this. That precludes cross-arch and cross-size use of kvm_walk_pages; kvm supports this use (see kvm_read2) so it should be a 64-bit kvm wrapper. > >> + u_long kmap_vaddr; > >> + u_long dmap_vaddr; > > > > These two should be kvaddr_t. > > Further pollution and style bugs in names, types and formatting. Somewhat difficult to change now though... but what about: struct kvm_page { u_int kp_version; kpaddr_t kp_paddr; kvaddr_t kp_kmap_vaddr; kvaddr_t kp_dmap_vaddr; vm_prot_t kp_prot; off_t kp_offset; size_t kp_len; }; > [kvaddr_t] is currently hard-coded as __uint64_t. That works for all supported > arches now, but eventually some typedefs will actually be needed for their > purpose of being implementation-depended and changeable. Except that these should be MI for cross-debugging. > >> + vm_prot_t prot; > >> + u_long offset; > > > > off_t? > > Further pollution and style bugs in names, types and formatting. > > Maybe uoff_t. off_t is 64-bits signed so can't reach most kernel addresses > on some 64-bit arches like amd64. I believe the offset here is the offset of the page in the vmcore file, so off_t should be appropriate. > >> + size_t len; > > Further pollution and style bugs 1 name and formatting. Off hand I'm not sure of the appropriate type for a MI size; in practice now though this len will be page size.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAPyFy2C%2BZ0aOGUBbgpLZ8sJbaN2YhEbR1YkV9Ya7POTSsbLqGQ>