Changeset View
Standalone View
sys/kern/kern_physmem.c
- This file was added.
/*- | |||||
* SPDX-License-Identifier: BSD-2-Clause-FreeBSD | |||||
* | |||||
* Copyright (c) 2018 Johannes Lundberg | |||||
* | |||||
* Redistribution and use in source and binary forms, with or without | |||||
* modification, are permitted provided that the following conditions | |||||
* are met: | |||||
* 1. Redistributions of source code must retain the above copyright | |||||
* notice, this list of conditions and the following disclaimer. | |||||
* 2. Redistributions in binary form must reproduce the above copyright | |||||
* notice, this list of conditions and the following disclaimer in the | |||||
* documentation and/or other materials provided with the distribution. | |||||
* | |||||
* THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND | |||||
* ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |||||
* IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE | |||||
* ARE DISCLAIMED. IN NO EVENT SHALL THE AUTHOR OR CONTRIBUTORS BE LIABLE | |||||
* FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |||||
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS | |||||
* OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) | |||||
* HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT | |||||
* LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY | |||||
* OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF | |||||
* SUCH DAMAGE. | |||||
*/ | |||||
#include <sys/cdefs.h> | |||||
__FBSDID("$FreeBSD$"); | |||||
#include <sys/types.h> | |||||
#include <sys/systm.h> | |||||
kib: Including sys/types.h is pointless when you include sys/param.h. | |||||
#include <x86/physmem.h> | |||||
Done Inline Actions... and including sys/param.h is pointless when you include sys/systm.h. kib: ... and including sys/param.h is pointless when you include sys/systm.h. | |||||
Done Inline ActionsIt 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. johalun0_gmail.com: It seems like you're suggesting that only including systm.h would be enough but that does not… | |||||
static int phys_avail_free_count(void); | |||||
static void phys_avail_insert_at(int index, vm_paddr_t val); | |||||
static void phys_avail_delete_at(int index); | |||||
vm_paddr_t phys_avail[PHYSMAP_SIZE + 2]; | |||||
vm_paddr_t dump_avail[PHYSMAP_SIZE + 2]; | |||||
Not Done Inline ActionsNow you copied this one more time. kib: Now you copied this one more time. | |||||
Done Inline ActionsCryptic 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. johalun0_gmail.com: Cryptic comments aren't really helpful for someone who's still learning... Can you please… | |||||
static int | |||||
phys_avail_free_count(void) | |||||
{ | |||||
int i; | |||||
Done Inline ActionsWhy 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] kib: Why i needs to be unsigned ? phys_avail is array of the fixed small size.
[I do not repeat… | |||||
Done Inline ActionsBecause negative index is an error. However in this case those functions are private so we can go with int for simplicity. johalun0_gmail.com: Because negative index is an error. However in this case those functions are private so we can… | |||||
for (i = PHYS_AVAIL_ARRAY_END; i > 0 && | |||||
Done Inline ActionsFunction must assert that there is space in the phys_avail to move the elements. kib: Function must assert that there is space in the phys_avail to move the elements.
Function must… | |||||
phys_avail[i - 1] == 0 && phys_avail[i] == 0; i -= 2); | |||||
return (PHYS_AVAIL_ARRAY_END - i); | |||||
} | |||||
Done Inline ActionsThis is better written with for(;;) where previous line moved to for init statement, and i-- is the for step statement. kib: This is better written with for(;;) where previous line moved to for init statement, and i-- is… | |||||
static void | |||||
phys_avail_insert_at(int index, vm_paddr_t val) | |||||
{ | |||||
int i; | |||||
KASSERT(phys_avail_free_count() > 0, | |||||
("phys_avail_insert_at: phys_avail has no more free slots")); | |||||
KASSERT(index >= 0 && index <= PHYS_AVAIL_ARRAY_END, | |||||
("phys_avail_insert_at: index %d is out of range", index)); | |||||
for (i = PHYS_AVAIL_ARRAY_END; i >= index; i--) { | |||||
phys_avail[i + 1] = phys_avail[i]; | |||||
} | |||||
phys_avail[i + 1] = val; | |||||
} | |||||
static void | |||||
phys_avail_delete_at(int index) | |||||
{ | |||||
int i; | |||||
Done Inline ActionsThere must be a blank line after locals declarations. kib: There must be a blank line after locals declarations. | |||||
KASSERT(index >= 0 && index <= PHYS_AVAIL_ARRAY_END, | |||||
("phys_avail_delete_at: index %d is out of range", index)); | |||||
i = index; | |||||
while (i <= PHYS_AVAIL_ARRAY_END) { | |||||
Not Done Inline ActionsI do not believe that this is robust. You probably need to make physmap_idx global and use it as the valid limit. kib: I do not believe that this is robust. You probably need to make physmap_idx global and use it… | |||||
Done Inline ActionsDo 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... johalun0_gmail.com: Do you mean that we expand this KPI to be not only for making holes, but also for inserting… | |||||
Not Done Inline ActionsYes, 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. kib: Yes, IMO providing the KPI which completely cover all phys_map[] manipulations for slicing and… | |||||
Done Inline ActionsYeah 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. johalun0_gmail.com: Yeah that makes sense but will grow this project way bigger than what I have time to do for 12. | |||||
Not Done Inline ActionsAnd 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. kib: And how this can work without phys_avail manipulations ? Anyway, show the patch, then it would… | |||||
phys_avail[i] = phys_avail[i + 1]; | |||||
i++; | |||||
} | |||||
} | |||||
void | |||||
phys_avail_reserve(vm_paddr_t start, vm_paddr_t end) | |||||
{ | |||||
int deleted, i; | |||||
KASSERT(start < end, ("phys_avail_reserve: invalid range start = " | |||||
"0x%016jx, end = 0x%016jx", (uintmax_t)start, (uintmax_t)end)); | |||||
/* Check if new hole is before first segment. */ | |||||
if (end <= phys_avail[0]) | |||||
return; | |||||
i = 0; | |||||
/* Check if between segments (inside existing hole) */ | |||||
while (phys_avail[i + 2] != 0 && phys_avail[i + 3] != 0) { | |||||
if (start >= phys_avail[i + 1] && end <= phys_avail[i + 2]) | |||||
return; | |||||
i += 2; | |||||
} | |||||
/* Check if after last segment end (i+1). */ | |||||
if (start >= phys_avail[i + 1]) | |||||
return; | |||||
i=0; | |||||
/* Check if inside any segment */ | |||||
while (phys_avail[i + 1] != 0) { | |||||
if (start > phys_avail[i] && end < phys_avail[i + 1]) { | |||||
phys_avail_insert_at(i + 1, end); | |||||
phys_avail_insert_at(i + 1, start); | |||||
return; | |||||
} | |||||
i += 2; | |||||
} | |||||
/* Other cases */ | |||||
i=0; | |||||
while (phys_avail[i] < start && i <= PHYS_AVAIL_ARRAY_END) { | |||||
i++; | |||||
} | |||||
if (phys_avail[i + 1] == 0) { | |||||
/* The new hole starts in the last segment and ends after. */ | |||||
phys_avail[i] = start; | |||||
return; | |||||
} | |||||
/* At the index located on or after the new hole start. */ | |||||
if (i % 2 == 0) { | |||||
/* | |||||
* Even index mean beginning of a segment. If hole ends in | |||||
* segment, shrink the segment. If exact match, delete the | |||||
* segment. Other cases are handled below. | |||||
*/ | |||||
if (start == phys_avail[i]) { | |||||
if (end < phys_avail[i + 1]) { | |||||
phys_avail[i] = end; | |||||
return; | |||||
} else if (end == phys_avail[i + 1]) { | |||||
phys_avail_delete_at(i); | |||||
phys_avail_delete_at(i); | |||||
return; | |||||
} | |||||
} | |||||
} else { | |||||
/* | |||||
* Odd index mean the hole starts inside a memory segment. | |||||
* Shrink segment to hole start. | |||||
*/ | |||||
phys_avail[i] = start; | |||||
i++; | |||||
} | |||||
deleted = 0; | |||||
while (phys_avail[i] <= end && phys_avail[i + 1] != 0) { | |||||
phys_avail_delete_at(i); | |||||
deleted++; | |||||
} | |||||
/* At the index located after the new hole end. */ | |||||
if (i % 2 == 0 && deleted % 2 == 1) { | |||||
/* | |||||
* If index is even and odd number of entries have been deleted, | |||||
* we are at the end of a segment. Insert hole end before this | |||||
* segment end. | |||||
*/ | |||||
phys_avail_insert_at(i, end); | |||||
} else if (i % 2 == 1) { | |||||
/* | |||||
* Odd index mean our hole ends in the previous segment, | |||||
* move segment start to new hole end. | |||||
*/ | |||||
phys_avail[i - 1] = start; | |||||
} | |||||
} |
Including sys/types.h is pointless when you include sys/param.h.