| Message ID | 20211206072337.9517-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mini-os: add missing PVH features | expand |
Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit: > - unsigned long pfn, max = 0; > + unsigned long pfns, max = 0; I'd say rather rename max to start. > e820_get_memmap(); > > @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void) > { > if ( e820_map[i].type != E820_RAM ) > continue; > - pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT; > - if ( pfn > max ) > - max = pfn; > + pfns = e820_map[i].size >> PAGE_SHIFT; > + max = e820_map[i].addr >> PAGE_SHIFT; since it's it's always the start of the e820 entry. > + if ( pages <= pfns ) > + return max + pages; > + pages -= pfns; > + max += pfns; Here we don't need do change max, only pages. Samuel
On 12.12.21 01:15, Samuel Thibault wrote: > Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit: >> - unsigned long pfn, max = 0; >> + unsigned long pfns, max = 0; > > I'd say rather rename max to start. > >> e820_get_memmap(); >> >> @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void) >> { >> if ( e820_map[i].type != E820_RAM ) >> continue; >> - pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT; >> - if ( pfn > max ) >> - max = pfn; >> + pfns = e820_map[i].size >> PAGE_SHIFT; >> + max = e820_map[i].addr >> PAGE_SHIFT; > > since it's it's always the start of the e820 entry. > >> + if ( pages <= pfns ) >> + return max + pages; >> + pages -= pfns; >> + max += pfns; > > Here we don't need do change max, only pages. It is needed in case the loop is finished. And this was the reason for naming it max. Juergen
Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit: > On 12.12.21 01:15, Samuel Thibault wrote: > > Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit: > > > - unsigned long pfn, max = 0; > > > + unsigned long pfns, max = 0; > > > > I'd say rather rename max to start. > > > > > e820_get_memmap(); > > > @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void) > > > { > > > if ( e820_map[i].type != E820_RAM ) > > > continue; > > > - pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT; > > > - if ( pfn > max ) > > > - max = pfn; > > > + pfns = e820_map[i].size >> PAGE_SHIFT; > > > + max = e820_map[i].addr >> PAGE_SHIFT; > > > > since it's it's always the start of the e820 entry. > > > > > + if ( pages <= pfns ) > > > + return max + pages; > > > + pages -= pfns; > > > + max += pfns; > > > > Here we don't need do change max, only pages. > > It is needed in case the loop is finished. > > And this was the reason for naming it max. Ah, ok. At first read the name was confusing me. Perhaps better use two variables then: start and max, so that we have start = e820_map[i].addr >> PAGE_SHIFT; if ( pages <= pfns ) return start + pages; pages -= pfns; max = start + pfns; Samuel
On 13.12.21 22:22, Samuel Thibault wrote: > Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit: >> On 12.12.21 01:15, Samuel Thibault wrote: >>> Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit: >>>> - unsigned long pfn, max = 0; >>>> + unsigned long pfns, max = 0; >>> >>> I'd say rather rename max to start. >>> >>>> e820_get_memmap(); >>>> @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void) >>>> { >>>> if ( e820_map[i].type != E820_RAM ) >>>> continue; >>>> - pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT; >>>> - if ( pfn > max ) >>>> - max = pfn; >>>> + pfns = e820_map[i].size >> PAGE_SHIFT; >>>> + max = e820_map[i].addr >> PAGE_SHIFT; >>> >>> since it's it's always the start of the e820 entry. >>> >>>> + if ( pages <= pfns ) >>>> + return max + pages; >>>> + pages -= pfns; >>>> + max += pfns; >>> >>> Here we don't need do change max, only pages. >> >> It is needed in case the loop is finished. >> >> And this was the reason for naming it max. > > Ah, ok. > > At first read the name was confusing me. Perhaps better use two > variables then: start and max, so that we have > > start = e820_map[i].addr >> PAGE_SHIFT; > if ( pages <= pfns ) > return start + pages; > pages -= pfns; > max = start + pfns; Hmm, or I can rename max to start, drop the "max += pfns;" and do a "return start + pfns;" at the end of the function. Juergen
Juergen Gross, le mar. 14 déc. 2021 07:35:54 +0100, a ecrit: > On 13.12.21 22:22, Samuel Thibault wrote: > > Juergen Gross, le lun. 13 déc. 2021 15:58:58 +0100, a ecrit: > > > On 12.12.21 01:15, Samuel Thibault wrote: > > > > Juergen Gross, le lun. 06 déc. 2021 08:23:30 +0100, a ecrit: > > > > > - unsigned long pfn, max = 0; > > > > > + unsigned long pfns, max = 0; > > > > > > > > I'd say rather rename max to start. > > > > > > > > > e820_get_memmap(); > > > > > @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void) > > > > > { > > > > > if ( e820_map[i].type != E820_RAM ) > > > > > continue; > > > > > - pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT; > > > > > - if ( pfn > max ) > > > > > - max = pfn; > > > > > + pfns = e820_map[i].size >> PAGE_SHIFT; > > > > > + max = e820_map[i].addr >> PAGE_SHIFT; > > > > > > > > since it's it's always the start of the e820 entry. > > > > > > > > > + if ( pages <= pfns ) > > > > > + return max + pages; > > > > > + pages -= pfns; > > > > > + max += pfns; > > > > > > > > Here we don't need do change max, only pages. > > > > > > It is needed in case the loop is finished. > > > > > > And this was the reason for naming it max. > > > > Ah, ok. > > > > At first read the name was confusing me. Perhaps better use two > > variables then: start and max, so that we have > > > > start = e820_map[i].addr >> PAGE_SHIFT; > > if ( pages <= pfns ) > > return start + pages; > > pages -= pfns; > > max = start + pfns; > > Hmm, or I can rename max to start, drop the "max += pfns;" and do a > "return start + pfns;" at the end of the function. That could do as well, yes. Samuel
diff --git a/arch/x86/mm.c b/arch/x86/mm.c index 8df93da..3bf6170 100644 --- a/arch/x86/mm.c +++ b/arch/x86/mm.c @@ -107,7 +107,6 @@ void arch_mm_preinit(void *p) { long ret; domid_t domid = DOMID_SELF; - unsigned long max; pt_base = page_table_base; first_free_pfn = PFN_UP(to_phys(&_end)); @@ -117,11 +116,8 @@ void arch_mm_preinit(void *p) xprintk("could not get memory size\n"); do_exit(); } - last_free_pfn = ret; - max = e820_get_maxpfn(); - if ( max < last_free_pfn ) - last_free_pfn = max; + last_free_pfn = e820_get_maxpfn(ret); } #endif diff --git a/e820.c b/e820.c index 336a8b8..14fd3cd 100644 --- a/e820.c +++ b/e820.c @@ -155,10 +155,10 @@ void arch_print_memmap(void) } #endif -unsigned long e820_get_maxpfn(void) +unsigned long e820_get_maxpfn(unsigned long pages) { int i; - unsigned long pfn, max = 0; + unsigned long pfns, max = 0; e820_get_memmap(); @@ -166,9 +166,12 @@ unsigned long e820_get_maxpfn(void) { if ( e820_map[i].type != E820_RAM ) continue; - pfn = (e820_map[i].addr + e820_map[i].size) >> PAGE_SHIFT; - if ( pfn > max ) - max = pfn; + pfns = e820_map[i].size >> PAGE_SHIFT; + max = e820_map[i].addr >> PAGE_SHIFT; + if ( pages <= pfns ) + return max + pages; + pages -= pfns; + max += pfns; } return max; diff --git a/include/e820.h b/include/e820.h index af2129f..6a57f05 100644 --- a/include/e820.h +++ b/include/e820.h @@ -49,6 +49,6 @@ struct __packed e820entry { extern struct e820entry e820_map[]; extern unsigned e820_entries; -unsigned long e820_get_maxpfn(void); +unsigned long e820_get_maxpfn(unsigned long pages); #endif /*__E820_HEADER*/
Sizing the available memory should respect memory holes, so look at the memory map when setting the boundary for the memory allocator. Signed-off-by: Juergen Gross <jgross@suse.com> --- arch/x86/mm.c | 6 +----- e820.c | 13 ++++++++----- include/e820.h | 2 +- 3 files changed, 10 insertions(+), 11 deletions(-)