[PATCH] KVM/MIPS32: Export min_low_pfn.

[PATCH] KVM/MIPS32: Export min_low_pfn.

[Sanjay Lal]

The KVM module uses the standard MIPS cache management routines, which use min_low_pfn.
This creates and indirect dependency, requiring min_low_pfn to be exported.

Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
---
 arch/mips/kernel/mips_ksyms.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
index 6e58e97..0299472 100644
--- a/arch/mips/kernel/mips_ksyms.c
+++ b/arch/mips/kernel/mips_ksyms.c
@@ -14,6 +14,7 @@
 #include <linux/mm.h>
 #include <asm/uaccess.h>
 #include <asm/ftrace.h>
+#include <linux/bootmem.h>
 
 extern void *__bzero(void *__s, size_t __count);
 extern long __strncpy_from_user_nocheck_asm(char *__to,
@@ -60,3 +61,8 @@ EXPORT_SYMBOL(invalid_pte_table);
 /* _mcount is defined in arch/mips/kernel/mcount.S */
 EXPORT_SYMBOL(_mcount);
 #endif
+
+/* The KVM module uses the standard MIPS cache functions which use
+ * min_low_pfn, requiring it to be exported.
+ */
+EXPORT_SYMBOL(min_low_pfn);
-- 
1.7.11.3

Re: [PATCH] KVM/MIPS32: Export min_low_pfn.

[David Daney]

On 05/17/2013 02:06 PM, Sanjay Lal wrote:
> The KVM module uses the standard MIPS cache management routines, which use min_low_pfn.
> This creates and indirect dependency, requiring min_low_pfn to be exported.
>
> Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> ---
>   arch/mips/kernel/mips_ksyms.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
> index 6e58e97..0299472 100644
> --- a/arch/mips/kernel/mips_ksyms.c
> +++ b/arch/mips/kernel/mips_ksyms.c
> @@ -14,6 +14,7 @@
>   #include <linux/mm.h>
>   #include <asm/uaccess.h>
>   #include <asm/ftrace.h>
> +#include <linux/bootmem.h>
>
>   extern void *__bzero(void *__s, size_t __count);
>   extern long __strncpy_from_user_nocheck_asm(char *__to,
> @@ -60,3 +61,8 @@ EXPORT_SYMBOL(invalid_pte_table);
>   /* _mcount is defined in arch/mips/kernel/mcount.S */
>   EXPORT_SYMBOL(_mcount);
>   #endif
> +
> +/* The KVM module uses the standard MIPS cache functions which use
> + * min_low_pfn, requiring it to be exported.
> + */
> +EXPORT_SYMBOL(min_low_pfn);

I think I asked this before, but I don't remember the answer:

Why not put EXPORT_SYMBOL(min_low_pfn) in mm/bootmem.c adjacent to where 
the symbol is defined?

Cluttering up the kernel with multiple architectures all doing 
architecture specific exports of the same symbol is not a clean way of 
doing things.

The second time something needs to be done, it should be factored out 
into common code.

David Daney

>

Re: [PATCH] KVM/MIPS32: Export min_low_pfn.

[Gleb Natapov]

On Fri, May 17, 2013 at 02:42:48PM -0700, David Daney wrote:
> On 05/17/2013 02:06 PM, Sanjay Lal wrote:
> >The KVM module uses the standard MIPS cache management routines, which use min_low_pfn.
> >This creates and indirect dependency, requiring min_low_pfn to be exported.
> >
> >Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> >---
> >  arch/mips/kernel/mips_ksyms.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> >diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
> >index 6e58e97..0299472 100644
> >--- a/arch/mips/kernel/mips_ksyms.c
> >+++ b/arch/mips/kernel/mips_ksyms.c
> >@@ -14,6 +14,7 @@
> >  #include <linux/mm.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/ftrace.h>
> >+#include <linux/bootmem.h>
> >
> >  extern void *__bzero(void *__s, size_t __count);
> >  extern long __strncpy_from_user_nocheck_asm(char *__to,
> >@@ -60,3 +61,8 @@ EXPORT_SYMBOL(invalid_pte_table);
> >  /* _mcount is defined in arch/mips/kernel/mcount.S */
> >  EXPORT_SYMBOL(_mcount);
> >  #endif
> >+
> >+/* The KVM module uses the standard MIPS cache functions which use
> >+ * min_low_pfn, requiring it to be exported.
> >+ */
> >+EXPORT_SYMBOL(min_low_pfn);
> 
> I think I asked this before, but I don't remember the answer:
> 
> Why not put EXPORT_SYMBOL(min_low_pfn) in mm/bootmem.c adjacent to
> where the symbol is defined?
> 
He did answered here:
http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/109895.

I suggested mips_ksyms.c solution as an option.

> Cluttering up the kernel with multiple architectures all doing
> architecture specific exports of the same symbol is not a clean way
> of doing things.
> 
> The second time something needs to be done, it should be factored
> out into common code.
> 
Exports are different. You define interface between the kernel and modules
here, exporting the symbol may not be desirable for some arch. And 
min_low_pfn is not the only example. Anything in arch _ksyms files is like that:
exported by some archs but not the others. 

--
			Gleb.

Re: [PATCH] KVM/MIPS32: Export min_low_pfn.

[Ralf Baechle]

On Fri, May 17, 2013 at 02:42:48PM -0700, David Daney wrote:

> On 05/17/2013 02:06 PM, Sanjay Lal wrote:
> >The KVM module uses the standard MIPS cache management routines, which use min_low_pfn.
> >This creates and indirect dependency, requiring min_low_pfn to be exported.
> >
> >Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> >---
> >  arch/mips/kernel/mips_ksyms.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> >diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
> >index 6e58e97..0299472 100644
> >--- a/arch/mips/kernel/mips_ksyms.c
> >+++ b/arch/mips/kernel/mips_ksyms.c
> >@@ -14,6 +14,7 @@
> >  #include <linux/mm.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/ftrace.h>
> >+#include <linux/bootmem.h>
> >
> >  extern void *__bzero(void *__s, size_t __count);
> >  extern long __strncpy_from_user_nocheck_asm(char *__to,
> >@@ -60,3 +61,8 @@ EXPORT_SYMBOL(invalid_pte_table);
> >  /* _mcount is defined in arch/mips/kernel/mcount.S */
> >  EXPORT_SYMBOL(_mcount);
> >  #endif
> >+
> >+/* The KVM module uses the standard MIPS cache functions which use
> >+ * min_low_pfn, requiring it to be exported.

The comment is wrong.  min_low_pfn is being referenced by pfn_valid() which
is implemented (simplified for purposes of this discussion) like:

int pfn_valid(unsigned long pfn)
{
	return pfn >= min_low_pfn && pfn < max_mapnr;
}

> >+ */
> >+EXPORT_SYMBOL(min_low_pfn);
> 
> I think I asked this before, but I don't remember the answer:
> 
> Why not put EXPORT_SYMBOL(min_low_pfn) in mm/bootmem.c adjacent to
> where the symbol is defined?
> 
> Cluttering up the kernel with multiple architectures all doing
> architecture specific exports of the same symbol is not a clean way
> of doing things.
> 
> The second time something needs to be done, it should be factored
> out into common code.

pfn_valid() should return 1 if for the pfn passed as the argment a struct
page exists in mem_map[] - this only affects the flatmem case.  However
it is possible that min_low_pfn is bigger than ARCH_PFN_OFFSET in which
case pfn_valid might return 0 even though it should have returned 1.

So I fixed the issue like:

static inline int pfn_valid(unsigned long pfn)
{
	return pfn >= ARCH_PFN_OFFSET && pfn < max_mapnr;
}

frv, ia64, metag, microblaze, score and sh are also implementing pfn_valid
referencing min_low_pfn.

Time to move pfn_valid() to asm-generic, fix the min_low_pfn /
ARCH_PFN_OFFSET issue and while at it, harden it against multiple
evaluation of its arguments?

  Ralf

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] KVM/MIPS32: Export min_low_pfn.

[Ralf Baechle]

On Fri, May 17, 2013 at 02:42:48PM -0700, David Daney wrote:

> On 05/17/2013 02:06 PM, Sanjay Lal wrote:
> >The KVM module uses the standard MIPS cache management routines, which use min_low_pfn.
> >This creates and indirect dependency, requiring min_low_pfn to be exported.
> >
> >Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> >---
> >  arch/mips/kernel/mips_ksyms.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> >diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
> >index 6e58e97..0299472 100644
> >--- a/arch/mips/kernel/mips_ksyms.c
> >+++ b/arch/mips/kernel/mips_ksyms.c
> >@@ -14,6 +14,7 @@
> >  #include <linux/mm.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/ftrace.h>
> >+#include <linux/bootmem.h>
> >
> >  extern void *__bzero(void *__s, size_t __count);
> >  extern long __strncpy_from_user_nocheck_asm(char *__to,
> >@@ -60,3 +61,8 @@ EXPORT_SYMBOL(invalid_pte_table);
> >  /* _mcount is defined in arch/mips/kernel/mcount.S */
> >  EXPORT_SYMBOL(_mcount);
> >  #endif
> >+
> >+/* The KVM module uses the standard MIPS cache functions which use
> >+ * min_low_pfn, requiring it to be exported.

The comment is wrong.  min_low_pfn is being referenced by pfn_valid() which
is implemented (simplified for purposes of this discussion) like:

int pfn_valid(unsigned long pfn)
{
	return pfn >= min_low_pfn && pfn < max_mapnr;
}

> >+ */
> >+EXPORT_SYMBOL(min_low_pfn);
> 
> I think I asked this before, but I don't remember the answer:
> 
> Why not put EXPORT_SYMBOL(min_low_pfn) in mm/bootmem.c adjacent to
> where the symbol is defined?
> 
> Cluttering up the kernel with multiple architectures all doing
> architecture specific exports of the same symbol is not a clean way
> of doing things.
> 
> The second time something needs to be done, it should be factored
> out into common code.

pfn_valid() should return 1 if for the pfn passed as the argment a struct
page exists in mem_map[] - this only affects the flatmem case.  However
it is possible that min_low_pfn is bigger than ARCH_PFN_OFFSET in which
case pfn_valid might return 0 even though it should have returned 1.

So I fixed the issue like:

static inline int pfn_valid(unsigned long pfn)
{
	return pfn >= ARCH_PFN_OFFSET && pfn < max_mapnr;
}

frv, ia64, metag, microblaze, score and sh are also implementing pfn_valid
referencing min_low_pfn.

Time to move pfn_valid() to asm-generic, fix the min_low_pfn /
ARCH_PFN_OFFSET issue and while at it, harden it against multiple
evaluation of its arguments?

  Ralf

Re: [PATCH] KVM/MIPS32: Export min_low_pfn.

[Ralf Baechle]

On Fri, May 17, 2013 at 02:42:48PM -0700, David Daney wrote:

> On 05/17/2013 02:06 PM, Sanjay Lal wrote:
> >The KVM module uses the standard MIPS cache management routines, which use min_low_pfn.
> >This creates and indirect dependency, requiring min_low_pfn to be exported.
> >
> >Signed-off-by: Sanjay Lal <sanjayl@kymasys.com>
> >---
> >  arch/mips/kernel/mips_ksyms.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> >diff --git a/arch/mips/kernel/mips_ksyms.c b/arch/mips/kernel/mips_ksyms.c
> >index 6e58e97..0299472 100644
> >--- a/arch/mips/kernel/mips_ksyms.c
> >+++ b/arch/mips/kernel/mips_ksyms.c
> >@@ -14,6 +14,7 @@
> >  #include <linux/mm.h>
> >  #include <asm/uaccess.h>
> >  #include <asm/ftrace.h>
> >+#include <linux/bootmem.h>
> >
> >  extern void *__bzero(void *__s, size_t __count);
> >  extern long __strncpy_from_user_nocheck_asm(char *__to,
> >@@ -60,3 +61,8 @@ EXPORT_SYMBOL(invalid_pte_table);
> >  /* _mcount is defined in arch/mips/kernel/mcount.S */
> >  EXPORT_SYMBOL(_mcount);
> >  #endif
> >+
> >+/* The KVM module uses the standard MIPS cache functions which use
> >+ * min_low_pfn, requiring it to be exported.

The comment is wrong.  min_low_pfn is being referenced by pfn_valid() which
is implemented (simplified for purposes of this discussion) like:

int pfn_valid(unsigned long pfn)
{
	return pfn >= min_low_pfn && pfn < max_mapnr;
}

> >+ */
> >+EXPORT_SYMBOL(min_low_pfn);
> 
> I think I asked this before, but I don't remember the answer:
> 
> Why not put EXPORT_SYMBOL(min_low_pfn) in mm/bootmem.c adjacent to
> where the symbol is defined?
> 
> Cluttering up the kernel with multiple architectures all doing
> architecture specific exports of the same symbol is not a clean way
> of doing things.
> 
> The second time something needs to be done, it should be factored
> out into common code.

pfn_valid() should return 1 if for the pfn passed as the argment a struct
page exists in mem_map[] - this only affects the flatmem case.  However
it is possible that min_low_pfn is bigger than ARCH_PFN_OFFSET in which
case pfn_valid might return 0 even though it should have returned 1.

So I fixed the issue like:

static inline int pfn_valid(unsigned long pfn)
{
	return pfn >= ARCH_PFN_OFFSET && pfn < max_mapnr;
}

frv, ia64, metag, microblaze, score and sh are also implementing pfn_valid
referencing min_low_pfn.

Time to move pfn_valid() to asm-generic, fix the min_low_pfn /
ARCH_PFN_OFFSET issue and while at it, harden it against multiple
evaluation of its arguments?

  Ralf