svn commit: r325728 - head/lib/libkvm

[Ed Maste]

On Tue, 5 Feb 2019 at 05:17, Bruce Evans <brde at optusnet.com.au> wrote:
>
> On Mon, 4 Feb 2019, Ed Maste wrote:
>
> > On Sat, 11 Nov 2017 at 18:31, Will Andrews <will at 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.