Page MenuHomeFreeBSD

libkvm: Fix the kvm_walk_pages in amd64
Needs ReviewPublic

Authored by emaste on Feb 13 2019, 9:21 PM.

Details

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 borako.ozarslan@gmail.com

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

lib/libkvm/kvm_minidump_amd64.c
373 ↗(On Diff #53890)

Whitespace nit: s/){/) {/

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 requested changes to this revision.Oct 11 2019, 5:17 PM

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.

This revision now requires changes to proceed.Oct 11 2019, 5:17 PM
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.

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 updated this revision to Diff 99023.
emaste edited reviewers, added: borako.ozarslan_gmail.com; removed: emaste.

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

lib/libkvm/kvm_private.c
795–796

just 1U I guess, will update locally