On Tue, 5 Feb 2019, Ed Maste wrote:
On Tue, 5 Feb 2019 at 05:17, Bruce Evans <b...@optusnet.com.au> wrote:
On Mon, 4 Feb 2019, Ed Maste wrote:
On Sat, 11 Nov 2017 at 18:31, Will Andrews <w...@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
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
