svn commit: r325728 - head/lib/libkvm

[Bruce Evans]

On Tue, 5 Feb 2019, Ed Maste wrote:

> 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.

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