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
Details
Details
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| lib/libkvm/kvm_minidump_amd64.c | ||
|---|---|---|
| 373 ↗ | (On Diff #53890) | Whitespace nit: s/){/) {/ |
Comment Actions
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?
Comment Actions
Thanks for the add!
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. | |
| 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. | |
Comment Actions
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 | |