[PATCH 0/2] e820: Fix handling of NvDIMM chips

[PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

Hi

There is a deficiency in current e820.c handling where unknown new memory-chip
types come up as a BUSY resource when some other driver (like pmem) tries to
call request_mem_region_exclusive() on that resource. Even though, actually
there is nothing using it.
>From inspecting the code and the history of e820.c it looks like a BUG.

In any way this is a problem for the new type-12 NvDIMM memory chips that
are circulating around. (It is estimated that there are already 100ds of
thousands NvDIMM chips in active use)

The patches below first fixes the above problem for any future type
memory, so external drivers can access these mem chips.

I then also add the NvDIMM type-12 memory constant so it comes up
nice in dprints and at /proc/iomem

Just as before all these chips are very much usable with the pmem
driver. This lets us remove the hack for type-12 NvDIMMs that ignores
the return code from request_mem_region_exclusive() in pmem.c.

There is a 3rd patch just for reference to pmem.c which enables
pmem to work also on Old Kernels which do not include these 2
patches.

For all the pmem people. I maintain a tree with these patches
and latest pmem code (Also including DAX) here:
	git://git.open-osd.org/pmem.git
	[web-view:http://git.open-osd.org/gitweb.cgi?p=pmem.git;a=summary]

List of patches:
[PATCH 1/2] e820: Don't let unknown DIMM type come out BUSY
[RFC 2/2] e820: Add the NvDIMM Memory type (type-12)
	These are for submission

[PATCH 3/3] pmem: Allow request_mem to fail (CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET)
	Just for reference

Thanks
Boaz

[PATCH 1/2] e820: Don't let unknown DIMM type come out BUSY

[Boaz Harrosh]

There is something not very nice (Gentlemen nice) In current
e820.c code.

At Multiple place for example @ memblock_x86_fill(), it will
add the different memory resources *except the E820_RESERVED type*

Then at e820_reserve_resources() it will mark all !E820_RESERVED
as busy.

This is all fine when we have only the known types one of:
	E820_RESERVED_KERN:
	E820_RAM:
	E820_ACPI:
	E820_NVS:
	E820_UNUSABLE:
	E820_RESERVED:

But if the system encounters a brand new memory type it will
not add it to any memory list, But will proceed to mark it
BUSY. So now any other Driver in the system that does know
how to deal with this new type, is not able to call
request_mem_region_exclusive() on this new type because it is
hard coded BUSY even though nothing really uses it.

So make any unknown type behave like E820_RESERVED memory,
it will show up as available to first caller of
request_mem_region_exclusive().

I Also change the string representation of an unknown type
from "reserved" (So to not confuse with memmap "reserved"
region). And call it "reserved-unknown"
I wish I could return "reserved-type-X" But this is not possible
because one must return a constant, code-segment, string.

(NOTE: These unknown-types where called "reserved" in
 /proc/iomem and in dmesg but behaved differently. What this
 patch does is name them differently but let them behave
 the same)

An example of such "UNKNOWN" type is the not Standard type-12
DDR3-NvDIMM which is used by multiple vendors for a while
now. (Estimated 100ds of thousands sold world wide)
A later patch adds type-12 to the list of "KNOWN", types.

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/kernel/e820.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 46201de..8cfd25f 100644
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -907,7 +907,23 @@ 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";
-	default:	return "reserved";
+	case E820_RESERVED:	return "reserved";
+	default:	return "reserved-unkown";
+	}
+}
+
+static bool _is_reserved_type(int e820_type)
+{
+	switch (e820_type) {
+	case E820_RESERVED_KERN:
+	case E820_RAM:
+	case E820_ACPI:
+	case E820_NVS:
+	case E820_UNUSABLE:
+		return false;
+	case E820_RESERVED:
+	default:
+		return true;
 	}
 }
 
@@ -940,7 +956,7 @@ void __init e820_reserve_resources(void)
 		 * pci device BAR resource and insert them later in
 		 * pcibios_resource_survey()
 		 */
-		if (e820.map[i].type != E820_RESERVED || res->start < (1ULL<<20)) {
+		if (!_is_reserved_type(e820.map[i].type) || res->start < (1ULL<<20)) {
 			res->flags |= IORESOURCE_BUSY;
 			insert_resource(&iomem_resource, res);
 		}
-- 
1.9.3

[RFC 2/2] e820: Add the NvDIMM Memory type (type-12)

[Boaz Harrosh]

There are multiple vendors of DDR3 NvDIMMs out in the market today.
At various stages of development/production. It is estimated that
there are already more the 100ds of thousands chips sold to
testers and sites.

All the vendors I know of, tagged these chips as type-12 memory.

[THIS IS A CALL FOR ALL NvDIMM Vendors if you have used other
 then type-12 NvDIMM, please email me and I will add support
 for it]

Now the ACPI comity, as far as I know, did not yet define a
standard type for NvDIMM. Also, as far as I know any NvDIMM
standard will be defined for DDR4. So DDR3 NvDIMM will continue
to be supported by the individual vendors.

I Wish and call the ACPI comity to Define that NvDIMM is type-12.
Also for DDR4

In this patch I name type-12 "NvDIMM-12", but if there
are positive responses I would like to just name it "NvDIMM",
For any kind of DDR-X.

Actually is there any reason why I should not submit the final
version as "NvDIMM" from the get go. Then if the ACPI defines
another new type-Y Memory we can name both types "NvDIMM", No?
Please comment

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 arch/x86/include/uapi/asm/e820.h | 2 +-
 arch/x86/kernel/e820.c           | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/uapi/asm/e820.h b/arch/x86/include/uapi/asm/e820.h
index d993e33..a0977e0 100644
--- a/arch/x86/include/uapi/asm/e820.h
+++ b/arch/x86/include/uapi/asm/e820.h
@@ -32,7 +32,7 @@
 #define E820_ACPI	3
 #define E820_NVS	4
 #define E820_UNUSABLE	5
-
+#define E820_NvDIMM_12 12
 
 /*
  * reserved RAM used by kernel itself
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
index 8cfd25f..17f7496 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_NvDIMM_12:
+		printk(KERN_CONT "NvDIMM-12");
+		break;
 	default:
 		printk(KERN_CONT "type %u", type);
 		break;
@@ -908,6 +911,7 @@ static inline const char *e820_type_to_string(int e820_type)
 	case E820_NVS:	return "ACPI Non-volatile Storage";
 	case E820_UNUSABLE:	return "Unusable memory";
 	case E820_RESERVED:	return "reserved";
+	case E820_NvDIMM_12:	return "NvDIMM-12";
 	default:	return "reserved-unkown";
 	}
 }
@@ -922,6 +926,7 @@ static bool _is_reserved_type(int e820_type)
 	case E820_UNUSABLE:
 		return false;
 	case E820_RESERVED:
+	case E820_NvDIMM_12:
 	default:
 		return true;
 	}
-- 
1.9.3

[PATCH 3/3] pmem: Allow request_mem to fail, (CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET)

[Boaz Harrosh]

With old Kernels there was a bug in x86 where any unknown
memory chip type would come up BUSY when calling
request_mem_region_exclusive().

So for pmem to work with old Kernels and real NvDIMM chips
we have a new Kconfig option CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET.

People have been running with hacked up pmem that will ignore
the return code from request_mem_region_exclusive. So here it is
official

Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
---
 drivers/block/Kconfig | 12 ++++++++++++
 drivers/block/pmem.c  | 11 +++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index 3b3200f..10879b8 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -430,6 +430,18 @@ config BLK_DEV_PMEM_USE_PAGES
 	  to other devices in the system, then you must say "Yes" here.
 	  If unsure leave as Yes.
 
+config BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET
+	bool "Ignore the return code from request_mem_region_exclusive"
+	depends on BLK_DEV_PMEM
+	help
+	  In Old Kernels type-12 Memory type which is used by NvDIMM
+	  chips Comes out busy when calling request_mem_region_exclusive,
+	  because of a bug.
+	  If this option is set to "yes". The pmem will ignore the
+	  failure, and continue as usual. If you have an old Kernel and
+	  a real NvDIMM chip you must say yes here.
+	  (Ignored if BLK_DEV_PMEM_USE_PAGES=y)
+
 config CDROM_PKTCDVD
 	tristate "Packet writing on CD/DVD media"
 	depends on !UML
diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
index 9eb7ffe..f84d033 100644
--- a/drivers/block/pmem.c
+++ b/drivers/block/pmem.c
@@ -197,10 +197,12 @@ int pmem_mapmem(struct pmem_device *pmem)
 
 	res_mem = request_mem_region_exclusive(pmem->phys_addr, pmem->size,
 					       "pmem");
-	if (!res_mem) {
+	if (unlikely(!res_mem)) {
 		pr_warn("pmem: request_mem_region_exclusive phys=0x%llx size=0x%zx failed\n",
-			   pmem->phys_addr, pmem->size);
-		return -EINVAL;
+			pmem->phys_addr, pmem->size);
+#ifndef CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET
+		return -EBUSY;
+#endif
 	}
 
 	pmem->virt_addr = ioremap_cache(pmem->phys_addr, pmem->size);
@@ -211,7 +213,8 @@ int pmem_mapmem(struct pmem_device *pmem)
 	return 0;
 
 out_release:
-	release_mem_region(pmem->phys_addr, pmem->size);
+	if (res_mem)
+		release_mem_region(pmem->phys_addr, pmem->size);
 	return err;
 }
 
-- 
1.9.3

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Matthew Wilcox]

On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
> In any way this is a problem for the new type-12 NvDIMM memory chips that
> are circulating around. (It is estimated that there are already 100ds of
> thousands NvDIMM chips in active use)

Hang on.  NV-DIMM chips don't know anyhing about E820 tables.  They don't
have anything in them that says "I am type 12!".  How they are reported
is up to the BIOS.  Just because your BIOS vendor has chosen to report
tham as type 12 doesn't mean that any other BIOS vedor is going to have
done the same thing.

Fortunately, the BIOS people have all got together and decided what
they're going to do, and it's not type 12.  Unfortunately, I think
I'm bound by various agreements to not say what they are going to do
until they do.  But putting this temporary workaround in the kernel to
accomodate one BIOS vendor's unreleased experimental code seems like
entirely the wrong idea.

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
>> In any way this is a problem for the new type-12 NvDIMM memory chips that
>> are circulating around. (It is estimated that there are already 100ds of
>> thousands NvDIMM chips in active use)
> 
> Hang on.  NV-DIMM chips don't know anyhing about E820 tables.  They don't
> have anything in them that says "I am type 12!".  How they are reported
> is up to the BIOS.  Just because your BIOS vendor has chosen to report
> tham as type 12 doesn't mean that any other BIOS vedor is going to have
> done the same thing.
> 
> Fortunately, the BIOS people have all got together and decided what
> they're going to do, and it's not type 12.  Unfortunately, I think
> I'm bound by various agreements to not say what they are going to do
> until they do.  But putting this temporary workaround in the kernel to
> accomodate one BIOS vendor's unreleased experimental code seems like
> entirely the wrong idea.
> 

I had a feeling I'm entering an holy war ;-).

I hope you are OK with my first patch. That an unknown type need not
be reported busy, and behave same as "reserved"?

Then if we agree about PATCH-1, which is the actual fix.
Then the 2nd patch (hence the RFC btw) is nothing more than
a name.

I have an old BIOS that knows nothing of NvDIMM, actually a few
of them they all report 12.
The fact of the matter is that all the people I've talked with,
reported that different vendor chips, all came up type-12.
Perhaps type-12 just means "Unknown to current BIOS"

What is the name you suggest "type-12" "unknown-12".
Do you understand why they all come out 12 ?

Thanks
Boaz

Re: [PATCH 3/3] pmem: Allow request_mem to fail, (CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET)

[Ross Zwisler]

On Mon, 2015-02-16 at 13:24 +0200, Boaz Harrosh wrote:
> With old Kernels there was a bug in x86 where any unknown
> memory chip type would come up BUSY when calling
> request_mem_region_exclusive().
> 
> So for pmem to work with old Kernels and real NvDIMM chips
> we have a new Kconfig option CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET.
> 
> People have been running with hacked up pmem that will ignore
> the return code from request_mem_region_exclusive. So here it is
> official
> 
> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>

I'm confused - I thought that this behavior was fixed by patch 1/3?
With that patch this memory reservation should not fail, correct?

If so, why do we need this patch?

> ---
>  drivers/block/Kconfig | 12 ++++++++++++
>  drivers/block/pmem.c  | 11 +++++++----
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index 3b3200f..10879b8 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -430,6 +430,18 @@ config BLK_DEV_PMEM_USE_PAGES
>  	  to other devices in the system, then you must say "Yes" here.
>  	  If unsure leave as Yes.
>  
> +config BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET
> +	bool "Ignore the return code from request_mem_region_exclusive"
> +	depends on BLK_DEV_PMEM
> +	help
> +	  In Old Kernels type-12 Memory type which is used by NvDIMM
> +	  chips Comes out busy when calling request_mem_region_exclusive,
> +	  because of a bug.
> +	  If this option is set to "yes". The pmem will ignore the
> +	  failure, and continue as usual. If you have an old Kernel and
> +	  a real NvDIMM chip you must say yes here.
> +	  (Ignored if BLK_DEV_PMEM_USE_PAGES=y)
> +
>  config CDROM_PKTCDVD
>  	tristate "Packet writing on CD/DVD media"
>  	depends on !UML
> diff --git a/drivers/block/pmem.c b/drivers/block/pmem.c
> index 9eb7ffe..f84d033 100644
> --- a/drivers/block/pmem.c
> +++ b/drivers/block/pmem.c
> @@ -197,10 +197,12 @@ int pmem_mapmem(struct pmem_device *pmem)
>  
>  	res_mem = request_mem_region_exclusive(pmem->phys_addr, pmem->size,
>  					       "pmem");
> -	if (!res_mem) {
> +	if (unlikely(!res_mem)) {
>  		pr_warn("pmem: request_mem_region_exclusive phys=0x%llx size=0x%zx failed\n",
> -			   pmem->phys_addr, pmem->size);
> -		return -EINVAL;
> +			pmem->phys_addr, pmem->size);
> +#ifndef CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET
> +		return -EBUSY;
> +#endif
>  	}
>  
>  	pmem->virt_addr = ioremap_cache(pmem->phys_addr, pmem->size);
> @@ -211,7 +213,8 @@ int pmem_mapmem(struct pmem_device *pmem)
>  	return 0;
>  
>  out_release:
> -	release_mem_region(pmem->phys_addr, pmem->size);
> +	if (res_mem)
> +		release_mem_region(pmem->phys_addr, pmem->size);
>  	return err;
>  }
>  

Re: [PATCH 3/3] pmem: Allow request_mem to fail, (CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET)

[Boaz Harrosh]

On 02/17/2015 10:52 PM, Ross Zwisler wrote:
> On Mon, 2015-02-16 at 13:24 +0200, Boaz Harrosh wrote:
>> With old Kernels there was a bug in x86 where any unknown
>> memory chip type would come up BUSY when calling
>> request_mem_region_exclusive().
>>
>> So for pmem to work with old Kernels and real NvDIMM chips
>> we have a new Kconfig option CONFIG_BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET.
>>
>> People have been running with hacked up pmem that will ignore
>> the return code from request_mem_region_exclusive. So here it is
>> official
>>
>> Signed-off-by: Boaz Harrosh <boaz@plexistor.com>
> 

Hi Ross, good morning

> I'm confused - I thought that this behavior was fixed by patch 1/3?
> With that patch this memory reservation should not fail, correct?
> 

Yes, I have tested it extensively and PATCH-1/3 fixes this problem
for sure.

> If so, why do we need this patch?
> 

I put in this patch for people that do-not-want/cannot compile their
own Kernel but have a need for pmem.c regardless. I will not include
this patch in the final submitted Kernel. Given that patch-1 gets
accepted before the merge of pmem.

Lets say that patch-1 and patch-3 are either or. A tree
that has 1/3 does not need 3/3, a tree that does not have 1/3 needs
3/3.

<>

Thanks
Boaz

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Dan Williams]

On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
> On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
>> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
>>> In any way this is a problem for the new type-12 NvDIMM memory chips that
>>> are circulating around. (It is estimated that there are already 100ds of
>>> thousands NvDIMM chips in active use)
>>
>> Hang on.  NV-DIMM chips don't know anyhing about E820 tables.  They don't
>> have anything in them that says "I am type 12!".  How they are reported
>> is up to the BIOS.  Just because your BIOS vendor has chosen to report
>> tham as type 12 doesn't mean that any other BIOS vedor is going to have
>> done the same thing.
>>
>> Fortunately, the BIOS people have all got together and decided what
>> they're going to do, and it's not type 12.  Unfortunately, I think
>> I'm bound by various agreements to not say what they are going to do
>> until they do.  But putting this temporary workaround in the kernel to
>> accomodate one BIOS vendor's unreleased experimental code seems like
>> entirely the wrong idea.
>>
>
> I had a feeling I'm entering an holy war ;-).
>
> I hope you are OK with my first patch. That an unknown type need not
> be reported busy, and behave same as "reserved"?

No, it seems the safe thing to do is prevent the kernel from accessing
any memory that it does not know the side-effects of accessing.

> Then if we agree about PATCH-1, which is the actual fix.
> Then the 2nd patch (hence the RFC btw) is nothing more than
> a name.
>
> I have an old BIOS that knows nothing of NvDIMM, actually a few
> of them they all report 12.
> The fact of the matter is that all the people I've talked with,
> reported that different vendor chips, all came up type-12.
> Perhaps type-12 just means "Unknown to current BIOS"
>
> What is the name you suggest "type-12" "unknown-12".
> Do you understand why they all come out 12 ?
>

In fact it was originally "type-6" until ACPI 5 claimed that number
for official use, so these platforms, with early proof-of-concept
nvdimm support, have already gone through one transition to a new
number.  They need to do the same once an official number for nvdimm
support is published.

Put another way, these early platforms are already using out-of-tree
patches for nvdimm enabling.  They can continue to do so, or switch to
standard methods when the standard is published.

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Ingo Molnar]

* Dan Williams <dan.j.williams@intel.com> wrote:

> On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
> > On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
> >> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
> >>> In any way this is a problem for the new type-12 NvDIMM memory chips that
> >>> are circulating around. (It is estimated that there are already 100ds of
> >>> thousands NvDIMM chips in active use)
> >>
> >> Hang on.  NV-DIMM chips don't know anyhing about E820 
> >> tables.  They don't have anything in them that says "I 
> >> am type 12!".  How they are reported is up to the 
> >> BIOS.  Just because your BIOS vendor has chosen to 
> >> report tham as type 12 doesn't mean that any other 
> >> BIOS vedor is going to have done the same thing.
> >>
> >> Fortunately, the BIOS people have all got together and 
> >> decided what they're going to do, and it's not type 
> >> 12.  Unfortunately, I think I'm bound by various 
> >> agreements to not say what they are going to do until 
> >> they do.  But putting this temporary workaround in the 
> >> kernel to accomodate one BIOS vendor's unreleased 
> >> experimental code seems like entirely the wrong idea.
> >>
> >
> > I had a feeling I'm entering an holy war ;-).
> >
> > I hope you are OK with my first patch. That an unknown 
> > type need not be reported busy, and behave same as 
> > "reserved"?
> 
> No, it seems the safe thing to do is prevent the kernel 
> from accessing any memory that it does not know the 
> side-effects of accessing.

Well, except when the kernel does know how to access it: 
when an nvdimm driver knows about its own memory region and 
knows how to handle it, right?

So is there any practical reason to mark the memory 
resource as busy in that case, instead of just adding it to 
the reserved list by default and allowing properly informed 
drivers to (exclusively) request it?

Thanks,

	Ingo

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Dan Williams]

On Wed, Feb 18, 2015 at 10:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dan Williams <dan.j.williams@intel.com> wrote:
>
>> On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>> > On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
>> >> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
>> >>> In any way this is a problem for the new type-12 NvDIMM memory chips that
>> >>> are circulating around. (It is estimated that there are already 100ds of
>> >>> thousands NvDIMM chips in active use)
>> >>
>> >> Hang on.  NV-DIMM chips don't know anyhing about E820
>> >> tables.  They don't have anything in them that says "I
>> >> am type 12!".  How they are reported is up to the
>> >> BIOS.  Just because your BIOS vendor has chosen to
>> >> report tham as type 12 doesn't mean that any other
>> >> BIOS vedor is going to have done the same thing.
>> >>
>> >> Fortunately, the BIOS people have all got together and
>> >> decided what they're going to do, and it's not type
>> >> 12.  Unfortunately, I think I'm bound by various
>> >> agreements to not say what they are going to do until
>> >> they do.  But putting this temporary workaround in the
>> >> kernel to accomodate one BIOS vendor's unreleased
>> >> experimental code seems like entirely the wrong idea.
>> >>
>> >
>> > I had a feeling I'm entering an holy war ;-).
>> >
>> > I hope you are OK with my first patch. That an unknown
>> > type need not be reported busy, and behave same as
>> > "reserved"?
>>
>> No, it seems the safe thing to do is prevent the kernel
>> from accessing any memory that it does not know the
>> side-effects of accessing.
>
> Well, except when the kernel does know how to access it:
> when an nvdimm driver knows about its own memory region and
> knows how to handle it, right?

Yes, except that "type-12" is something picked out of the air that may
be invalidated by a future spec change.

If firmware wants any driver to handle a memory range it can already
use E820_RESERVED.  The only reason a new-type was picked in these
early implementations was for experiments around reserving nvdimm
memory for driver use, but also extending it to be covered with struct
page mappings.  Outside of that there is no real driving reason for
the new type.

> So is there any practical reason to mark the memory
> resource as busy in that case, instead of just adding it to
> the reserved list by default and allowing properly informed
> drivers to (exclusively) request it?

I'm not sure we want firmware to repeat this confusion going forward.
Why support new memory types unless defined by ACPI or otherwise
sufficiently described by E820_RESERVED?

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Ingo Molnar]

* Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, Feb 18, 2015 at 10:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Dan Williams <dan.j.williams@intel.com> wrote:
> >
> >> On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
> >> > On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
> >> >> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
> >> >>> In any way this is a problem for the new type-12 NvDIMM memory chips that
> >> >>> are circulating around. (It is estimated that there are already 100ds of
> >> >>> thousands NvDIMM chips in active use)
> >> >>
> >> >> Hang on.  NV-DIMM chips don't know anyhing about E820
> >> >> tables.  They don't have anything in them that says "I
> >> >> am type 12!".  How they are reported is up to the
> >> >> BIOS.  Just because your BIOS vendor has chosen to
> >> >> report tham as type 12 doesn't mean that any other
> >> >> BIOS vedor is going to have done the same thing.
> >> >>
> >> >> Fortunately, the BIOS people have all got together and
> >> >> decided what they're going to do, and it's not type
> >> >> 12.  Unfortunately, I think I'm bound by various
> >> >> agreements to not say what they are going to do until
> >> >> they do.  But putting this temporary workaround in the
> >> >> kernel to accomodate one BIOS vendor's unreleased
> >> >> experimental code seems like entirely the wrong idea.
> >> >>
> >> >
> >> > I had a feeling I'm entering an holy war ;-).
> >> >
> >> > I hope you are OK with my first patch. That an unknown
> >> > type need not be reported busy, and behave same as
> >> > "reserved"?
> >>
> >> No, it seems the safe thing to do is prevent the 
> >> kernel from accessing any memory that it does not know 
> >> the side-effects of accessing.
> >
> > Well, except when the kernel does know how to access 
> > it: when an nvdimm driver knows about its own memory 
> > region and knows how to handle it, right?
> 
> Yes, except that "type-12" is something picked out of the 
> air that may be invalidated by a future spec change.
> 
> If firmware wants any driver to handle a memory range it 
> can already use E820_RESERVED.  The only reason a 
> new-type was picked in these early implementations was 
> for experiments around reserving nvdimm memory for driver 
> use, but also extending it to be covered with struct page 
> mappings.  Outside of that there is no real driving 
> reason for the new type.

But ... if a user is blessed/haunted with such firmware, 
why not let new types fall back to 'reserved', which is a 
reasonable default that still allows sufficiently aware 
Linux drivers to work, right?

> > So is there any practical reason to mark the memory 
> > resource as busy in that case, instead of just adding 
> > it to the reserved list by default and allowing 
> > properly informed drivers to (exclusively) request it?
> 
> I'm not sure we want firmware to repeat this confusion 
> going forward. Why support new memory types unless 
> defined by ACPI or otherwise sufficiently described by 
> E820_RESERVED?

Because it would make the kernel more functional? We should 
always err on the side of allowing more functionality and 
not erect roadblocks.

Thanks,

	Ingo

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Dan Williams]

On Wed, Feb 18, 2015 at 10:53 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dan Williams <dan.j.williams@intel.com> wrote:
>
>> On Wed, Feb 18, 2015 at 10:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Dan Williams <dan.j.williams@intel.com> wrote:
>> >
>> >> On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>> >> > On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
>> >> >> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
>> >> >>> In any way this is a problem for the new type-12 NvDIMM memory chips that
>> >> >>> are circulating around. (It is estimated that there are already 100ds of
>> >> >>> thousands NvDIMM chips in active use)
>> >> >>
>> >> >> Hang on.  NV-DIMM chips don't know anyhing about E820
>> >> >> tables.  They don't have anything in them that says "I
>> >> >> am type 12!".  How they are reported is up to the
>> >> >> BIOS.  Just because your BIOS vendor has chosen to
>> >> >> report tham as type 12 doesn't mean that any other
>> >> >> BIOS vedor is going to have done the same thing.
>> >> >>
>> >> >> Fortunately, the BIOS people have all got together and
>> >> >> decided what they're going to do, and it's not type
>> >> >> 12.  Unfortunately, I think I'm bound by various
>> >> >> agreements to not say what they are going to do until
>> >> >> they do.  But putting this temporary workaround in the
>> >> >> kernel to accomodate one BIOS vendor's unreleased
>> >> >> experimental code seems like entirely the wrong idea.
>> >> >>
>> >> >
>> >> > I had a feeling I'm entering an holy war ;-).
>> >> >
>> >> > I hope you are OK with my first patch. That an unknown
>> >> > type need not be reported busy, and behave same as
>> >> > "reserved"?
>> >>
>> >> No, it seems the safe thing to do is prevent the
>> >> kernel from accessing any memory that it does not know
>> >> the side-effects of accessing.
>> >
>> > Well, except when the kernel does know how to access
>> > it: when an nvdimm driver knows about its own memory
>> > region and knows how to handle it, right?
>>
>> Yes, except that "type-12" is something picked out of the
>> air that may be invalidated by a future spec change.
>>
>> If firmware wants any driver to handle a memory range it
>> can already use E820_RESERVED.  The only reason a
>> new-type was picked in these early implementations was
>> for experiments around reserving nvdimm memory for driver
>> use, but also extending it to be covered with struct page
>> mappings.  Outside of that there is no real driving
>> reason for the new type.
>
> But ... if a user is blessed/haunted with such firmware,
> why not let new types fall back to 'reserved', which is a
> reasonable default that still allows sufficiently aware
> Linux drivers to work, right?

True.

>
>> > So is there any practical reason to mark the memory
>> > resource as busy in that case, instead of just adding
>> > it to the reserved list by default and allowing
>> > properly informed drivers to (exclusively) request it?
>>
>> I'm not sure we want firmware to repeat this confusion
>> going forward. Why support new memory types unless
>> defined by ACPI or otherwise sufficiently described by
>> E820_RESERVED?
>
> Because it would make the kernel more functional? We should
> always err on the side of allowing more functionality and
> not erect roadblocks.
>

I'm not convinced Linux is better off enabling one-off BIOS
implementations to pick non-standard numbers.  Would it not be safer
to at least confirm with the user via a configuration option, "do you
want drivers to access unknown memory types"?

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Ingo Molnar]

* Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, Feb 18, 2015 at 10:53 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * Dan Williams <dan.j.williams@intel.com> wrote:
> >
> >> On Wed, Feb 18, 2015 at 10:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
> >> >
> >> > * Dan Williams <dan.j.williams@intel.com> wrote:
> >> >
> >> >> On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
> >> >> > On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
> >> >> >> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
> >> >> >>> In any way this is a problem for the new type-12 NvDIMM memory chips that
> >> >> >>> are circulating around. (It is estimated that there are already 100ds of
> >> >> >>> thousands NvDIMM chips in active use)
> >> >> >>
> >> >> >> Hang on.  NV-DIMM chips don't know anyhing about E820
> >> >> >> tables.  They don't have anything in them that says "I
> >> >> >> am type 12!".  How they are reported is up to the
> >> >> >> BIOS.  Just because your BIOS vendor has chosen to
> >> >> >> report tham as type 12 doesn't mean that any other
> >> >> >> BIOS vedor is going to have done the same thing.
> >> >> >>
> >> >> >> Fortunately, the BIOS people have all got together and
> >> >> >> decided what they're going to do, and it's not type
> >> >> >> 12.  Unfortunately, I think I'm bound by various
> >> >> >> agreements to not say what they are going to do until
> >> >> >> they do.  But putting this temporary workaround in the
> >> >> >> kernel to accomodate one BIOS vendor's unreleased
> >> >> >> experimental code seems like entirely the wrong idea.
> >> >> >>
> >> >> >
> >> >> > I had a feeling I'm entering an holy war ;-).
> >> >> >
> >> >> > I hope you are OK with my first patch. That an unknown
> >> >> > type need not be reported busy, and behave same as
> >> >> > "reserved"?
> >> >>
> >> >> No, it seems the safe thing to do is prevent the
> >> >> kernel from accessing any memory that it does not know
> >> >> the side-effects of accessing.
> >> >
> >> > Well, except when the kernel does know how to access
> >> > it: when an nvdimm driver knows about its own memory
> >> > region and knows how to handle it, right?
> >>
> >> Yes, except that "type-12" is something picked out of the
> >> air that may be invalidated by a future spec change.
> >>
> >> If firmware wants any driver to handle a memory range it
> >> can already use E820_RESERVED.  The only reason a
> >> new-type was picked in these early implementations was
> >> for experiments around reserving nvdimm memory for driver
> >> use, but also extending it to be covered with struct page
> >> mappings.  Outside of that there is no real driving
> >> reason for the new type.
> >
> > But ... if a user is blessed/haunted with such firmware,
> > why not let new types fall back to 'reserved', which is a
> > reasonable default that still allows sufficiently aware
> > Linux drivers to work, right?
> 
> True.
> 
> >
> >> > So is there any practical reason to mark the memory
> >> > resource as busy in that case, instead of just adding
> >> > it to the reserved list by default and allowing
> >> > properly informed drivers to (exclusively) request it?
> >>
> >> I'm not sure we want firmware to repeat this confusion
> >> going forward. Why support new memory types unless
> >> defined by ACPI or otherwise sufficiently described by
> >> E820_RESERVED?
> >
> > Because it would make the kernel more functional? We should
> > always err on the side of allowing more functionality and
> > not erect roadblocks.
> >
> 
> I'm not convinced Linux is better off enabling one-off 
> BIOS implementations to pick non-standard numbers.  Would 
> it not be safer to at least confirm with the user via a 
> configuration option, "do you want drivers to access 
> unknown memory types"?

Well, we could emit a warning (or taint the kernel), to 
keep the user informed that there's a version mismatch 
between kernel and firmware - but otherwise still allow 
informed drivers to register that region?

Thanks,

	Ingo

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Dan Williams]

On Wed, Feb 18, 2015 at 11:27 AM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * Dan Williams <dan.j.williams@intel.com> wrote:
>
>> On Wed, Feb 18, 2015 at 10:53 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * Dan Williams <dan.j.williams@intel.com> wrote:
>> >
>> >> On Wed, Feb 18, 2015 at 10:30 AM, Ingo Molnar <mingo@kernel.org> wrote:
>> >> >
>> >> > * Dan Williams <dan.j.williams@intel.com> wrote:
>> >> >
>> >> >> On Tue, Feb 17, 2015 at 12:42 AM, Boaz Harrosh <boaz@plexistor.com> wrote:
>> >> >> > On 02/17/2015 12:03 AM, Matthew Wilcox wrote:
>> >> >> >> On Mon, Feb 16, 2015 at 01:07:07PM +0200, Boaz Harrosh wrote:
>> >> >> >>> In any way this is a problem for the new type-12 NvDIMM memory chips that
>> >> >> >>> are circulating around. (It is estimated that there are already 100ds of
>> >> >> >>> thousands NvDIMM chips in active use)
>> >> >> >>
>> >> >> >> Hang on.  NV-DIMM chips don't know anyhing about E820
>> >> >> >> tables.  They don't have anything in them that says "I
>> >> >> >> am type 12!".  How they are reported is up to the
>> >> >> >> BIOS.  Just because your BIOS vendor has chosen to
>> >> >> >> report tham as type 12 doesn't mean that any other
>> >> >> >> BIOS vedor is going to have done the same thing.
>> >> >> >>
>> >> >> >> Fortunately, the BIOS people have all got together and
>> >> >> >> decided what they're going to do, and it's not type
>> >> >> >> 12.  Unfortunately, I think I'm bound by various
>> >> >> >> agreements to not say what they are going to do until
>> >> >> >> they do.  But putting this temporary workaround in the
>> >> >> >> kernel to accomodate one BIOS vendor's unreleased
>> >> >> >> experimental code seems like entirely the wrong idea.
>> >> >> >>
>> >> >> >
>> >> >> > I had a feeling I'm entering an holy war ;-).
>> >> >> >
>> >> >> > I hope you are OK with my first patch. That an unknown
>> >> >> > type need not be reported busy, and behave same as
>> >> >> > "reserved"?
>> >> >>
>> >> >> No, it seems the safe thing to do is prevent the
>> >> >> kernel from accessing any memory that it does not know
>> >> >> the side-effects of accessing.
>> >> >
>> >> > Well, except when the kernel does know how to access
>> >> > it: when an nvdimm driver knows about its own memory
>> >> > region and knows how to handle it, right?
>> >>
>> >> Yes, except that "type-12" is something picked out of the
>> >> air that may be invalidated by a future spec change.
>> >>
>> >> If firmware wants any driver to handle a memory range it
>> >> can already use E820_RESERVED.  The only reason a
>> >> new-type was picked in these early implementations was
>> >> for experiments around reserving nvdimm memory for driver
>> >> use, but also extending it to be covered with struct page
>> >> mappings.  Outside of that there is no real driving
>> >> reason for the new type.
>> >
>> > But ... if a user is blessed/haunted with such firmware,
>> > why not let new types fall back to 'reserved', which is a
>> > reasonable default that still allows sufficiently aware
>> > Linux drivers to work, right?
>>
>> True.
>>
>> >
>> >> > So is there any practical reason to mark the memory
>> >> > resource as busy in that case, instead of just adding
>> >> > it to the reserved list by default and allowing
>> >> > properly informed drivers to (exclusively) request it?
>> >>
>> >> I'm not sure we want firmware to repeat this confusion
>> >> going forward. Why support new memory types unless
>> >> defined by ACPI or otherwise sufficiently described by
>> >> E820_RESERVED?
>> >
>> > Because it would make the kernel more functional? We should
>> > always err on the side of allowing more functionality and
>> > not erect roadblocks.
>> >
>>
>> I'm not convinced Linux is better off enabling one-off
>> BIOS implementations to pick non-standard numbers.  Would
>> it not be safer to at least confirm with the user via a
>> configuration option, "do you want drivers to access
>> unknown memory types"?
>
> Well, we could emit a warning (or taint the kernel), to
> keep the user informed that there's a version mismatch
> between kernel and firmware - but otherwise still allow
> informed drivers to register that region?

Sounds good to me.

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Christoph Hellwig]

On Wed, Feb 18, 2015 at 10:15:32AM -0800, Dan Williams wrote:
> In fact it was originally "type-6" until ACPI 5 claimed that number
> for official use, so these platforms, with early proof-of-concept
> nvdimm support, have already gone through one transition to a new
> number.  They need to do the same once an official number for nvdimm
> support is published.
> 
> Put another way, these early platforms are already using out-of-tree
> patches for nvdimm enabling.  They can continue to do so, or switch to
> standard methods when the standard is published.

Not supporting hardware that is widely avaiable (I have some, too)
is not very user friendly.

I'll submit a patch allowing a nvdimm_type= kernel option that allows
to detect them, but will do nothing by default.  The code needed is very
small and it would be very useful for all kinds of projects.

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Dan Williams]

On Wed, Feb 18, 2015 at 4:47 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Feb 18, 2015 at 10:15:32AM -0800, Dan Williams wrote:
>> In fact it was originally "type-6" until ACPI 5 claimed that number
>> for official use, so these platforms, with early proof-of-concept
>> nvdimm support, have already gone through one transition to a new
>> number.  They need to do the same once an official number for nvdimm
>> support is published.
>>
>> Put another way, these early platforms are already using out-of-tree
>> patches for nvdimm enabling.  They can continue to do so, or switch to
>> standard methods when the standard is published.
>
> Not supporting hardware that is widely avaiable (I have some, too)
> is not very user friendly.

Yes, as I agreed with Ingo, allowing a driver to assume control of an
unknown memory type with a warning or a kernel taint seems fine.

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/19/2015 02:47 AM, Christoph Hellwig wrote:
> On Wed, Feb 18, 2015 at 10:15:32AM -0800, Dan Williams wrote:
>> In fact it was originally "type-6" until ACPI 5 claimed that number
>> for official use, so these platforms, with early proof-of-concept
>> nvdimm support, have already gone through one transition to a new
>> number.  They need to do the same once an official number for nvdimm
>> support is published.
>>
>> Put another way, these early platforms are already using out-of-tree
>> patches for nvdimm enabling.  They can continue to do so, or switch to
>> standard methods when the standard is published.
> 
> Not supporting hardware that is widely avaiable (I have some, too)
> is not very user friendly.
> 
> I'll submit a patch allowing a nvdimm_type= kernel option that allows
> to detect them, but will do nothing by default.  The code needed is very
> small and it would be very useful for all kinds of projects.
> 

I do not see why you need the nvdimm_type= kernel option at all.

I have here a script that auto detects any NvDIMM. It works with all
the chips that I have access to. And Also it has support for if you have
memmap=sss\$aaa.
For all these detected regions it will load a pmem device.

It is easy to filter for any type of memory you want. What
will the (annoying) kernel option give you?

OK I might be jumping the guns, send the patch and I'll look
at it.

Thanks
Boaz

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Ingo Molnar]

* Dan Williams <dan.j.williams@intel.com> wrote:

> On Wed, Feb 18, 2015 at 4:47 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Wed, Feb 18, 2015 at 10:15:32AM -0800, Dan Williams wrote:
> >> In fact it was originally "type-6" until ACPI 5 
> >> claimed that number for official use, so these 
> >> platforms, with early proof-of-concept nvdimm support, 
> >> have already gone through one transition to a new 
> >> number.  They need to do the same once an official 
> >> number for nvdimm support is published.
> >>
> >> Put another way, these early platforms are already 
> >> using out-of-tree patches for nvdimm enabling.  They 
> >> can continue to do so, or switch to standard methods 
> >> when the standard is published.
> >
> > Not supporting hardware that is widely avaiable (I have 
> > some, too) is not very user friendly.
> 
> Yes, as I agreed with Ingo, allowing a driver to assume 
> control of an unknown memory type with a warning or a 
> kernel taint seems fine.

If someone cooks up such a patch I can apply it.

Thanks,

	Ingo

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/18/2015 09:35 PM, Dan Williams wrote:
> On Wed, Feb 18, 2015 at 11:27 AM, Ingo Molnar <mingo@kernel.org> wrote:
>>
<>
>>>>>>>
>>>>>>> No, it seems the safe thing to do is prevent the
>>>>>>> kernel from accessing any memory that it does not know
>>>>>>> the side-effects of accessing.
>>>>>>
<>

The Kernel does not do any such access.

Reading the code the  "busy" state of the unknown type looks
like an accident to me. The code just assumes all types are
known. And did a negative check.

Consider a memory-region that sits on a PCIE slot. The BUS
does not know anything about that BAR, it just exposes that
information up the stack so a driver can positively identify
its device and drive it.

Same here. e820 is just a description of what sits on the
DDR I^2C bus as read by the BIOS (or other means). The actual
driver of these is for example the add_memory() call. Well sure
unknown-types are not added to any active use.

Parallel to what you are saying is that we band any PCI-ID
that does not have a registered device-id, the card will be
enumerated but drivers will receive "already taken" even
though no one is using it.

If you really believe in what you are saying then please
move away from the "busy" bit. "busy" bit means "taken".
Have a new bit saying. "no one told me about this device"

>>>>>> Well, except when the kernel does know how to access
>>>>>> it: when an nvdimm driver knows about its own memory
>>>>>> region and knows how to handle it, right?
>>>>>
<>
>>>>
>>>> But ... if a user is blessed/haunted with such firmware,
>>>> why not let new types fall back to 'reserved', which is a
>>>> reasonable default that still allows sufficiently aware
>>>> Linux drivers to work, right?
>>>

I want to emphasize here. After my patch the new type is
distinctly differentiated from the regular "reserved"
type so any knowledgeable driver can easily distinguish
it from a regular "reserved" region.

Only that when such driver wants to register its use
of that region it does not receive a phony busy.

[And Proceed to use that region by ignoring the phony busy]  

<>
>>
>> Well, we could emit a warning (or taint the kernel), to
>> keep the user informed that there's a version mismatch
>> between kernel and firmware - but otherwise still allow
>> informed drivers to register that region?
> 
> Sounds good to me.
> 

There is already a distinct message both at dmesg bring up:
	 user: [mem 0x0000000100000000-0x000000017fffffff] type 12
And at /proc/iomem
	100000000-1ffffffff : reserved-unknown

I would love to make this:
	100000000-1ffffffff : unknown-12

But it will need a bit of code to maintain such strings

So the information for any one that wants to look for it is there.

Do you require another redundant message who's purpose is to scare
people off, like:
	e820: WARN [mem 0x0000000100000000-0x000000017fffffff] is unknown type 12

Sure I'll add it

Thanks
Boaz

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/19/2015 12:01 PM, Ingo Molnar wrote:
> 
> * Dan Williams <dan.j.williams@intel.com> wrote:
> 
>> On Wed, Feb 18, 2015 at 4:47 PM, Christoph Hellwig <hch@infradead.org> wrote:
>>> On Wed, Feb 18, 2015 at 10:15:32AM -0800, Dan Williams wrote:
>>>> In fact it was originally "type-6" until ACPI 5 
>>>> claimed that number for official use, so these 
>>>> platforms, with early proof-of-concept nvdimm support, 
>>>> have already gone through one transition to a new 
>>>> number.  They need to do the same once an official 
>>>> number for nvdimm support is published.
>>>>
>>>> Put another way, these early platforms are already 
>>>> using out-of-tree patches for nvdimm enabling.  They 
>>>> can continue to do so, or switch to standard methods 
>>>> when the standard is published.
>>>
>>> Not supporting hardware that is widely avaiable (I have 
>>> some, too) is not very user friendly.
>>
>> Yes, as I agreed with Ingo, allowing a driver to assume 
>> control of an unknown memory type with a warning or a 
>> kernel taint seems fine.
> 
> If someone cooks up such a patch I can apply it.
> 
> Thanks,
> 
> 	Ingo
> 

I will submit a new version of my patch-1 with the pr_warn.

Or did you already apply my patch-1 and you want one on top?
What is the URL of your tree please?

Thanks
Boaz

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Ingo Molnar]

* Boaz Harrosh <boaz@plexistor.com> wrote:

> Do you require another redundant message who's purpose is to scare
> people off, like:
>
> 	e820: WARN [mem 0x0000000100000000-0x000000017fffffff] is unknown type 12
> 
> Sure I'll add it

That message looks useful (and not very scary), and also 
emit a warning if such a region is then claimed by a 
driver.

This puts some pressure on both the firmware and the kernel 
side to get their act together, without holding 
functionality hostage.

Thanks,

	Ingo

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Ingo Molnar]

* Boaz Harrosh <boaz@plexistor.com> wrote:

> On 02/19/2015 12:01 PM, Ingo Molnar wrote:
> > 
> > * Dan Williams <dan.j.williams@intel.com> wrote:
> > 
> >> On Wed, Feb 18, 2015 at 4:47 PM, Christoph Hellwig <hch@infradead.org> wrote:
> >>> On Wed, Feb 18, 2015 at 10:15:32AM -0800, Dan Williams wrote:
> >>>> In fact it was originally "type-6" until ACPI 5 
> >>>> claimed that number for official use, so these 
> >>>> platforms, with early proof-of-concept nvdimm support, 
> >>>> have already gone through one transition to a new 
> >>>> number.  They need to do the same once an official 
> >>>> number for nvdimm support is published.
> >>>>
> >>>> Put another way, these early platforms are already 
> >>>> using out-of-tree patches for nvdimm enabling.  They 
> >>>> can continue to do so, or switch to standard methods 
> >>>> when the standard is published.
> >>>
> >>> Not supporting hardware that is widely avaiable (I have 
> >>> some, too) is not very user friendly.
> >>
> >> Yes, as I agreed with Ingo, allowing a driver to assume 
> >> control of an unknown memory type with a warning or a 
> >> kernel taint seems fine.
> > 
> > If someone cooks up such a patch I can apply it.
> > 
> > Thanks,
> > 
> > 	Ingo
> > 
> 
> I will submit a new version of my patch-1 with the 
> pr_warn.
> 
> Or did you already apply my patch-1 and you want one on 
> top? What is the URL of your tree please?

New patch please, and please also Cc: everyone who 
expressed interest in the thread and who wasn't Cc:-ed to 
the original patch.

Thanks,

	Ingo

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/19/2015 12:31 PM, Ingo Molnar wrote:
<>
>> I will submit a new version of my patch-1 with the 
>> pr_warn.
>>
>> Or did you already apply my patch-1 and you want one on 
>> top? What is the URL of your tree please?
> 
> New patch please, and please also Cc: everyone who 
> expressed interest in the thread and who wasn't Cc:-ed to 
> the original patch.
> 
> Thanks,
> 
> 	Ingo
> 

Thank you Ingo

I will submit V2 of my patch-1, with the changes you requested

And also add a new patch that adds the "warning if such a region is then claimed"
Because this will need to go to a generic part of the Kernel.

working on it now

Thanks again
Boaz

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Christoph Hellwig]

On Thu, Feb 19, 2015 at 11:25:37AM +0200, Boaz Harrosh wrote:
> I do not see why you need the nvdimm_type= kernel option at all.
> 
> I have here a script that auto detects any NvDIMM. It works with all
> the chips that I have access to. And Also it has support for if you have
> memmap=sss\$aaa.
> For all these detected regions it will load a pmem device.
> 
> It is easy to filter for any type of memory you want. What
> will the (annoying) kernel option give you?
> 
> OK I might be jumping the guns, send the patch and I'll look
> at it.

The kernel option means we can autodetect the nvdimms in kernel space
with just that option set, no need for needing userspace setup.

How does your script / patch work?

I won't be back to my nvdimm enabled hardware until after LSF/MM,
so any work from me in this area will have to wait a bit.

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/22/2015 06:27 PM, Christoph Hellwig wrote:
> On Thu, Feb 19, 2015 at 11:25:37AM +0200, Boaz Harrosh wrote:
>> I do not see why you need the nvdimm_type= kernel option at all.
>>
>> I have here a script that auto detects any NvDIMM. It works with all
>> the chips that I have access to. And Also it has support for if you have
>> memmap=sss\$aaa.
>> For all these detected regions it will load a pmem device.
>>
>> It is easy to filter for any type of memory you want. What
>> will the (annoying) kernel option give you?
>>
>> OK I might be jumping the guns, send the patch and I'll look
>> at it.
> 
> The kernel option means we can autodetect the nvdimms in kernel space
> with just that option set, no need for needing userspace setup.
> 

Do you mean that pmem is:
- Loaded without any parameters.
- A new API is defined for enumerating NvDIMMs. pmem uses that for
  creating new devices.
- e820 registers such devices according to special type passed on Kernel
  command-line. (That could be a Kconfig as well you know, I hate Kernel
  command-line)

(Similar to what very old prd.c did only not hacking the e820 lists directly)

> How does your script / patch work?
> 

Easy just parses /proc/iomem + looks at Kernel command line.

It has support not only for type-12 memory but also for "reserved"
regions, made by memmap=sss$aaa stated on command line. And/or also
an /etc/pmem.cfg list of regions. So you can go automatic or manual or
a mix of both (And slice a region for xfstests scratch device).

In any case it helps to have my patches, but also old Kernels are
supported with the BLK_DEV_PMEM_IGNORE_REQUEST_MEM_RET. On old Kernels
they are all "reserved" regions, no unknown-12 type.

Once a list of regions or split-regions is established pmem is loaded
with that list.

> I won't be back to my nvdimm enabled hardware until after LSF/MM,
> so any work from me in this area will have to wait a bit.
> 

Have a good time. Will you guys have a talk about pmem ? If yes I
would love it if you guys can talk about the use-of-pages-with-pmem.
Please tell me I can write a looong argument about it.

Thanks
Boaz

Re: [Linux-nvdimm] [PATCH 0/2] e820: Fix handling of NvDIMM chips

[Boaz Harrosh]

On 02/22/2015 07:05 PM, Boaz Harrosh wrote:
> On 02/22/2015 06:27 PM, Christoph Hellwig wrote:
<>
>> I won't be back to my nvdimm enabled hardware until after LSF/MM,
>> so any work from me in this area will have to wait a bit.
>>
<>

BTW: I have an out-of-tree patch that enables me to just emulate
an NvDIMM inside a VM, so I can develop at the comfort of my LP.

You can find it at my pmem tree.

All it does is adds a new type of memmap=.  So if you specify memmap=2G\&4G on
Kernel command-line the code will insert a type-12 memory to the list, instead of "reserved"
region when a "$" is parsed.

>From here on that region will look exactly like an DDR3-NvDIMM with
supporting BIOS to the system, safe the persistence of course.
(Actually there is a trick to force the KVM-Host to reserve that memory
 across VM boots, to emulate persistence)

Cheers
Boaz