Messages in this thread | | | Date | Mon, 13 Aug 2012 09:31:59 -0500 | From | Jacob Shin <> | Subject | Re: [PATCH 1/5] x86: Only direct map addresses that are marked as E820_RAM |
| |
On Sat, Aug 11, 2012 at 12:49:48PM -0700, Tejun Heo wrote: > Hello, Jacob.
Hi,
> > On Thu, Aug 09, 2012 at 04:23:05PM -0500, Jacob Shin wrote: > > +struct range pfn_mapped[E820_X_MAX]; > > +int nr_pfn_mapped; > > Why aren't these __initdata? Are they gonna be used for other > purposes?
Yes, the thought was that later code may want to know what pfns are direct mapped or not. For example, memory hotplug has to call init_memory_mapping and updates direct mapping.
> > > +void add_pfn_range_mapped(unsigned long start_pfn, unsigned long end_pfn) > > +{ > > + nr_pfn_mapped = add_range_with_merge(pfn_mapped, E820_X_MAX, > > + nr_pfn_mapped, start_pfn, end_pfn); > > + > > + if (end_pfn > max_pfn_mapped) > > + max_pfn_mapped = end_pfn; > > Maybe use max()?
Okay,
> > > + if ((end_pfn <= (1UL << (32 - PAGE_SHIFT))) && > > + (end_pfn > max_low_pfn_mapped)) > > + max_low_pfn_mapped = end_pfn; > > +} > > + > > +int pfn_range_is_mapped(unsigned long start_pfn, unsigned long end_pfn) > > bool?
Okay, will change to bool.
> > > +{ > > + int i; > > + > > + for (i = 0; i < nr_pfn_mapped; i++) > > + if ((start_pfn >= pfn_mapped[i].start) && > > + (end_pfn <= pfn_mapped[i].end)) > > + break; > > + > > + return i < nr_pfn_mapped; > > +} > > for (...) > if (xxx) > return true; > return false; > > > +int pfn_is_mapped(unsigned long pfn) > > +{ > > + int i; > > + > > + for (i = 0; i < nr_pfn_mapped; i++) > > + if ((pfn >= pfn_mapped[i].start) && > > + (pfn < pfn_mapped[i].end)) > > + break; > > + > > + return i < nr_pfn_mapped; > > +} > > How about... > > return pfn_range_is_mapped(pfn, pfn + 1);
Okay,
> > > @@ -913,14 +958,40 @@ void __init setup_arch(char **cmdline_p) > > > > init_gbpages(); > > > > - /* max_pfn_mapped is updated here */ > > - max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT); > > - max_pfn_mapped = max_low_pfn_mapped; > > + init_pfn = max_pfn_mapped; > > + > > + memset(pfn_mapped, 0, sizeof(pfn_mapped)); > > + nr_pfn_mapped = 0; > > Are these necessary? We clear .bss way before control reaches here.
Ah okay, I'll remove them, and test to double check.
> > > + > > + add_pfn_range_mapped(0, max_pfn_mapped); > > + > > + for (i = 0; i < e820.nr_map; i++) { > > + struct e820entry *ei = &e820.map[i]; > > + u64 start = ei->addr; > > + u64 end = ei->addr + ei->size; > > + > > + if (ei->type != E820_RAM) > > + continue; > > + > > + if (end <= (init_pfn << PAGE_SHIFT)) > > + continue; > > + > > + if (start < (init_pfn << PAGE_SHIFT)) > > + start = init_pfn << PAGE_SHIFT; > > + > > +#ifdef CONFIG_X86_32 > > + if ((start >> PAGE_SHIFT) >= max_low_pfn) > > + continue; > > + > > + if ((end >> PAGE_SHIFT) > max_low_pfn) > > + end = max_low_pfn << PAGE_SHIFT; > > +#endif > > + pfn = init_memory_mapping(start, end); > > + add_pfn_range_mapped(start >> PAGE_SHIFT, pfn); > > + } > > Some comments please? Also, while this may be the right thing to do, > if I'm not mistaken, this is also likely to make linear space to use > smaller mappings depending on how the physical memory is laid out, > which could be a trade off we're willing to make, but that *should* be > explicit. Please describe what's going on and provide rationale.
Ah .. okay, so you are concerned about BIOSes with E820 that break up a large linear memory range into 2 different E820 entries? But if I'm not mistaken, the E820 code does some cleansing of the values it gets from the BIOS, in arch/x86/kernel/e820.c: sanitize_e820_map
But yes, I'll add comments, as well as break this logic out to its own function as Yinghai suggested.
Thanks!
-Jacob
> > Thanks. > > -- > tejun >
| |