Date: Wed, 6 Feb 2019 03:25:21 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Maste <emaste@freebsd.org> Cc: Bruce Evans <brde@optusnet.com.au>, 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: <20190206024025.X3175@besplex.bde.org> In-Reply-To: <CAPyFy2C%2BZ0aOGUBbgpLZ8sJbaN2YhEbR1YkV9Ya7POTSsbLqGQ@mail.gmail.com> References: <201711112330.vABNUwXC077395@repo.freebsd.org> <CAPyFy2BwNGHkMjj2rG5N5S=7E8N=9jfAUBki1L8eCY3kMeM8fw@mail.gmail.com> <20190205202145.A1080@besplex.bde.org> <CAPyFy2C%2BZ0aOGUBbgpLZ8sJbaN2YhEbR1YkV9Ya7POTSsbLqGQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 5 Feb 2019, Ed Maste wrote: > 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. It would need the kva wrapper type in sys/_types.h. In fact, older, more standard types are already declared there, but with underscores as needed to have no pollution in there. sys/types.h and some other files turn the underscored versions into non-underscored versions. >>> 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. kvm or system? kvaddr_t is system and the corresponding physical address type should probably be system too. The name kvaddr_t is not very good. kva is a good abbreviation, and kva_ is a good prefix. kvaddr is not so good for either. We now want a physical address type and its matching name is kpaddr_t, but that means that the prefix is just k. > >>>> + 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; > }; This should have tabs after 3 shorter type names. Signed kp_offset seems wrong. Apart from it not reaching the top of 64- bit address spaces, adding unsigned kp_len to it gives an unsigned type. u_int, vm_prot_t and size_t give sparse packing with binary incompatibilities. vm_prot_t is 8 bits, so there there is 7 bytes of padding after kp_prot on amd64 and only 3 bytes on i386. 4 or 0 bytes of padding after kp_version and kp_len. This is hard to fix now. But you already changed the ABI on i386 by expanding all the u_long's. >> [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. That is spelled vm_ooffset_t in the kernel. This was MD in theory before r313194 2 years ago, but it was always 64 bits signed ad was made MI to give a consistent ABI. The MD version had less pollution than the MI version -- it gave an underscored version that was available without including <sys/types.h>. It was still hard to remember to use it instead of off_t. Then it was changed to uint64_t in r341398 for much the same reason as one of me reasons above -- most uses of it convert it to an unsigned type (sometimes by unsigned poisoning). So vm_ooffset_t is 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. I also don't like most uses of size_t, especially in ABIs. Many are for values that are sure to be small. Small values can be packed into uint32_t or smaller. If the size is unlimited, use uint64_t. The address space might be unlimited, but 64 bits for a single (non-sparse) object is preposterously large. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190206024025.X3175>