Re: [PATCH 2/3] x86: add a is_e820_ram() helper

[Yinghai Lu <yinghai@kernel.org>]

On Thu, Mar 26, 2015 at 2:34 AM, Christoph Hellwig <hch@lst.de> wrote:
> On Thu, Mar 26, 2015 at 10:02:15AM +0100, Ingo Molnar wrote:
>> This is_e820_ram() factoring out becomes really messy in patch #3.
...
> Does this patch (replaces patches 2 and 3) look better to you?
>
> ---
> From 4a6fdc8433559d649d7bf707f72aafa4488f2d23 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Wed, 25 Mar 2015 12:24:11 +0100
> Subject: x86: add support for the non-standard protected e820 type
>
> Various recent BIOSes support NVDIMMs or ADR using a non-standard
> e820 memory type, and Intel supplied reference Linux code using this
> type to various vendors.
>
> Wire this e820 table type up to export platform devices for the pmem
> driver so that we can use it in Linux, and also provide a memmap=
> argument to manually tag memory as protected, which can be used
> if the BIOSs doesn't use the standard nonstandard interface, or
> we just want to test the pmem driver with regular memory.
>
> Based on an earlier patch from Dave Jiang <dave.jiang@intel.com>
>
...
> diff --git a/arch/x86/kernel/pmem.c b/arch/x86/kernel/pmem.c
> new file mode 100644
> index 0000000..f970048
> --- /dev/null
> +++ b/arch/x86/kernel/pmem.c
...
> +
> +void __init reserve_pmem(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < e820.nr_map; i++) {
> +               struct e820entry *ei = &e820.map[i];
> +
> +               if (ei->type != E820_PRAM)
> +                       continue;
> +
> +               memblock_reserve(ei->addr, ei->addr + ei->size);
> +               max_pfn_mapped = init_memory_mapping(
> +                               ei->addr < 1UL << 32 ? 1UL << 32 : ei->addr,
> +                               ei->addr + ei->size);
> +       }
> +}

What do you want to get here?

You did not modify memblock_x86_fill() to treat
E820_PRAM as E820_RAM, so memblock will not have any
entry for E820_PRAM, so you do not need to call memblock_reserve
there.

And the same time,  init_memory_mapping() will call
init_range_memory_mapping/for_each_mem_pfn_range() to
set kernel mapping for memory range in memblock only.
So here calling init_memory_mapping will not do anything.
then just drop calling to that init_memory_mapping.
--- so will not kernel mapping pmem, is that what you intended to have?

After those two changes, You do not need this reserve_pmem at all.
Just drop it.


> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 0a2421c..f2bed2b 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1158,6 +1158,8 @@ void __init setup_arch(char **cmdline_p)
>
>         early_acpi_boot_init();
>
> +       reserve_pmem();
> +

Not needed.

>         initmem_init();
>         dma_contiguous_reserve(max_pfn_mapped << PAGE_SHIFT);

Thanks

Yinghai