libkvm: Fix the kvm_walk_pages in amd64

[emaste]

Summary

kvm_walk_pages did not work at all on amd64. Fixed it so that it correctly accesses the bitmap instead of going past it's length.

submitted by: Bora Ozarslan [e-mail]

[jhb]

lib/libkvm/kvm_minidump_amd64.c

373 ↗

(On Diff #53890)

Whitespace nit: s/){/) {/

[jhb]

Will should really review this since he added this API.

For the first part of the the hunk, the current code invokes a single callback for each superpage passing in the length of the superpage. The patch just splits this up into 512 4K calls instead. I think the old code there is probably more correct and efficient even though the comment is stale and implies the loop used by the new code. I think the kvm_bitmap_next changes are the actual fixes?

[will]

Thanks for the add!

In D19183#479806, @jhb wrote:

For the first part of the the hunk, the current code invokes a single callback for each superpage passing in the length of the superpage. The patch just splits this up into 512 4K calls instead. I think the old code there is probably more correct and efficient even though the comment is stale and implies the loop used by the new code.

Yes, the comment is obviously stale, from a much earlier version of the implementation. I think the patch should instead fix the comment. Requesting this change.

I think the kvm_bitmap_next changes are the actual fixes?

Yes. I think this part of the change is correct (though I prefer the constant vs bare 8s). *idx and first_invalid were meant to use bits as units. It seems the correctness of the previous version was not properly validated. Thanks!

lib/libkvm/kvm_private.c

786

Why? The constant makes clear this number is derived from the number of bits in an bm->map.

[jhb]

lib/libkvm/kvm_private.c

786

I almost made the same comment (in the name of having a smaller diff), but then I noticed that _kvm_bitmap_set() uses bare 8's.

[will]

lib/libkvm/kvm_private.c

786

Good point, but if we are changing anything here, I'd fix that usage to match, rather than the other way around.

[emaste]

See also D21554

[emaste]

Limit to just the _kvm_bitmap_next changes, restoring use of CHAR_BIT.
Comment can be fixed in a separate change.

[emaste]

lib/libkvm/kvm_private.c

795–796

just 1U I guess, will update locally

[emaste]

1U