Skip site navigation (1)Skip section navigation (2)
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>