From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752708AbbCZPtr (ORCPT ); Thu, 26 Mar 2015 11:49:47 -0400 Received: from mail-wi0-f172.google.com ([209.85.212.172]:35669 "EHLO mail-wi0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751774AbbCZPtm (ORCPT ); Thu, 26 Mar 2015 11:49:42 -0400 Message-ID: <55142A92.4060607@plexistor.com> Date: Thu, 26 Mar 2015 17:49:38 +0200 From: Boaz Harrosh User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Christoph Hellwig , Ingo Molnar CC: linux-nvdimm@ml01.01.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, x86@kernel.org, ross.zwisler@linux.intel.com, axboe@kernel.dk, boaz@plexistor.com Subject: Re: [PATCH 2/3] x86: add a is_e820_ram() helper References: <1427358764-6126-1-git-send-email-hch@lst.de> <1427358764-6126-3-git-send-email-hch@lst.de> <20150326090215.GA11520@gmail.com> <20150326093424.GA28217@lst.de> In-Reply-To: <20150326093424.GA28217@lst.de> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > > Signed-off-by: Christoph Hellwig > Tested-by: Ross Zwisler > --- > 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 > 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 > +#include > +#include > +#include > +#include > +#include > + > +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); > >