lkml.org 
[lkml]   [2012]   [Aug]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>



\
 
 \ /
  Last update: 2012-08-13 17:23    [W:0.137 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site