Skip to contentSkip to conversation

paravirt: isolate module ops

PATCH Applied Ingo Molnar 6 participants 23 messages Jan 6, 2007 – Jan 8, 2007

Subsystems (4)
Memory management MM core Char and misc drivers PARAVIRT_OPS interface
lore
Share & tools
lore

Ingo Molnar

author
#0 lore raw

PATCH 1/1 paravirt: isolate module ops

Subject: [patch] paravirt: isolate module ops
From: Ingo Molnar <[email protected]>

only export those operations to modules that have been available to them
historically: irq disable/enable, io-delay, udelay, etc.

this isolates that functionality from other paravirtualization functionality that modules have no business messing with.

boot and build tested with CONFIG_PARAVIRT=y.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/paravirt.c |   41 +++++++++++++++++++++++++++
 include/asm-i386/delay.h    |    4 +-
 include/asm-i386/paravirt.h |   65 ++++++++++++++++++++++----------------------
 3 files changed, 75 insertions(+), 35 deletions(-)
arch/i386/kernel/paravirt.c +40 -1
Index: linux/arch/i386/kernel/paravirt.c
===================================================================
@@ -492,6 +492,7 @@ struct paravirt_ops paravirt_ops = {
 
  	.patch = native_patch,
 	.banner = default_banner,
+
 	.arch_setup = native_nop,
 	.memory_setup = machine_specific_memory_setup,
 	.get_wallclock = native_get_wallclock,

@@ -566,4 +567,42 @@ struct paravirt_ops paravirt_ops = {
 	.irq_enable_sysexit = native_irq_enable_sysexit,
 	.iret = native_iret,
 };
-EXPORT_SYMBOL(paravirt_ops);
+
+/*
+ * These are exported to modules:
+ */
+struct paravirt_ops paravirt_mod_ops = {
+	.name = "bare hardware",
+	.paravirt_enabled = 0,
+	.kernel_rpl = 0,
+
+ 	.patch = native_patch,
+	.banner = default_banner,
+
+	.save_fl = native_save_fl,
+	.restore_fl = native_restore_fl,
+	.irq_disable = native_irq_disable,
+	.irq_enable = native_irq_enable,
+
+	.cpuid = native_cpuid,
+
+	.read_msr = native_read_msr,
+	.write_msr = native_write_msr,
+
+	.read_tsc = native_read_tsc,
+	.read_pmc = native_read_pmc,
+
+	.io_delay = native_io_delay,
+	.const_udelay = __const_udelay,
+
+#ifdef CONFIG_X86_LOCAL_APIC
+	.apic_write = native_apic_write,
+	.apic_write_atomic = native_apic_write_atomic,
+	.apic_read = native_apic_read,
+#endif
+
+	.flush_tlb_user = native_flush_tlb,
+	.flush_tlb_kernel = native_flush_tlb_global,
+	.flush_tlb_single = native_flush_tlb_single,
+};
+EXPORT_SYMBOL(paravirt_mod_ops);
include/asm-i386/delay.h +2 -2
Index: linux/include/asm-i386/delay.h
===================================================================
@@ -17,9 +17,9 @@ extern void __const_udelay(unsigned long
 extern void __delay(unsigned long loops);
 
 #if defined(CONFIG_PARAVIRT) && !defined(USE_REAL_TIME_DELAY)
-#define udelay(n) paravirt_ops.const_udelay((n) * 0x10c7ul)
+#define udelay(n) paravirt_mod_ops.const_udelay((n) * 0x10c7ul)
 
-#define ndelay(n) paravirt_ops.const_udelay((n) * 5ul)
+#define ndelay(n) paravirt_mod_ops.const_udelay((n) * 5ul)
 
 #else /* !PARAVIRT || USE_REAL_TIME_DELAY */
include/asm-i386/paravirt.h +33 -32

195 lines not on this page · raw message

#1 lore raw
Quoted 34 lines · Ingo Molnar

Subject: [patch] paravirt: isolate module ops
From: Ingo Molnar <[email protected]>

only export those operations to modules that have been available to them historically: irq disable/enable, io-delay, udelay, etc.

this isolates that functionality from other paravirtualization functionality that modules have no business messing with.

boot and build tested with CONFIG_PARAVIRT=y.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/paravirt.c |   41 +++++++++++++++++++++++++++
 include/asm-i386/delay.h    |    4 +-
 include/asm-i386/paravirt.h |   65 ++++++++++++++++++++++----------------------
 3 files changed, 75 insertions(+), 35 deletions(-)

Index: linux/arch/i386/kernel/paravirt.c
===================================================================

arch/i386/kernel/paravirt.c +1 -1
@@ -492,6 +492,7 @@ struct paravirt_ops paravirt_ops = {
 
  	.patch = native_patch,
 	.banner = default_banner,
+
 	.arch_setup = native_nop,
 	.memory_setup = machine_specific_memory_setup,
 	.get_wallclock = native_get_wallclock,

@@ -566,4 +567,42 @@ struct paravirt_ops paravirt_ops = {
 	.irq_enable_sysexit = native_irq_enable_sysexit,
 	.iret = native_iret,
 };
-EXPORT_SYMBOL(paravirt_ops);

arch/i386/kernel/paravirt.c struct paravirt_ops paravirt_ops = { · line 579

Quoted 10 lines
arch/i386/kernel/paravirt.c +10 -0
+
+/*
+ * These are exported to modules:
+ */
+struct paravirt_ops paravirt_mod_ops = {
+	.name = "bare hardware",
+	.paravirt_enabled = 0,
+	.kernel_rpl = 0,
+
+ 	.patch = native_patch,

I don't think you want to leave that one... patching the kernel isn't something modules should be doing.

Perhaps a separate structure is better for module ops - this seems error prone to calling the wrong one of paravirt_ops vs. paravirt_mod_ops, also error prone to set them up in the code which sets paravirt_ops for each hypervisor. Although that does require more dissection, it does make sure everyone gets it right, and makes the interface very explicitly clear in the header file, which is nice.

If you feel strongly about this, I suggest you push it upstream now, not into Andrew's tree (it doesn't apply cleanly there, and breaks the VMI patches which are in there). But this is no problem, I can fix that up easily once you get it in.

Zach

#2 lore raw
Quoted 42 lines · Zachary Amsden
Quoted 41 lines · Ingo Molnar

Subject: [patch] paravirt: isolate module ops
From: Ingo Molnar <[email protected]>

only export those operations to modules that have been available to them
historically: irq disable/enable, io-delay, udelay, etc.

this isolates that functionality from other paravirtualization functionality that modules have no business messing with.

boot and build tested with CONFIG_PARAVIRT=y.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/paravirt.c |   41 +++++++++++++++++++++++++++
 include/asm-i386/delay.h    |    4 +-
 include/asm-i386/paravirt.h |   65 ++++++++++++++++++++++----------------------
 3 files changed, 75 insertions(+), 35 deletions(-)

Index: linux/arch/i386/kernel/paravirt.c
===================================================================

arch/i386/kernel/paravirt.c +7 -1
@@ -492,6 +492,7 @@ struct paravirt_ops paravirt_ops = {
 
  	.patch = native_patch,
 	.banner = default_banner,
+
 	.arch_setup = native_nop,
 	.memory_setup = machine_specific_memory_setup,
 	.get_wallclock = native_get_wallclock,

@@ -566,4 +567,42 @@ struct paravirt_ops paravirt_ops = {
 	.irq_enable_sysexit = native_irq_enable_sysexit,
 	.iret = native_iret,
 };
-EXPORT_SYMBOL(paravirt_ops);
+
+/*
+ * These are exported to modules:
+ */
+struct paravirt_ops paravirt_mod_ops = {
+	.name = "bare hardware",
Quoted 8 lines

arch/i386/kernel/paravirt.c struct paravirt_ops paravirt_ops = { · line 579

Quoted 5 lines
arch/i386/kernel/paravirt.c +4 -0
+	.paravirt_enabled = 0,
+	.kernel_rpl = 0,
+
+ 	.patch = native_patch,

I don't think you want to leave that one... patching the kernel isn't something modules should be doing.

Yeah, this patch is terrible, but I know why Ingo did it; it's my fault for not finishing my version yesterday (but allmodconfig takes a while to build and every change to paravirt.h rebuilds the universe...).

So here's my variant, compile-tested with "make allmodconfig". Exports individual functions, some of which can be reduced over time as the modules involved are whacked into shape.

Note: it reduces the patching space by 1 byte (direct jumps vs indirect jumps). If this a problem for any paravirt_ops backend in practice, we can add noops...

Cheers,
Rusty.
PS. May break with other configurations...

Name: don't export paravirt_ops structure, do individual functions

Some of the more obscure ones are only used by one or two modules. We can almost certainly reduce them with more work.

Signed-off-by Rusty Russell <[email protected]>
arch/i386/kernel/paravirt.c +148 -1
diff -r 48f31ae5d7b5 arch/i386/kernel/paravirt.c
@@ -596,6 +596,154 @@ static int __init print_banner(void)
 	return 0;
 }
 core_initcall(print_banner);
+
+unsigned long paravirt_save_flags(void)
+{
+	return paravirt_ops.save_fl();
+}
+EXPORT_SYMBOL(paravirt_save_flags);
+
+void paravirt_restore_flags(unsigned long flags)
+{
+	paravirt_ops.restore_fl(flags);
+}
+EXPORT_SYMBOL(paravirt_restore_flags);
+
+void paravirt_irq_disable(void)
+{
+	paravirt_ops.irq_disable();
+}
+EXPORT_SYMBOL(paravirt_irq_disable);
+
+void paravirt_irq_enable(void)
+{
+	paravirt_ops.irq_enable();
+}
+EXPORT_SYMBOL(paravirt_irq_enable);
+
+void paravirt_io_delay(void)
+{
+	paravirt_ops.io_delay();
+}
+EXPORT_SYMBOL(paravirt_io_delay);
+
+void paravirt_const_udelay(unsigned long loops)
+{
+	paravirt_ops.const_udelay(loops);
+}
+EXPORT_SYMBOL(paravirt_const_udelay);
+
+u64 paravirt_read_msr(unsigned int msr, int *err)
+{
+	return paravirt_ops.read_msr(msr, err);
+}
+EXPORT_SYMBOL(paravirt_read_msr);
+
+int paravirt_write_msr(unsigned int msr, u64 val)
+{
+	return paravirt_ops.write_msr(msr, val);
+}
+EXPORT_SYMBOL(paravirt_write_msr);
+
+u64 paravirt_read_tsc(void)
+{
+	return paravirt_ops.read_tsc();
+}
+EXPORT_SYMBOL(paravirt_read_tsc);
+
+int paravirt_enabled(void)
+{
+	return paravirt_ops.paravirt_enabled;
+}
+EXPORT_SYMBOL(paravirt_enabled);
+
+void clts(void)
+{
+	paravirt_ops.clts();
+}
+EXPORT_SYMBOL(clts);
+
+unsigned long read_cr0(void)
+{
+	return paravirt_ops.read_cr0();
+}
+EXPORT_SYMBOL(read_cr0);
+
+void write_cr0(unsigned long cr0)
+{
+	paravirt_ops.write_cr0(cr0);
+}
+EXPORT_SYMBOL(write_cr0);
+
+void wbinvd(void)
+{
+	paravirt_ops.wbinvd();
+}
+EXPORT_SYMBOL(wbinvd);
+
+void raw_safe_halt(void)
+{
+	paravirt_ops.safe_halt();
+}
+EXPORT_SYMBOL(raw_safe_halt);
+
+void halt(void)
+{
+	paravirt_ops.safe_halt();
+}
+EXPORT_SYMBOL(halt);
+
+#ifdef CONFIG_X86_LOCAL_APIC
+void apic_write(unsigned long reg, unsigned long v)
+{
+	paravirt_ops.apic_write(reg,v);
+}
+EXPORT_SYMBOL_GPL(apic_write);
+
+unsigned long apic_read(unsigned long reg)
+{
+	return paravirt_ops.apic_read(reg);
+}
+EXPORT_SYMBOL_GPL(apic_read);
+#endif
+
+void __cpuid(unsigned int *eax, unsigned int *ebx,
+	     unsigned int *ecx, unsigned int *edx)
+{
+	paravirt_ops.cpuid(eax, ebx, ecx, edx);
+}
+EXPORT_SYMBOL(__cpuid);
+
+#ifdef CONFIG_X86_PAE
+unsigned long long pgd_val(pgd_t x)
+{
+	return paravirt_ops.pgd_val(x);
+}
+unsigned long long pte_val(pte_t x)
+{
+	return paravirt_ops.pte_val(x);
+}
+pte_t __pte(unsigned long long x)
+{
+	return paravirt_ops.make_pte(x);
+}
+#else
+unsigned long pgd_val(pgd_t x)
+{
+	return paravirt_ops.pgd_val(x);
+}
+unsigned long pte_val(pte_t x)
+{
+	return paravirt_ops.pte_val(x);
+}
+pte_t __pte(unsigned long x)
+{
+	return paravirt_ops.make_pte(x);
+}
+#endif
+EXPORT_SYMBOL_GPL(pgd_val);
+EXPORT_SYMBOL_GPL(pte_val);
+EXPORT_SYMBOL_GPL(__pte);
 
 /* We simply declare start_kernel to be the paravirt probe of last resort. */
 paravirt_probe(start_kernel);

@@ -710,4 +858,3 @@ struct paravirt_ops paravirt_ops = {
 
 	.startup_ipi_hook = (void *)native_nop,
 };
-EXPORT_SYMBOL(paravirt_ops);
include/asm-i386/delay.h +2 -2
diff -r 48f31ae5d7b5 include/asm-i386/delay.h
@@ -17,9 +17,9 @@ extern void __delay(unsigned long loops)
 extern void __delay(unsigned long loops);
 
 #if defined(CONFIG_PARAVIRT) && !defined(USE_REAL_TIME_DELAY)
-#define udelay(n) paravirt_ops.const_udelay((n) * 0x10c7ul)
+#define udelay(n) paravirt_const_udelay((n) * 0x10c7ul)
 
-#define ndelay(n) paravirt_ops.const_udelay((n) * 5ul)
+#define ndelay(n) paravirt_const_udelay((n) * 5ul)
 
 #else /* !PARAVIRT || USE_REAL_TIME_DELAY */
include/asm-i386/paravirt.h +60 -66

256 lines not on this page · raw message

include/linux/irqflags.h +5 -5
diff -r 48f31ae5d7b5 include/linux/irqflags.h
@@ -74,11 +74,11 @@
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
-#define safe_halt()						\
-	do {							\
-		trace_hardirqs_on();				\
-		raw_safe_halt();				\
-	} while (0)
+static inline void safe_halt(void)
+{
+	trace_hardirqs_on();
+	raw_safe_halt();
+}
 
 #define local_save_flags(flags)		raw_local_save_flags(flags)
author
#3 lore raw

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 604

Quoted 13 lines · Rusty Russell

diff -r 48f31ae5d7b5 arch/i386/kernel/paravirt.c

arch/i386/kernel/paravirt.c +6 -0
@@ -596,6 +596,154 @@ static int __init print_banner(void)
 	return 0;
 }
 core_initcall(print_banner);
+
+unsigned long paravirt_save_flags(void)
+{
+	return paravirt_ops.save_fl();
+}
+EXPORT_SYMBOL(paravirt_save_flags);

ok, i like this one too - i agree that it's better than mine because it isolates on a per-API level not on a per-lowlevel-paravirt-op level. But this doesnt do the most crutial step: the removal of the paravirt_ops export. Without that the module build test is pointless.

btw., your patch does not apply to current -git - could you please rebase this patch to the head of your queue so that upstream can pick it up?

Ingo

author
#4 lore raw
Quoted 2 lines · Ingo Molnar

this doesnt do the most crutial step: the removal of the paravirt_ops export. [...]

ah, you removed it already ... it hid at the very last line of the patch chunk. Good :)

Ingo

#5 lore raw
In reply to Ingo Molnar
Quoted 3 lines · Ingo Molnar

btw., your patch does not apply to current -git - could you please rebase this patch to the head of your queue so that upstream can pick it up?

OK, here it is against rc3-git4.

Name: don't export paravirt_ops structure, do individual functions

Wrap the paravirt_ops members we want to export in wrapper functions. Since we binary-patch the critical ones, this doesn't make a speed impact.

I moved drm_follow_page into the core, to avoid having to wrap the various pte ops. Unlining kernel_fpu_end and using that in the RAID6 code would remove the need to export clts/read_cr0/write_cr0 too.

Signed-off-by Rusty Russell <[email protected]>
arch/i386/kernel/paravirt.c +117 -1
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/dontdiff --minimal linux-2.6.20-rc3-git4/arch/i386/kernel/paravirt.c working-2.6.20-rc3-git4/arch/i386/kernel/paravirt.c
@@ -482,6 +482,123 @@ static int __init print_banner(void)
 }
 core_initcall(print_banner);
 
+unsigned long paravirt_save_flags(void)
+{
+	return paravirt_ops.save_fl();
+}
+EXPORT_SYMBOL(paravirt_save_flags);
+
+void paravirt_restore_flags(unsigned long flags)
+{
+	paravirt_ops.restore_fl(flags);
+}
+EXPORT_SYMBOL(paravirt_restore_flags);
+
+void paravirt_irq_disable(void)
+{
+	paravirt_ops.irq_disable();
+}
+EXPORT_SYMBOL(paravirt_irq_disable);
+
+void paravirt_irq_enable(void)
+{
+	paravirt_ops.irq_enable();
+}
+EXPORT_SYMBOL(paravirt_irq_enable);
+
+void paravirt_io_delay(void)
+{
+	paravirt_ops.io_delay();
+}
+EXPORT_SYMBOL(paravirt_io_delay);
+
+void paravirt_const_udelay(unsigned long loops)
+{
+	paravirt_ops.const_udelay(loops);
+}
+EXPORT_SYMBOL(paravirt_const_udelay);
+
+u64 paravirt_read_msr(unsigned int msr, int *err)
+{
+	return paravirt_ops.read_msr(msr, err);
+}
+EXPORT_SYMBOL(paravirt_read_msr);
+
+int paravirt_write_msr(unsigned int msr, u64 val)
+{
+	return paravirt_ops.write_msr(msr, val);
+}
+EXPORT_SYMBOL(paravirt_write_msr);
+
+u64 paravirt_read_tsc(void)
+{
+	return paravirt_ops.read_tsc();
+}
+EXPORT_SYMBOL(paravirt_read_tsc);
+
+int paravirt_enabled(void)
+{
+	return paravirt_ops.paravirt_enabled;
+}
+EXPORT_SYMBOL(paravirt_enabled);
+
+void clts(void)
+{
+	paravirt_ops.clts();
+}
+EXPORT_SYMBOL(clts);
+
+unsigned long read_cr0(void)
+{
+	return paravirt_ops.read_cr0();
+}
+EXPORT_SYMBOL_GPL(read_cr0);
+
+void write_cr0(unsigned long cr0)
+{
+	paravirt_ops.write_cr0(cr0);
+}
+EXPORT_SYMBOL_GPL(write_cr0);
+
+void wbinvd(void)
+{
+	paravirt_ops.wbinvd();
+}
+EXPORT_SYMBOL(wbinvd);
+
+void raw_safe_halt(void)
+{
+	paravirt_ops.safe_halt();
+}
+EXPORT_SYMBOL_GPL(raw_safe_halt);
+
+void halt(void)
+{
+	paravirt_ops.safe_halt();
+}
+EXPORT_SYMBOL_GPL(halt);
+
+#ifdef CONFIG_X86_LOCAL_APIC
+void apic_write(unsigned long reg, unsigned long v)
+{
+	paravirt_ops.apic_write(reg,v);
+}
+EXPORT_SYMBOL_GPL(apic_write);
+
+unsigned long apic_read(unsigned long reg)
+{
+	return paravirt_ops.apic_read(reg);
+}
+EXPORT_SYMBOL_GPL(apic_read);
+#endif
+
+void __cpuid(unsigned int *eax, unsigned int *ebx,
+	     unsigned int *ecx, unsigned int *edx)
+{
+	paravirt_ops.cpuid(eax, ebx, ecx, edx);
+}
+EXPORT_SYMBOL(__cpuid);
+
 /* We simply declare start_kernel to be the paravirt probe of last resort. */
 paravirt_probe(start_kernel);
 

@@ -566,4 +683,3 @@ struct paravirt_ops paravirt_ops = {
 	.irq_enable_sysexit = native_irq_enable_sysexit,
 	.iret = native_iret,
 };
-EXPORT_SYMBOL(paravirt_ops);
drivers/char/drm/drm_memory.h +1 -5
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/dontdiff --minimal linux-2.6.20-rc3-git4/drivers/char/drm/drm_memory.h working-2.6.20-rc3-git4/drivers/char/drm/drm_memory.h
@@ -58,11 +58,7 @@
 
 static inline unsigned long drm_follow_page(void *vaddr)
 {
-	pgd_t *pgd = pgd_offset_k((unsigned long)vaddr);
-	pud_t *pud = pud_offset(pgd, (unsigned long)vaddr);
-	pmd_t *pmd = pmd_offset(pud, (unsigned long)vaddr);
-	pte_t *ptep = pte_offset_kernel(pmd, (unsigned long)vaddr);
-	return pte_pfn(*ptep) << PAGE_SHIFT;
+	return __follow_page(vaddr);
 }
 
 #else				/* __OS_HAS_AGP */
include/asm-i386/delay.h +2 -2
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/dontdiff --minimal linux-2.6.20-rc3-git4/include/asm-i386/delay.h working-2.6.20-rc3-git4/include/asm-i386/delay.h
@@ -17,9 +17,9 @@ extern void __const_udelay(unsigned long
 extern void __delay(unsigned long loops);
 
 #if defined(CONFIG_PARAVIRT) && !defined(USE_REAL_TIME_DELAY)
-#define udelay(n) paravirt_ops.const_udelay((n) * 0x10c7ul)
+#define udelay(n) paravirt_const_udelay((n) * 0x10c7ul)
 
-#define ndelay(n) paravirt_ops.const_udelay((n) * 5ul)
+#define ndelay(n) paravirt_const_udelay((n) * 5ul)
 
 #else /* !PARAVIRT || USE_REAL_TIME_DELAY */
include/asm-i386/paravirt.h +44 -51

199 lines not on this page · raw message

include/linux/irqflags.h +5 -5
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/dontdiff --minimal linux-2.6.20-rc3-git4/include/linux/irqflags.h working-2.6.20-rc3-git4/include/linux/irqflags.h
@@ -74,11 +74,11 @@
 #endif /* CONFIG_TRACE_IRQFLAGS_SUPPORT */
 
 #ifdef CONFIG_TRACE_IRQFLAGS_SUPPORT
-#define safe_halt()						\
-	do {							\
-		trace_hardirqs_on();				\
-		raw_safe_halt();				\
-	} while (0)
+static inline void safe_halt(void)
+{
+	trace_hardirqs_on();
+	raw_safe_halt();
+}
 
 #define local_save_flags(flags)		raw_local_save_flags(flags)
include/linux/mm.h +2 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/dontdiff --minimal linux-2.6.20-rc3-git4/include/linux/mm.h working-2.6.20-rc3-git4/include/linux/mm.h
@@ -1127,6 +1127,8 @@ struct page *follow_page(struct vm_area_
 #define FOLL_GET	0x04	/* do get_page on page */
 #define FOLL_ANON	0x08	/* give ZERO_PAGE if no pgtable */
 
+unsigned long __follow_page(void *vaddr);
+
 #ifdef CONFIG_PROC_FS
 void vm_stat_account(struct mm_struct *, unsigned long, struct file *, long);
 #else
mm/memory.c +11 -0
diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/dontdiff --minimal linux-2.6.20-rc3-git4/mm/memory.c working-2.6.20-rc3-git4/mm/memory.c
@@ -976,6 +976,17 @@ no_page_table:
 	return page;
 }
 
+/* You don't want to use this function.  It's for drm_memory.c. */
+unsigned long __follow_page(void *vaddr)
+{
+	pgd_t *pgd = pgd_offset_k((unsigned long)vaddr);
+	pud_t *pud = pud_offset(pgd, (unsigned long)vaddr);
+	pmd_t *pmd = pmd_offset(pud, (unsigned long)vaddr);
+	pte_t *ptep = pte_offset_kernel(pmd, (unsigned long)vaddr);
+	return pte_pfn(*ptep) << PAGE_SHIFT;
+}
+EXPORT_SYMBOL_GPL(__follow_page);
+
 int get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
 		unsigned long start, int len, int write, int force,
 		struct page **pages, struct vm_area_struct **vmas)
author
#6 lore raw
Quoted 3 lines · Rusty Russell

I moved drm_follow_page into the core, to avoid having to wrap the various pte ops. Unlining kernel_fpu_end and using that in the RAID6 code would remove the need to export clts/read_cr0/write_cr0 too.

yeah, let's do that too.

Ingo

#7 lore raw
In reply to Rusty Russell
Quoted 2 lines · Rusty Russell

+int paravirt_write_msr(unsigned int msr, u64 val);

If binary modules using debug registers makes us nervous, the reprogramming MSRs is also similarly bad.

include/asm-i386/paravirt.h static inline void pte_update_defer(stru · line 339

Quoted 3 lines
include/asm-i386/paravirt.h +2 -0
+void raw_safe_halt(void);
+void halt(void);

These shouldn't be done by modules, ever. Only the scheduler should decide to halt.

#8 lore raw
Quoted 6 lines · Zachary Amsden
Quoted 2 lines · Rusty Russell

+int paravirt_write_msr(unsigned int msr, u64 val);

If binary modules using debug registers makes us nervous, the reprogramming MSRs is also similarly bad.

Yes, but this is simply from experience. Several modules wrote msrs (you can take out the EXPORT_SYMBOL and find them quite quickly).

Quoted 5 lines

include/asm-i386/paravirt.h static inline void pte_update_defer(stru · line 339

Quoted 2 lines
include/asm-i386/paravirt.h +2 -0
+void raw_safe_halt(void);
+void halt(void);

These shouldn't be done by modules, ever. Only the scheduler should decide to halt.

Several modules implement alternate halt loops. I guess being in a module for specific CPUs makes sense...

Cheers!
Rusty.

#9 lore raw
Quoted 4 lines · Rusty Russell
Quoted 2 lines · Zachary Amsden

Rusty Russell wrote:

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

+int paravirt_write_msr(unsigned int msr, u64 val);

If binary modules using debug registers makes us nervous, the reprogramming MSRs is also similarly bad.

Yes, but this is simply from experience. Several modules wrote msrs (you can take out the EXPORT_SYMBOL and find them quite quickly).

Several in tree, GPL'd modules did so. I'm not aware of out of tree modules that do that, and if they do, they are misbehaving. Reprogramming MTRR memory regions under the kernel's nose is not a proper way to behave, and this is the most benign use I can think of for write access to MSRs. If this really breaks any code out there, then there should be a proper, controlled API to do this so the kernel can manage it.

Quoted 9 lines
Quoted 5 lines

include/asm-i386/paravirt.h static inline void pte_update_defer(stru · line 339

Quoted 3 lines
include/asm-i386/paravirt.h +2 -0
+void raw_safe_halt(void);
+void halt(void);

These shouldn't be done by modules, ever. Only the scheduler should decide to halt.

Several modules implement alternate halt loops. I guess being in a module for specific CPUs makes sense...

Yes, but halting is a behavior that can easily introduce critical, grind to a halt problems because of race conditions. I have no problems having modules implement alternative halt loops. I think there is a serious debuggability issue with binary modules invoking halt directly.

Zach

author
#10 lore raw
In reply to Rusty Russell

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 676

Quoted 3 lines · Rusty Russell
arch/i386/kernel/paravirt.c +3 -0
+EXPORT_SYMBOL(clts);
+EXPORT_SYMBOL(read_cr0);
+EXPORT_SYMBOL(write_cr0);

mark these a _GPL export. Perhaps even mark the symbol deprecated, to be unexported once we fix raid6.

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 707

Quoted 5 lines
arch/i386/kernel/paravirt.c +5 -0
+EXPORT_SYMBOL(wbinvd);
+EXPORT_SYMBOL(raw_safe_halt);
+EXPORT_SYMBOL(halt);
+EXPORT_SYMBOL_GPL(apic_write);
+EXPORT_SYMBOL_GPL(apic_read);

these should be _GPL too. If any module uses it and breaks a user's box we need that big licensing hint to be able to debug them ...

Ingo

#11 lore raw
Quoted 10 lines · Ingo Molnar

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 676

Quoted 4 lines · Rusty Russell
arch/i386/kernel/paravirt.c +3 -0
+EXPORT_SYMBOL(clts);
+EXPORT_SYMBOL(read_cr0);
+EXPORT_SYMBOL(write_cr0);

mark these a _GPL export. Perhaps even mark the symbol deprecated, to be unexported once we fix raid6.

read / write cr0 must not be GPL, since kernel_fpu_end uses them and is inline. clts I don't think matters.

Quoted 9 lines

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 707

Quoted 6 lines
arch/i386/kernel/paravirt.c +5 -0
+EXPORT_SYMBOL(wbinvd);
+EXPORT_SYMBOL(raw_safe_halt);
+EXPORT_SYMBOL(halt);
+EXPORT_SYMBOL_GPL(apic_write);
+EXPORT_SYMBOL_GPL(apic_read);

these should be _GPL too. If any module uses it and breaks a user's box we need that big licensing hint to be able to debug them ...

Perhaps also, MSRs are too dangerous for binary modules to be messing with.

Agree on halt - but wbinvd can theoretically be used for device mapped memory consistency.

Zach

#12 lore raw
Quoted 13 lines · Zachary Amsden
Quoted 11 lines · Ingo Molnar

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 676

Quoted 4 lines · Rusty Russell
arch/i386/kernel/paravirt.c +3 -0
+EXPORT_SYMBOL(clts);
+EXPORT_SYMBOL(read_cr0);
+EXPORT_SYMBOL(write_cr0);

mark these a _GPL export. Perhaps even mark the symbol deprecated, to be unexported once we fix raid6.

Quoted 1 line

read / write cr0 must not be GPL, since kernel_fpu_end uses them

but kernel_fpu_begin() is _GPL already so this is just symmetry...

Signature · 2 lines
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org
#13 lore raw
In reply to Ingo Molnar
Quoted 8 lines · Ingo Molnar

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 676

Quoted 3 lines · Rusty Russell
arch/i386/kernel/paravirt.c +3 -0
+EXPORT_SYMBOL(clts);
+EXPORT_SYMBOL(read_cr0);
+EXPORT_SYMBOL(write_cr0);

mark these a _GPL export. Perhaps even mark the symbol deprecated, to be unexported once we fix raid6.

OK...

Quoted 8 lines

arch/i386/kernel/paravirt.c static int __init print_banner(void) · line 707

Quoted 5 lines
arch/i386/kernel/paravirt.c +5 -0
+EXPORT_SYMBOL(wbinvd);
+EXPORT_SYMBOL(raw_safe_halt);
+EXPORT_SYMBOL(halt);
+EXPORT_SYMBOL_GPL(apic_write);
+EXPORT_SYMBOL_GPL(apic_read);

these should be _GPL too. If any module uses it and breaks a user's box we need that big licensing hint to be able to debug them ...

OK. I GPLed the ones which I thought were really obscure, but I was trying to follow existing policy of not _GPL'ing existing symbols.

Cheers,
Rusty.

PS. drm_memory.h has a "drm_follow_page": this forces us to uninline various page tables ops. Can this use follow_page() somehow, or do we need an "__follow_page" export for this case?

#14 lore raw
Quoted 3 lines · Rusty Russell

PS. drm_memory.h has a "drm_follow_page": this forces us to uninline various page tables ops. Can this use follow_page() somehow, or do we need an "__follow_page" export for this case?

Not if avoidable. And it seems avoidable as drm really should be using vmalloc_to_page. Untested patch below:

drivers/char/drm/drm_memory.c +22 -15
Index: linux-2.6/drivers/char/drm/drm_memory.c
===================================================================
@@ -211,6 +211,23 @@
 }
 #endif  /*  0  */
 
+static int is_agp_mapping(void *pt, unsigned long size, drm_device_t *dev)
+{
+	unsigned long addr = (unsigned long)pt;
+
+	if (addr >= VMALLOC_START && addr < VMALLOC_END) {
+		unsigned long phys;
+		drm_map_t *map;
+
+		phys = page_to_phys(vmalloc_to_page(pt)) + offset_in_page(pt);
+		map = drm_lookup_map(phys, size, dev);
+		if (map && map->type == _DRM_AGP)
+			return 1;
+	}
+
+	return 0;
+}
+
 void drm_ioremapfree(void *pt, unsigned long size,
 				   drm_device_t * dev)
 {

@@ -219,21 +236,11 @@
 	 * routines for handling mappings in the AGP space.  Hopefully this can be done in
 	 * a future revision of the interface...
 	 */
-	if (drm_core_has_AGP(dev) && dev->agp && dev->agp->cant_use_aperture
-	    && ((unsigned long)pt >= VMALLOC_START
-		&& (unsigned long)pt < VMALLOC_END)) {
-		unsigned long offset;
-		drm_map_t *map;
-
-		offset = drm_follow_page(pt) | ((unsigned long)pt & ~PAGE_MASK);
-		map = drm_lookup_map(offset, size, dev);
-		if (map && map->type == _DRM_AGP) {
-			vunmap(pt);
-			return;
-		}
-	}
-
-	iounmap(pt);
+	if (drm_core_has_AGP(dev) && dev->agp && dev->agp->cant_use_aperture &&
+	    is_agp_mapping(pt, size, dev))
+		vunmap(pt);
+	else
+		iounmap(pt);
 }
 EXPORT_SYMBOL(drm_ioremapfree);
drivers/char/drm/drm_memory.h +0 -17
Index: linux-2.6/drivers/char/drm/drm_memory.h
===================================================================
@@ -55,23 +55,6 @@
 #  define PAGE_AGP	PAGE_KERNEL
 # endif
 #endif
-
-static inline unsigned long drm_follow_page(void *vaddr)
-{
-	pgd_t *pgd = pgd_offset_k((unsigned long)vaddr);
-	pud_t *pud = pud_offset(pgd, (unsigned long)vaddr);
-	pmd_t *pmd = pmd_offset(pud, (unsigned long)vaddr);
-	pte_t *ptep = pte_offset_kernel(pmd, (unsigned long)vaddr);
-	return pte_pfn(*ptep) << PAGE_SHIFT;
-}
-
-#else				/* __OS_HAS_AGP */
-
-static inline unsigned long drm_follow_page(void *vaddr)
-{
-	return 0;
-}
-
 #endif
 
 void *drm_ioremap(unsigned long offset, unsigned long size,
#15 lore raw
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Rusty Russell

PS. drm_memory.h has a "drm_follow_page": this forces us to uninline various page tables ops. Can this use follow_page() somehow, or do we need an "__follow_page" export for this case?

Not if avoidable. And it seems avoidable as drm really should be using vmalloc_to_page. Untested patch below:

Even better we can actualy avid most of the page table walks completely. First there is a number of places that can never have the vmalloc case an may use ioremap/iounmap directly. Secondly drm_core_ioremap/ drm_core_ioremapfree already have the right drm_map to check wich kind of mapping we have - we just need to use it instead of discarding that information! The only leaves direct drm_ioremapfree in i810_dma.c and i830_dma.c which I don't quite manage to follow. Maybe Dave has an idea how to get rid of those aswell.

drivers/char/drm/drm_bufs.c +4 -4
Index: linux-2.6/drivers/char/drm/drm_bufs.c
===================================================================
@@ -178,7 +178,7 @@
 			}
 		}
 		if (map->type == _DRM_REGISTERS)
-			map->handle = drm_ioremap(map->offset, map->size, dev);
+			map->handle = ioremap(map->offset, map->size);
 		break;
 
 	case _DRM_SHM:

@@ -238,7 +238,7 @@
 	list = drm_alloc(sizeof(*list), DRM_MEM_MAPS);
 	if (!list) {
 		if (map->type == _DRM_REGISTERS)
-			drm_ioremapfree(map->handle, map->size, dev);
+			iounmap(map->handle);
 		drm_free(map, sizeof(*map), DRM_MEM_MAPS);
 		return -EINVAL;
 	}

@@ -255,7 +255,7 @@
 	ret = drm_map_handle(dev, &list->hash, user_token, 0);
 	if (ret) {
 		if (map->type == _DRM_REGISTERS)
-			drm_ioremapfree(map->handle, map->size, dev);
+			iounmap(map->handle);
 		drm_free(map, sizeof(*map), DRM_MEM_MAPS);
 		drm_free(list, sizeof(*list), DRM_MEM_MAPS);
 		mutex_unlock(&dev->struct_mutex);

@@ -362,7 +362,7 @@
 
 	switch (map->type) {
 	case _DRM_REGISTERS:
-		drm_ioremapfree(map->handle, map->size, dev);
+		iounmap(map->handle);
 		/* FALLTHROUGH */
 	case _DRM_FRAME_BUFFER:
 		if (drm_core_has_MTRR(dev) && map->mtrr >= 0) {
drivers/char/drm/drm_vm.c +6 -1
Index: linux-2.6/drivers/char/drm/drm_vm.c
===================================================================
@@ -227,7 +227,12 @@
 							   map->size);
 					DRM_DEBUG("mtrr_del = %d\n", retcode);
 				}
-				drm_ioremapfree(map->handle, map->size, dev);
+				/*
+				 * XXX(hch) autmatic mapping/unmapping only happens for
+				 * _DRM_REGISTERS in all other places.  Should we have
+				 * this check here aswell?
+				 */
+				iounmap(map->handle);
 				break;
 			case _DRM_SHM:
 				vfree(map->handle);
drivers/char/drm/drmP.h +2 -21
Index: linux-2.6/drivers/char/drm/drmP.h
===================================================================
@@ -1059,27 +1059,8 @@
 extern int drm_mm_init(drm_mm_t *mm, unsigned long start, unsigned long size);
 extern void drm_mm_takedown(drm_mm_t *mm);
 
-/* Inline replacements for DRM_IOREMAP macros */
-static __inline__ void drm_core_ioremap(struct drm_map *map,
-					struct drm_device *dev)
-{
-	map->handle = drm_ioremap(map->offset, map->size, dev);
-}
-
-#if 0
-static __inline__ void drm_core_ioremap_nocache(struct drm_map *map,
-						struct drm_device *dev)
-{
-	map->handle = drm_ioremap_nocache(map->offset, map->size, dev);
-}
-#endif  /*  0  */
-
-static __inline__ void drm_core_ioremapfree(struct drm_map *map,
-					    struct drm_device *dev)
-{
-	if (map->handle && map->size)
-		drm_ioremapfree(map->handle, map->size, dev);
-}
+extern void drm_core_ioremap(struct drm_map *map, struct drm_device *dev);
+extern void drm_core_ioremapfree(struct drm_map *map, struct drm_device *dev);
 
 static __inline__ struct drm_map *drm_core_findmap(struct drm_device *dev,
 						   unsigned int token)
drivers/char/drm/drm_memory.c +23 -14
Index: linux-2.6/drivers/char/drm/drm_memory.c
===================================================================
@@ -197,20 +197,6 @@
 }
 EXPORT_SYMBOL(drm_ioremap);
 
-#if 0
-void *drm_ioremap_nocache(unsigned long offset,
-					unsigned long size, drm_device_t * dev)
-{
-	if (drm_core_has_AGP(dev) && dev->agp && dev->agp->cant_use_aperture) {
-		drm_map_t *map = drm_lookup_map(offset, size, dev);
-
-		if (map && map->type == _DRM_AGP)
-			return agp_remap(offset, size, dev);
-	}
-	return ioremap_nocache(offset, size);
-}
-#endif  /*  0  */
-
 void drm_ioremapfree(void *pt, unsigned long size,
 				   drm_device_t * dev)
 {

@@ -238,3 +224,26 @@
 EXPORT_SYMBOL(drm_ioremapfree);
 
 #endif				/* debug_memory */
+
+void drm_core_ioremap(struct drm_map *map, struct drm_device *dev)
+{
+	if (drm_core_has_AGP(dev) &&
+	    dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP)
+		map->handle = agp_remap(map->offset, map->size, dev);
+	else
+		map->handle = ioremap(map->offset, map->size);
+}
+EXPORT_SYMBOL_GPL(drm_core_ioremap);
+
+void drm_core_ioremapfree(struct drm_map *map, struct drm_device *dev)
+{
+	if (!map->handle || !map->size)
+		return;
+
+	if (drm_core_has_AGP(dev) &&
+	    dev->agp && dev->agp->cant_use_aperture && map->type == _DRM_AGP)
+		vunmap(map->handle);
+	else
+		iounmap(map->handle);
+}
+EXPORT_SYMBOL_GPL(drm_core_ioremapfree);
author
#16 lore raw
Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Rusty Russell

PS. drm_memory.h has a "drm_follow_page": this forces us to uninline various page tables ops. Can this use follow_page() somehow, or do we need an "__follow_page" export for this case?

Not if avoidable. And it seems avoidable as drm really should be using vmalloc_to_page. Untested patch below:

Even better we can actualy avid most of the page table walks completely.

agreed. I think there's an important side-observation here as well:
having inlined functions uninlined and exported puts them under a lot more scrutiny. Hence individual exports instead of the global paravirt_ops export is a big plus.

Ingo

Applied
#17 lore raw
In reply to Christoph Hellwig
Quoted 10 lines

Even better we can actualy avid most of the page table walks completely. First there is a number of places that can never have the vmalloc case an may use ioremap/iounmap directly. Secondly drm_core_ioremap/ drm_core_ioremapfree already have the right drm_map to check wich kind of mapping we have - we just need to use it instead of discarding that information! The only leaves direct drm_ioremapfree in i810_dma.c and i830_dma.c which I don't quite manage to follow. Maybe Dave has an idea how to get rid of those aswell.

I've applied two patches to the DRM git
http://gitweb.freedesktop.org/?p=mesa/drm.git;a=summary

They need a fair bit of testing, I've tested them no i810, but I need to test them on a few other boards before I'd consider putting them in -mm..

Dave.

Signature · 3 lines
David Airlie, Software Engineer
http://www.skynet.ie/~airlied / airlied at skynet.ie
Linux kernel - DRI, VAX / pam_smb / ILUG
#18 lore raw
In reply to Christoph Hellwig
Quoted 7 lines · Christoph Hellwig
Quoted 3 lines · Rusty Russell

PS. drm_memory.h has a "drm_follow_page": this forces us to uninline various page tables ops. Can this use follow_page() somehow, or do we need an "__follow_page" export for this case?

Not if avoidable. And it seems avoidable as drm really should be using vmalloc_to_page. Untested patch below:

Thanks Christoph, that patch looks great to me! I didn't know about vmalloc_to_page...

Want to produce a signed-off version?

Thanks,
Rusty.

#19 lore raw
In reply to Ingo Molnar
Quoted 9 lines · Ingo Molnar

Subject: [patch] paravirt: isolate module ops
From: Ingo Molnar <[email protected]>

only export those operations to modules that have been available to them historically: irq disable/enable, io-delay, udelay, etc.

this isolates that functionality from other paravirtualization functionality that modules have no business messing with.

boot and build tested with CONFIG_PARAVIRT=y.

I would suggest a slightly different carving. For one, no TLB flushes. If you can't modify PTEs, why do you need to have TLB flushes? And I would allow CR0 read / write for code which saves and restores FPU state - possibly even debug register access, although any code which touches DRs could be doing something sneaky. I'm on the fence about that one.

Here is a partially tested patch against the -mm tree. Let me know what you think of this slightly different approach.

arch/i386/kernel/paravirt.c +40 -1
diff -r 3242f865ce8e arch/i386/kernel/paravirt.c
@@ -492,6 +492,7 @@ struct paravirt_ops paravirt_ops = {
 
  	.patch = native_patch,
 	.banner = default_banner,
+
 	.arch_setup = native_nop,
 	.memory_setup = machine_specific_memory_setup,
 	.get_wallclock = native_get_wallclock,

@@ -577,4 +578,42 @@ struct paravirt_ops paravirt_ops = {
 
 	.startup_ipi_hook = (void *)native_nop,
 };
-EXPORT_SYMBOL(paravirt_ops);
+
+/*
+ * These are exported to modules:
+ */
+struct paravirt_mod_ops paravirt_mod_ops = {
+	.name = "bare hardware",
+	.paravirt_enabled = 0,
+	.kernel_rpl = 0,
+
+ 	.patch = native_patch,
+	.banner = default_banner,
+
+	.save_fl = native_save_fl,
+	.restore_fl = native_restore_fl,
+	.irq_disable = native_irq_disable,
+	.irq_enable = native_irq_enable,
+
+	.cpuid = native_cpuid,
+
+	.read_msr = native_read_msr,
+	.write_msr = native_write_msr,
+
+	.read_tsc = native_read_tsc,
+	.read_pmc = native_read_pmc,
+
+	.io_delay = native_io_delay,
+	.const_udelay = __const_udelay,
+
+#ifdef CONFIG_X86_LOCAL_APIC
+	.apic_write = native_apic_write,
+	.apic_write_atomic = native_apic_write_atomic,
+	.apic_read = native_apic_read,
+#endif
+
+	.flush_tlb_user = native_flush_tlb,
+	.flush_tlb_kernel = native_flush_tlb_global,
+	.flush_tlb_single = native_flush_tlb_single,
+};
+EXPORT_SYMBOL(paravirt_mod_ops);
include/asm-i386/delay.h +2 -2
diff -r 3242f865ce8e include/asm-i386/delay.h
@@ -17,9 +17,9 @@ extern void __delay(unsigned long loops)
 extern void __delay(unsigned long loops);
 
 #if defined(CONFIG_PARAVIRT) && !defined(USE_REAL_TIME_DELAY)
-#define udelay(n) paravirt_ops.const_udelay((n) * 0x10c7ul)
+#define udelay(n) paravirt_mod_ops.const_udelay((n) * 0x10c7ul)
 
-#define ndelay(n) paravirt_ops.const_udelay((n) * 5ul)
+#define ndelay(n) paravirt_mod_ops.const_udelay((n) * 5ul)
 
 #else /* !PARAVIRT || USE_REAL_TIME_DELAY */
include/asm-i386/paravirt.h +84 -74

335 lines not on this page · raw message

arch/i386/kernel/asm-offsets.c +4 -4
diff -r 3242f865ce8e arch/i386/kernel/asm-offsets.c
@@ -104,11 +104,11 @@ void foo(void)
 
 #ifdef CONFIG_PARAVIRT
 	BLANK();
-	OFFSET(PARAVIRT_enabled, paravirt_ops, paravirt_enabled);
-	OFFSET(PARAVIRT_irq_disable, paravirt_ops, irq_disable);
-	OFFSET(PARAVIRT_irq_enable, paravirt_ops, irq_enable);
+	OFFSET(PARAVIRT_enabled, paravirt_mod_ops, paravirt_enabled);
+	OFFSET(PARAVIRT_irq_disable, paravirt_mod_ops, irq_disable);
+	OFFSET(PARAVIRT_irq_enable, paravirt_mod_ops, irq_enable);
 	OFFSET(PARAVIRT_irq_enable_sysexit, paravirt_ops, irq_enable_sysexit);
 	OFFSET(PARAVIRT_iret, paravirt_ops, iret);
-	OFFSET(PARAVIRT_read_cr0, paravirt_ops, read_cr0);
+	OFFSET(PARAVIRT_read_cr0, paravirt_mod_ops, read_cr0);
 #endif
 }
#20 lore raw
Quoted 4 lines

I would suggest a slightly different carving. For one, no TLB flushes. If you can't modify PTEs, why do you need to have TLB flushes? And I would allow CR0 read / write for code which saves and restores FPU state

no that is abstracted away by kernel_fpu_begin/end. Modules have no business doing that themselves

Quoted 2 lines

- possibly even debug register access, although any code which touches DRs could be doing something sneaky. I'm on the fence about that one.

lets not allow it at all

Signature · 2 lines
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org
#21 lore raw
Quoted 6 lines · Arjan van de Ven
Quoted 2 lines

I would suggest a slightly different carving. For one, no TLB flushes. If you can't modify PTEs, why do you need to have TLB flushes? And I would allow CR0 read / write for code which saves and restores FPU state

no that is abstracted away by kernel_fpu_begin/end. Modules have no business doing that themselves

As long as they don't rely on inlines for that... checking and kernel_fpu_end is inline and uses stts(), which requires CR0 read / write. One can easily imagine binary modules which do use the fpu, and these were not broken before, so breaking them now seems the wrong thing to do.

I agree on debug registers - anything touching them is way too shady. And there is no reason modules should be doing raw page table operations, they should use proper mm functions and leave the page details to the mm layer, which doesn't do these things inline.

Basically, it is just the things that do get inlined that I think we should worry about. If you all feel strongly that this should be fixed in 2.6.20, perhaps the best thing to do is in fact EXPORT_SYMBOL_GPL(paravirt_ops), and we can queue up a patch in -mm which will export those paravirt_ops required inline by modules for 2.6.21. Otherwise, I think there will be too many rejects against the paravirt code in Andrew's tree.

Zach

#22 lore raw
In reply to Arjan van de Ven
Quoted 7 lines · Arjan van de Ven
Quoted 4 lines

I would suggest a slightly different carving. For one, no TLB flushes. If you can't modify PTEs, why do you need to have TLB flushes? And I would allow CR0 read / write for code which saves and restores FPU state

no that is abstracted away by kernel_fpu_begin/end. Modules have no business doing that themselves

Sure, but it'll take some time to fix the raid modules (which are the ones which abuse this).

I'm testing a patch now, I'll send the clts removal patch on top of that once it's done.

Rusty.