From owner-svn-src-head@freebsd.org Tue Feb 5 16:25:28 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1B4B014BEC28; Tue, 5 Feb 2019 16:25:28 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail104.syd.optusnet.com.au (mail104.syd.optusnet.com.au [211.29.132.246]) by mx1.freebsd.org (Postfix) with ESMTP id 5B7C8824AB; Tue, 5 Feb 2019 16:25:26 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail104.syd.optusnet.com.au (Postfix) with ESMTPS id B26C643277B; Wed, 6 Feb 2019 03:25:23 +1100 (AEDT) Date: Wed, 6 Feb 2019 03:25:21 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Ed Maste cc: Bruce Evans , Will Andrews , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r325728 - head/lib/libkvm In-Reply-To: Message-ID: <20190206024025.X3175@besplex.bde.org> References: <201711112330.vABNUwXC077395@repo.freebsd.org> <20190205202145.A1080@besplex.bde.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=UJetJGXy c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=PO7r1zJSAAAA:8 a=6I5d2MoRAAAA:8 a=Nm4g2UKS2DVHwyA5FXEA:9 a=3MUEs-bSANMeB8kN:21 a=bFoZouqJEdPud0MP:21 a=CjuIK1q_8ugA:10 a=IjZwj45LgO3ly-622nXo:22 X-Rspamd-Queue-Id: 5B7C8824AB X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.94 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.94)[-0.941,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Feb 2019 16:25:28 -0000 On Tue, 5 Feb 2019, Ed Maste wrote: > On Tue, 5 Feb 2019 at 05:17, Bruce Evans wrote: >> >> On Mon, 4 Feb 2019, Ed Maste wrote: >> >>> On Sat, 11 Nov 2017 at 18:31, Will Andrews 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 . 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