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

[Boaz Harrosh <boaz@plexistor.com>]

On 03/26/2015 11:34 AM, Christoph Hellwig wrote:
<>

Please re-post this patch stand alone because git am on this will
Give me the wrong title and commit message

small comments ...

> 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>
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
>  Documentation/kernel-parameters.txt |  6 ++++
>  arch/x86/Kconfig                    | 10 ++++++
>  arch/x86/include/asm/setup.h        |  6 ++++
>  arch/x86/include/uapi/asm/e820.h    | 10 ++++++
>  arch/x86/kernel/Makefile            |  1 +
>  arch/x86/kernel/e820.c              | 31 ++++++++++++----
>  arch/x86/kernel/pmem.c              | 70 +++++++++++++++++++++++++++++++++++++
>  arch/x86/kernel/setup.c             |  2 ++
>  8 files changed, 130 insertions(+), 6 deletions(-)
>  create mode 100644 arch/x86/kernel/pmem.c
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index bfcb1a6..c87122d 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1965,6 +1965,12 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			         or
>  			         memmap=0x10000$0x18690000
>  
> +	memmap=nn[KMG]!ss[KMG]
> +			[KNL,X86] Mark specific memory as protected.
> +			Region of memory to be used, from ss to ss+nn.
> +			The memory region may be marked as e820 type 12 (0xc)
> +			and is NVDIMM or ADR memory.
> +

Do we need to escape "\!" this character on grub command line ? It might
help to note that. I did like the original "|" BTW

>  	memory_corruption_check=0/1 [X86]
>  			Some BIOSes seem to corrupt the first 64k of
>  			memory when doing things like suspend/resume.
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b7d31ca..c0e8ee3 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1430,6 +1430,16 @@ config ILLEGAL_POINTER_VALUE
>  
>  source "mm/Kconfig"
>  
> +config X86_PMEM_LEGACY
> +	bool "Support non-standard NVDIMMs and ADR protected memory"
> +	help
> +	  Treat memory marked using the non-standard e820 type of 12 as used
> +	  by the Intel Sandy Bridge-EP reference BIOS as protected memory.
> +	  The kernel will offer these regions to the pmem driver so
> +	  they can be used for persistent storage.
> +
> +	  Say Y if unsure.
> +
>  config HIGHPTE
>  	bool "Allocate 3rd-level pagetables from highmem"
>  	depends on HIGHMEM
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index ff4e7b2..2352fde 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -57,6 +57,12 @@ extern void x86_ce4100_early_setup(void);
>  static inline void x86_ce4100_early_setup(void) { }
>  #endif
>  
> +#ifdef CONFIG_X86_PMEM_LEGACY
> +void reserve_pmem(void);
> +#else
> +static inline void reserve_pmem(void) { }
> +#endif
> +
>  #ifndef _SETUP
>  
>  #include <asm/espfix.h>
> diff --git a/arch/x86/include/uapi/asm/e820.h b/arch/x86/include/uapi/asm/e820.h
> index d993e33..ce0d0bf 100644
> --- a/arch/x86/include/uapi/asm/e820.h
> +++ b/arch/x86/include/uapi/asm/e820.h
> @@ -33,6 +33,16 @@
>  #define E820_NVS	4
>  #define E820_UNUSABLE	5
>  
> +/*
> + * This is a non-standardized way to represent ADR or NVDIMM regions that
> + * persist over a reboot.  The kernel will ignore their special capabilities
> + * unless the CONFIG_X86_PMEM_LEGACY option is set.
> + *
> + * Note that older platforms also used 6 for the same type of memory,
> + * but newer versions switched to 12 as 6 was assigned differently.  Some
> + * time they will learn..
> + */
> +#define E820_PRAM	12

Why the PRAM Name. For one 2/3 of this patch say PMEM the Kconfig
to enable is _PMEM_, the driver stack that gets loaded is pmem,
so PRAM is unexpected.

Also I do believe PRAM is not the correct name. Yes NvDIMMs are RAM,
but there are other not RAM technologies that can be supported exactly
the same way.
MEM is a more general name meaning "on the memory bus". I think.

I would love the consistency.

>  
>  /*
>   * reserved RAM used by kernel itself
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index cdb1b70..971f18c 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -94,6 +94,7 @@ obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvmclock.o
>  obj-$(CONFIG_PARAVIRT)		+= paravirt.o paravirt_patch_$(BITS).o
>  obj-$(CONFIG_PARAVIRT_SPINLOCKS)+= paravirt-spinlocks.o
>  obj-$(CONFIG_PARAVIRT_CLOCK)	+= pvclock.o
> +obj-$(CONFIG_X86_PMEM_LEGACY)	+= pmem.o
>  
>  obj-$(CONFIG_PCSPKR_PLATFORM)	+= pcspeaker.o
>  
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index 46201de..4bd525a 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -149,6 +149,9 @@ static void __init e820_print_type(u32 type)
>  	case E820_UNUSABLE:
>  		printk(KERN_CONT "unusable");
>  		break;
> +	case E820_PRAM:
> +		printk(KERN_CONT "persistent (type %u)", type);

This case can only mean 12 in this patch. (I think historically
you had a Kconfig to set its Number

> +		break;
>  	default:
>  		printk(KERN_CONT "type %u", type);
>  		break;
> @@ -688,8 +691,15 @@ 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)
> +
> +		switch (ei->type) {
> +		case E820_RAM:
> +		case E820_PRAM:
> +		case E820_RESERVED_KERN:
> +			break;
> +		default:
>  			register_nosave_region(PFN_UP(ei->addr), pfn);
> +		}
>  
>  		if (pfn >= limit_pfn)
>  			break;
> @@ -748,7 +758,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 +769,11 @@ 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)
> +		/*
> +		 * Persistent memory is accounted as ram for purposes of
> +		 * establishing max_pfn and mem_map.
> +		 */
> +		if (ei->type != E820_RAM && ei->type != E820_PRAM)
>  			continue;
>  
>  		start_pfn = ei->addr >> PAGE_SHIFT;
> @@ -784,12 +798,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)
> @@ -866,6 +880,9 @@ static int __init parse_memmap_one(char *p)
>  	} else if (*p == '$') {
>  		start_at = memparse(p+1, &p);
>  		e820_add_region(start_at, mem_size, E820_RESERVED);
> +	} else if (*p == '!') {
> +		start_at = memparse(p+1, &p);
> +		e820_add_region(start_at, mem_size, E820_PRAM);
>  	} else
>  		e820_remove_range(mem_size, ULLONG_MAX - mem_size, E820_RAM, 1);
>  
> @@ -907,6 +924,7 @@ static inline const char *e820_type_to_string(int e820_type)
>  	case E820_ACPI:	return "ACPI Tables";
>  	case E820_NVS:	return "ACPI Non-volatile Storage";
>  	case E820_UNUSABLE:	return "Unusable memory";
> +	case E820_PRAM: return "Persistent RAM";

if you change E820_PRAM, then Also here

>  	default:	return "reserved";
>  	}
>  }
> @@ -941,7 +959,8 @@ void __init e820_reserve_resources(void)
>  		 * pcibios_resource_survey()
>  		 */
>  		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20)) {
> -			res->flags |= IORESOURCE_BUSY;
> +			if (e820.map[i].type != E820_PRAM)
> +				res->flags |= IORESOURCE_BUSY;
>  			insert_resource(&iomem_resource, res);
>  		}
>  		res++;
> 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
> @@ -0,0 +1,70 @@
> +/*
> + * Copyright (c) 2009, Intel Corporation.
> + * Copyright (c) 2015, Christoph Hellwig.
> + */
> +#include <linux/memblock.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <asm/e820.h>
> +#include <asm/page_types.h>
> +#include <asm/setup.h>
> +
> +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);
> +	}
> +}
> +
> +static __init void register_pmem_device(struct resource *res)
> +{
> +	struct platform_device *pdev;
> +	int error;
> +
> +	pdev = platform_device_alloc("pmem", PLATFORM_DEVID_AUTO);
> +	if (!pdev)
> +		return;
> +
> +	error = platform_device_add_resources(pdev, res, 1);
> +	if (error)
> +		goto out_put_pdev;
> +
> +	error = platform_device_add(pdev);
> +	if (error)
> +		goto out_put_pdev;
> +	return;
> +out_put_pdev:
> +	dev_warn(&pdev->dev, "failed to add pmem device!\n");
> +	platform_device_put(pdev);
> +}
> +
> +static __init int register_pmem_devices(void)
> +{
> +	int i;
> +
> +	for (i = 0; i < e820.nr_map; i++) {
> +		struct e820entry *ei = &e820.map[i];
> +
> +		if (ei->type == E820_PRAM) {

See here it would be cleaner to ask for E820_PMEM in a 
register_pmem_devices member

Just my $0.017

Thanks
Boaz

> +			struct resource res = {
> +				.flags	= IORESOURCE_MEM,
> +				.start	= ei->addr,
> +				.end	= ei->addr + ei->size - 1,
> +			};
> +			register_pmem_device(&res);
> +		}
> +	}
> +
> +	return 0;
> +}
> +device_initcall(register_pmem_devices);
> 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();
> +
>  	initmem_init();
>  	dma_contiguous_reserve(max_pfn_mapped << PAGE_SHIFT);
>  
>