| Message ID | 1368824818-22503-1-git-send-email-sanjayl@kymasys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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(+)