| Message ID | 1362897887-30808-4-git-send-email-yinghai@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, On Sat, Mar 09, 2013 at 10:44:30PM -0800, Yinghai Lu wrote: > Now we have arch_pfn_mapped array, and max_low_pfn_mapped should not > be used anymore. > > User should use arch_pfn_mapped or just 1UL<<(32-PAGE_SHIFT) instead. > > Only user is ACPI_INITRD_TABLE_OVERRIDE, and it should not use that, > as later accessing is using early_ioremap(). Change to try to 4G below > and then 4G above. ... > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index 586e7e9..c08cdb6 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -624,9 +624,13 @@ void __init acpi_initrd_override(void *data, size_t size) > if (table_nr == 0) > return; > > - acpi_tables_addr = > - memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT, > - all_tables_size, PAGE_SIZE); > + /* under 4G at first, then above 4G */ > + acpi_tables_addr = memblock_find_in_range(0, (1ULL<<32) - 1, > + all_tables_size, PAGE_SIZE); > + if (!acpi_tables_addr) > + acpi_tables_addr = memblock_find_in_range(0, > + ~(phys_addr_t)0, > + all_tables_size, PAGE_SIZE); So, it's changing the allocation from <=4G to <=4G first and then >4G. The only explanation given is "as later accessing is using early_ioremap()", but I can't see why that can be a reason for that. early_ioremap() doesn't care whether the given physaddr is under 4G or not, it unconditionally maps it into fixmap, so whether the allocated address is below or above 4G doesn't make any difference. Changing the allowed range of the allocation should be a separate patch. It has some chance of its own breakage and the change itself isn't really related to this one. Please try to elaborate the reasoning behind "why", so that readers of the description don't have to deduce (oh well, guess) your intentions behind the changes. As much as it would help the readers, it'd also help you even more as you would have had to explicitly write something like "the table is accessed with early_ioremap() so the address doesn't need to be restricted under 4G; however, to avoid unnecessary remappings, first try <= 4G and then > 4G." Then, you would be compelled to check whether the statement you explicitly wrote is true, which isn't in this case and you would also realize that the change isn't trivial and doesn't really belong with this patch. By not doing the due diligence, you're offloading what you should have done to others, which isn't very nice. I think the descriptions are better in this posting than the last time but it's still lacking, so, please putfff more effort into describing the changes and reasoning behind them. Thanks.
On Thu, Apr 4, 2013 at 10:36 AM, Tejun Heo <tj@kernel.org> wrote: > Hello, > > On Sat, Mar 09, 2013 at 10:44:30PM -0800, Yinghai Lu wrote: >> Now we have arch_pfn_mapped array, and max_low_pfn_mapped should not >> be used anymore. >> >> User should use arch_pfn_mapped or just 1UL<<(32-PAGE_SHIFT) instead. >> >> Only user is ACPI_INITRD_TABLE_OVERRIDE, and it should not use that, >> as later accessing is using early_ioremap(). Change to try to 4G below >> and then 4G above. > ... >> diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c >> index 586e7e9..c08cdb6 100644 >> --- a/drivers/acpi/osl.c >> +++ b/drivers/acpi/osl.c >> @@ -624,9 +624,13 @@ void __init acpi_initrd_override(void *data, size_t size) >> if (table_nr == 0) >> return; >> >> - acpi_tables_addr = >> - memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT, >> - all_tables_size, PAGE_SIZE); >> + /* under 4G at first, then above 4G */ >> + acpi_tables_addr = memblock_find_in_range(0, (1ULL<<32) - 1, >> + all_tables_size, PAGE_SIZE); >> + if (!acpi_tables_addr) >> + acpi_tables_addr = memblock_find_in_range(0, >> + ~(phys_addr_t)0, >> + all_tables_size, PAGE_SIZE); > > So, it's changing the allocation from <=4G to <=4G first and then >4G. > The only explanation given is "as later accessing is using > early_ioremap()", but I can't see why that can be a reason for that. > early_ioremap() doesn't care whether the given physaddr is under 4G or > not, it unconditionally maps it into fixmap, so whether the allocated > address is below or above 4G doesn't make any difference. > > Changing the allowed range of the allocation should be a separate > patch. It has some chance of its own breakage and the change itself > isn't really related to this one. Ok, will separate that "try above 4G" to another patch. > > Please try to elaborate the reasoning behind "why", so that readers of > the description don't have to deduce (oh well, guess) your intentions > behind the changes. As much as it would help the readers, it'd also > help you even more as you would have had to explicitly write something > like "the table is accessed with early_ioremap() so the address > doesn't need to be restricted under 4G; however, to avoid unnecessary > remappings, first try <= 4G and then > 4G." Then, you would be > compelled to check whether the statement you explicitly wrote is true, > which isn't in this case and you would also realize that the change > isn't trivial and doesn't really belong with this patch. By not doing > the due diligence, you're offloading what you should have done to > others, which isn't very nice. > > I think the descriptions are better in this posting than the last time > but it's still lacking, so, please putfff more effort into describing > the changes and reasoning behind them. ok. Thanks a lot. Yinghai
diff --git a/arch/x86/include/asm/page_types.h b/arch/x86/include/asm/page_types.h index 54c9787..b012b82 100644 --- a/arch/x86/include/asm/page_types.h +++ b/arch/x86/include/asm/page_types.h @@ -43,7 +43,6 @@ extern int devmem_is_allowed(unsigned long pagenr); -extern unsigned long max_low_pfn_mapped; extern unsigned long max_pfn_mapped; static inline phys_addr_t get_max_mapped(void) diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c index 1629577..e75c6e6 100644 --- a/arch/x86/kernel/setup.c +++ b/arch/x86/kernel/setup.c @@ -113,13 +113,11 @@ #include <asm/prom.h> /* - * max_low_pfn_mapped: highest direct mapped pfn under 4GB - * max_pfn_mapped: highest direct mapped pfn over 4GB + * max_pfn_mapped: highest direct mapped pfn * * The direct mapping only covers E820_RAM regions, so the ranges and gaps are * represented by pfn_mapped */ -unsigned long max_low_pfn_mapped; unsigned long max_pfn_mapped; #ifdef CONFIG_DMI diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c index 59b7fc4..abcc241 100644 --- a/arch/x86/mm/init.c +++ b/arch/x86/mm/init.c @@ -313,10 +313,6 @@ static void add_pfn_range_mapped(unsigned long start_pfn, unsigned long end_pfn) nr_pfn_mapped = clean_sort_range(pfn_mapped, E820_X_MAX); max_pfn_mapped = max(max_pfn_mapped, end_pfn); - - if (start_pfn < (1UL<<(32-PAGE_SHIFT))) - max_low_pfn_mapped = max(max_low_pfn_mapped, - min(end_pfn, 1UL<<(32-PAGE_SHIFT))); } bool pfn_range_is_mapped(unsigned long start_pfn, unsigned long end_pfn) diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c index 586e7e9..c08cdb6 100644 --- a/drivers/acpi/osl.c +++ b/drivers/acpi/osl.c @@ -624,9 +624,13 @@ void __init acpi_initrd_override(void *data, size_t size) if (table_nr == 0) return; - acpi_tables_addr = - memblock_find_in_range(0, max_low_pfn_mapped << PAGE_SHIFT, - all_tables_size, PAGE_SIZE); + /* under 4G at first, then above 4G */ + acpi_tables_addr = memblock_find_in_range(0, (1ULL<<32) - 1, + all_tables_size, PAGE_SIZE); + if (!acpi_tables_addr) + acpi_tables_addr = memblock_find_in_range(0, + ~(phys_addr_t)0, + all_tables_size, PAGE_SIZE); if (!acpi_tables_addr) { WARN_ON(1); return;
Now we have arch_pfn_mapped array, and max_low_pfn_mapped should not be used anymore. User should use arch_pfn_mapped or just 1UL<<(32-PAGE_SHIFT) instead. Only user is ACPI_INITRD_TABLE_OVERRIDE, and it should not use that, as later accessing is using early_ioremap(). Change to try to 4G below and then 4G above. -v2: Leave alone max_low_pfn_mapped in i915 code according to tj. Suggested-by: H. Peter Anvin <hpa@zytor.com> Signed-off-by: Yinghai Lu <yinghai@kernel.org> Cc: "Rafael J. Wysocki" <rjw@sisk.pl> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: David Airlie <airlied@linux.ie> Cc: Jacob Shin <jacob.shin@amd.com> Cc: linux-acpi@vger.kernel.org Cc: dri-devel@lists.freedesktop.org --- arch/x86/include/asm/page_types.h | 1 - arch/x86/kernel/setup.c | 4 +--- arch/x86/mm/init.c | 4 ---- drivers/acpi/osl.c | 10 +++++++--- 4 files changed, 8 insertions(+), 11 deletions(-)