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

[Ingo Molnar <mingo@kernel.org>]

* Christoph Hellwig <hch@lst.de> wrote:

> This will allow to deal with persistent memory which needs to be
> treated like ram in many, but not all cases.
> 
> Based on an earlier patch from Dave Jiang <dave.jiang@intel.com>.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  arch/x86/kernel/e820.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 46201de..de088e3 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -48,6 +48,11 @@ unsigned long pci_mem_start = 0xaeedbabe;
>  EXPORT_SYMBOL(pci_mem_start);
>  #endif
>  
> +static inline bool is_e820_ram(__u32 type)
> +{
> +	return type == E820_RAM;
> +}
> +
>  /*
>   * This function checks if any part of the range <start,end> is mapped
>   * with type.
> @@ -688,7 +693,7 @@ void __init e820_mark_nosave_regions(unsigned long limit_pfn)
>  			register_nosave_region(pfn, PFN_UP(ei->addr));
>  
>  		pfn = PFN_DOWN(ei->addr + ei->size);
> -		if (ei->type != E820_RAM && ei->type != E820_RESERVED_KERN)
> +		if (!is_e820_ram(ei->type) && ei->type != E820_RESERVED_KERN)
>  			register_nosave_region(PFN_UP(ei->addr), pfn);
>  
>  		if (pfn >= limit_pfn)
> @@ -748,7 +753,7 @@ u64 __init early_reserve_e820(u64 size, u64 align)
>  /*
>   * Find the highest page frame number we have available
>   */
> -static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
> +static unsigned long __init e820_end_pfn(unsigned long limit_pfn)
>  {
>  	int i;
>  	unsigned long last_pfn = 0;
> @@ -759,7 +764,7 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
>  		unsigned long start_pfn;
>  		unsigned long end_pfn;
>  
> -		if (ei->type != type)
> +		if (!is_e820_ram(ei->type))
>  			continue;
>  
>  		start_pfn = ei->addr >> PAGE_SHIFT;
> @@ -784,12 +789,12 @@ static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
>  }
>  unsigned long __init e820_end_of_ram_pfn(void)
>  {
> -	return e820_end_pfn(MAX_ARCH_PFN, E820_RAM);
> +	return e820_end_pfn(MAX_ARCH_PFN);
>  }
>  
>  unsigned long __init e820_end_of_low_ram_pfn(void)
>  {
> -	return e820_end_pfn(1UL<<(32 - PAGE_SHIFT), E820_RAM);
> +	return e820_end_pfn(1UL<<(32 - PAGE_SHIFT));
>  }
>  
>  static void early_panic(char *msg)

This is_e820_ram() factoring out becomes really messy in patch #3.

So you left out a bunch of places making comparisons with E820_RAM, 
notably e820_reserve_resources_late() and memblock_x86_fill() - and of 
course those have to be left out, otherwise NVRAM might be registered 
and used as real kernel RAM!

And this shows the weakness and confusion caused by the factoring out 
of is_e820_ram() and then adding E820_PMEM to its definition...

I'd rather you add explicit checks to E820_PMEM (why not E820_PRAM, to 
keep in line with the E820_RAM name?), and not lie about 
is_e820_ram(). It should result in the exact same end result, with 
less confusion.

I have no fundamental objections to the driver otherwise.

Thanks,

	Ingo