RE: [PATCH 3/3] x86: add support for the non-standard protected e820 type

["Elliott, Robert (Server Storage)" <Elliott@hp.com>]

A few editorial nits follow...

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Christoph Hellwig
> Sent: Wednesday, March 25, 2015 11:04 AM
> Subject: [PATCH 3/3] 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.

If this goes into the kernel, I think someone should request that the
ACPI specification mark the value 12 as permanently tainted.  Otherwise
they could assign it to some new meaning that conflicts with all
of this.

A few editorial nits follow...

> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-
> parameters.txt
> index bfcb1a6..98eeaca 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 type 12 and
> +			is NVDIMM or ADR memory.

It can be confusing that E820h type values differ from UEFI 
memory map type values, so it might be worth emphasizing that is 
an E820h type value.

Showing hex alongside would also clarify that it is indeed a 
decimal 12.

Suggestion: "e820 type 12 (0xc)"

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b7d31ca..93a27e4 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1430,6 +1430,19 @@ config ILLEGAL_POINTER_VALUE
> 
>  source "mm/Kconfig"
> 
> +config X86_PMEM_LEGACY
> +	bool "Support non-stanard NVDIMMs and ADR protected memory"

stanard s/b standard


> +	help
> +	  Treat memory marked using the non-stard e820 type of 12 as used

stard s/b standard

> +	  by the Intel Sandy Bridge-EP reference BIOS as protected memory.
> +	  The kernel will the offer these regions to the pmem driver so

the s/b then

> +	  they can be used for persistent storage.
> +
> +	  If you say N the kernel will treat the ADR region like an e820
> +	  reserved region.
> +
> +	  Say Y if unsure
> +

...
> diff --git a/arch/x86/include/uapi/asm/e820.h
> b/arch/x86/include/uapi/asm/e820.h
> index d993e33..e040950 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_PROTECTED_KERN	12

The p in pmem means persistent, not protected.  To me, protected sounds 
like a security feature.  I suggest using a different macro name and 
text strings.

...
> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
> index de088e3..8c6a976 100644
> --- a/arch/x86/kernel/e820.c
> +++ b/arch/x86/kernel/e820.c
> @@ -48,10 +48,22 @@ unsigned long pci_mem_start = 0xaeedbabe;
>  EXPORT_SYMBOL(pci_mem_start);
>  #endif
> 
> +/*
> + * Memory protected by the system ADR (asynchronous dram refresh)
> + * mechanism is accounted as ram for purposes of establishing max_pfn
> + * and mem_map.
> + */

ADR doesn't really protect or ensure persistence; it just puts memory 
into self-refresh mode. Batteries/capacitors and other logic is what
provides the persistence.

...
> @@ -154,6 +166,9 @@ static void __init e820_print_type(u32 type)
>  	case E820_UNUSABLE:
>  		printk(KERN_CONT "unusable");
>  		break;
> +	case E820_PROTECTED_KERN:
> +		printk(KERN_CONT "protected (type %u)\n", type);
> +		break;

Same "protect" comment applies there, and a few other places in
the patch not excerpted.