phys_avail manipulation routine
Needs ReviewPublic
Actions

Authored by johalun0_gmail.com on Oct 5 2018, 1:00 PM.

Details

Reviewers

mmacy
bwidawsk
kib

Summary

phys_avail parts have been moved from https://reviews.freebsd.org/D16719 to this separate diff.

I have refactored things a bit with inspiration from arm's physmem.c. So far only for i386/amd. Let me know what you think. Are the files in the correct places or should kern_physmem.c be in MD folder? (although there would be lots of code duplication). Should we include more archs?

From Linux 4.16 stolen memory detection has been moved to early boot. DRM drivers don't work properly without this and we'd like to have this functionality in 12.0 so that we can update drm drivers beyond 4.15.

Test Plan

Updated tests here: https://onlinegdb.com/ByrZs759m

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

johalun0_gmail.com created this revision.Oct 5 2018, 1:00 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 5 2018, 1:00 PM

johalun0_gmail.com mentioned this in D16719: Add pci_early function for Intel stolen memory (and possibly more).Oct 5 2018, 1:03 PM

zeising added a subscriber: zeising.Oct 5 2018, 1:16 PM

imp added inline comments.Oct 5 2018, 3:42 PM

sys/i386/include/physmem.h
1 ↗	(On Diff #48779)	You should put this file in sys/x86/include and have the canonical one-liners for i386 and amd64.

I think this change must include convertion of some hand-written manipulations of the physmap to use the KPI. I believe I already pointed this out, in particular, look at the allocation of the bootstrap pagemaps for APs.

sys/amd64/include/physmem.h
4 ↗	(On Diff #48779)	This is certainly not true. This file consists of the declarations and symbols copied verbatim from machdep.c and pmap.h At very least you need to preserve the copyright of the author of the lines you copied. Also I doubt that your copyright is suitable there at all. Also, this file looks to be better placed at x86/include.
sys/kern/kern_physmem.c
32	Including sys/types.h is pointless when you include sys/param.h.
33	... and including sys/param.h is pointless when you include sys/systm.h.
41	Now you copied this one more time.
46	Why i needs to be unsigned ? phys_avail is array of the fixed small size. [I do not repeat the notes which are applied more than one time. Check each case yourself]
47	Function must assert that there is space in the phys_avail to move the elements. Function must assert that index has the sane value, at least inside the array range, and much better, inside the valid part of the array.
52	This is better written with for(;;) where previous line moved to for init statement, and i-- is the for step statement.
72	There must be a blank line after locals declarations.
78	I do not believe that this is robust. You probably need to make physmap_idx global and use it as the valid limit.

johalun0_gmail.com marked 13 inline comments as done.Oct 9 2018, 9:11 AM

johalun0_gmail.com added inline comments.

sys/amd64/include/physmem.h
4 ↗	(On Diff #48779)	Yeah this was an oversight. Copyrights fixed and file moved to x86.
sys/kern/kern_physmem.c
33	It seems like you're suggesting that only including systm.h would be enough but that does not compile on my system. Removing one of them can be done though.
41	Cryptic comments aren't really helpful for someone who's still learning... Can you please explain? Regarding the dump_avail array, I wasn't sure if I should let it remain in pmap.h or move it to this new header. I thought moving it here made most sense since PHYSMAP_SIZE is declared in physmem.h.
46	Because negative index is an error. However in this case those functions are private so we can go with int for simplicity.

Patch srat.c to use phys_avail_reserve()

Herald added a subscriber: rpokala. · View Herald TranscriptOct 9 2018, 1:20 PM

johalun0_gmail.com marked an inline comment as done.Oct 23 2018, 3:19 PM

johalun0_gmail.com added inline comments.

sys/kern/kern_physmem.c
78	Do you mean that we expand this KPI to be not only for making holes, but also for inserting memory ranges into phys_avail and make all users of phys_avail use these functions? I think that would be the only way to keep a consistent global index...

kib added inline comments.Oct 23 2018, 3:38 PM

sys/kern/kern_physmem.c
78	Yes, IMO providing the KPI which completely cover all phys_map[] manipulations for slicing and growing. We currently has many places each of which does editing of the array in haphazard way, and this already caused a lot of troubles, see the commit log for mp_machdep.c. I object against adding the KPI as yet another way to modify phys_map[], since it increases the existing mess. Adding this KPI, if done properly (i.e. covering all uses) should both eliminate the continuous source of problems, including hard to diagnose early boot problems, and make the KPI itself robust.

johalun0_gmail.com marked an inline comment as done.Oct 24 2018, 7:59 AM

johalun0_gmail.com added inline comments.

sys/kern/kern_physmem.c
78	Yeah that makes sense but will grow this project way bigger than what I have time to do for 12. In this case, I suggest we omit the phys_avail part from https://reviews.freebsd.org/D16719 so we can get that committed and re-enabled it once this code is in. At least then we can get graphics drivers working again. We still won't be guaranteed to have the memory reserved in case of a buggy bios but that would be the same if we detect stolen memory in the drm driver which would be the other alternative.

kib added inline comments.Oct 24 2018, 6:34 PM

sys/kern/kern_physmem.c
78	And how this can work without phys_avail manipulations ? Anyway, show the patch, then it would be easier to understand the proposal and to discuss it.