Skip to contentSkip to conversation

Bugfix for VMI paravirt ops

PATCH In review Zachary Amsden 4 participants 18 messages Apr 5, 2007 – Apr 6, 2007 Generic include/asm header files

lore
Share & tools
lore

Zachary Amsden

author
#0 lore raw

PATCH 1/1 Bugfix for VMI paravirt ops

I noticed this never got applied. There was some feedback which I did not include in this patch because I think it is inappropriate to touch code outside vmi.c at this point for 2.6.21. Please apply; this patch is needed as a bugfix in 2.6.21. An updated version for 2.6.22 will come later which has a nicer interface.

Zach

Critical bugfix; when using software RAID, potentially USB or AIO in highmem configurations, drivers are allowed to use kmap_atomic from interrupt context. This is incompatible with the current implementation of lazy MMU mode, and means the kmap will silently fail, causing either memory corruption or kernel panics.

The fix is to disable interrupts on the CPU when entering a lazy MMU state; this is totally safe, as preemption is already disabled, and lazy update state can neither be nested nor overlapping. Thus per-cpu variables to track the state and flags can be used to disable interrupts during this critical region.

Signed-off-by Zachary Amsden <[email protected]>
arch/i386/kernel/vmi.c +27 -1
Index: ubuntu-2.6.20/arch/i386/kernel/vmi.c
===================================================================
@@ -69,6 +69,7 @@
 	void (fastcall *flush_tlb)(int);
 	void (fastcall *set_initial_ap_state)(int, int);
 	void (fastcall *halt)(void);
+ 	void (fastcall *set_lazy_mode)(int mode);
 } vmi_ops;
  
 /* XXX move this to alternative.h */

@@ -577,6 +578,31 @@
 }
 #endif
 
+static void vmi_set_lazy_mode(int new_mode)
+{
+	static DEFINE_PER_CPU(int, mode);
+	static DEFINE_PER_CPU(unsigned long, flags);
+	int cpu = smp_processor_id();
+
+	if (!vmi_ops.set_lazy_mode)
+		return;
+
+	/*
+	 * Modes do not nest or overlap, so we can simply disable
+	 * irqs when entering a mode and re-enable when leaving.
+	 */
+	BUG_ON(per_cpu(mode, cpu) && new_mode);
+	BUG_ON(!new_mode && !per_cpu(mode, cpu));
+	
+	if (new_mode)
+		local_irq_save(per_cpu(flags, cpu));
+	else
+		local_irq_restore(per_cpu(flags, cpu));
+
+	vmi_ops.set_lazy_mode(new_mode);
+	per_cpu(mode, cpu) = new_mode;
+}
+
 static inline int __init check_vmi_rom(struct vrom_header *rom)
 {
 	struct pci_header *pci;

@@ -806,7 +832,7 @@
 	para_wrap(load_esp0, vmi_load_esp0, set_kernel_stack, UpdateKernelStack);
 	para_fill(set_iopl_mask, SetIOPLMask);
 	para_fill(io_delay, IODelay);
-	para_fill(set_lazy_mode, SetLazyMode);
+	para_wrap(set_lazy_mode, vmi_set_lazy_mode, set_lazy_mode, SetLazyMode);
 
 	/* user and kernel flush are just handled with different flags to FlushTLB */
 	para_wrap(flush_tlb_user, vmi_flush_tlb_user, flush_tlb, FlushTLB);
author
#1 lore raw
Quoted 2 lines · Zachary Amsden

I noticed this never got applied. There was some feedback which I did not include in this patch because I think it is inappropriate to touch code outside vmi.c at this point for 2.6.21. Please apply; this patch is needed as a bugfix in 2.6.21. An updated version for 2.6.22 will come later which has a nicer interface.

Erm, stale patch, sorry. This one instead.

Critical bugfix; when using software RAID, potentially USB or AIO in highmem configurations, drivers are allowed to use kmap_atomic from interrupt context. This is incompatible with the current implementation of lazy MMU mode, and means the kmap will silently fail, causing either memory corruption or kernel panics.

The fix is to disable interrupts on the CPU when entering a lazy MMU state; this is totally safe, as preemption is already disabled, and lazy update state can neither be nested nor overlapping. Thus per-cpu variables to track the state and flags can be used to disable interrupts during this critical region.

Signed-off-by Zachary Amsden <[email protected]>
arch/i386/kernel/vmi.c +27 -1
diff -r eee69881b2f9 arch/i386/kernel/vmi.c
@@ -69,6 +69,7 @@ static struct {
 	void (*flush_tlb)(int);
 	void (*set_initial_ap_state)(int, int);
 	void (*halt)(void);
+  	void (*set_lazy_mode)(int mode);
 } vmi_ops;
 
 /*

@@ -545,6 +546,31 @@ vmi_startup_ipi_hook(int phys_apicid, un
 }
 #endif
 
+static void vmi_set_lazy_mode(int new_mode)
+{
+	static DEFINE_PER_CPU(int, mode);
+	static DEFINE_PER_CPU(unsigned long, flags);
+	int cpu = smp_processor_id();
+
+	if (!vmi_ops.set_lazy_mode)
+		return;
+
+	/*
+	 * Modes do not nest or overlap, so we can simply disable
+	 * irqs when entering a mode and re-enable when leaving.
+	 */
+	BUG_ON(per_cpu(mode, cpu) && new_mode);
+	BUG_ON(!new_mode && !per_cpu(mode, cpu));
+	
+	if (new_mode)
+		local_irq_save(per_cpu(flags, cpu));
+	else
+		local_irq_restore(per_cpu(flags, cpu));
+
+	vmi_ops.set_lazy_mode(new_mode);
+	per_cpu(mode, cpu) = new_mode;
+}
+
 static inline int __init check_vmi_rom(struct vrom_header *rom)
 {
 	struct pci_header *pci;

@@ -769,7 +795,7 @@ static inline int __init activate_vmi(vo
 	para_wrap(load_esp0, vmi_load_esp0, set_kernel_stack, UpdateKernelStack);
 	para_fill(set_iopl_mask, SetIOPLMask);
 	para_fill(io_delay, IODelay);
-	para_fill(set_lazy_mode, SetLazyMode);
+	para_wrap(set_lazy_mode, vmi_set_lazy_mode, set_lazy_mode, SetLazyMode);
 
 	/* user and kernel flush are just handled with different flags to FlushTLB */
 	para_wrap(flush_tlb_user, vmi_flush_tlb_user, flush_tlb, FlushTLB);
#3 lore raw
In reply to Zachary Amsden
Quoted 3 lines · On Friday 06 April 2007 01:29:56 Zachary Amsden wrote:

I noticed this never got applied. There was some feedback which I did not include in this patch because I think it is inappropriate to touch code outside vmi.c at this point for 2.6.21.

I think it is. That is why i didn't apply it.

-Andi

#2 lore raw
In reply to Zachary Amsden
Quoted 6 lines · On Thu, 05 Apr 2007 16:34:43 -0700 Zachary Amsden <[email protected]> wrote:
Quoted 6 lines

I noticed this never got applied. There was some feedback which I did not include in this patch because I think it is inappropriate to touch code outside vmi.c at this point for 2.6.21. Please apply; this patch is needed as a bugfix in 2.6.21. An updated version for 2.6.22 will come later which has a nicer interface.

There was a big foodfight last time you sent this out and I'd assumed that there were still unresolved issues. Or at least a general auru of unhappiness.

I guess we merge it now, then (forget to) fix up those issues later on.

Quoted 1 line

Erm, stale patch, sorry. This one instead.

yeah, that's the patch which has been in -mm for a week.

author
#4 lore raw
In reply to Andi Kleen
Quoted 7 lines · Andi Kleen
Quoted 2 lines · On Friday 06 April 2007 01:29:56 Zachary Amsden wrote:

I noticed this never got applied. There was some feedback which I did not include in this patch because I think it is inappropriate to touch code outside vmi.c at this point for 2.6.21.

I think it is. That is why i didn't apply it.

Well at this point, the "proper" fix is dependent on Jeremy's kmap_atomic_pte changes, which are definitely too late to pull into 2.6.21. Can we just apply this patch please?

Thanks,

Zach

#5 lore raw
Quoted 3 lines · Zachary Amsden

Well at this point, the "proper" fix is dependent on Jeremy's kmap_atomic_pte changes, which are definitely too late to pull into 2.6.21. Can we just apply this patch please?

Hm, I think they're independent aren't they? Your fix is about making lazy_mmu disable interrupts; that's independent of how highpte pages get mapped.

J

author
#6 lore raw
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 4 lines · Zachary Amsden

Well at this point, the "proper" fix is dependent on Jeremy's kmap_atomic_pte changes, which are definitely too late to pull into 2.6.21. Can we just apply this patch please?

Hm, I think they're independent aren't they? Your fix is about making lazy_mmu disable interrupts; that's independent of how highpte pages get mapped.

No, they are totally dependent. The reason interrupts are disabled is to stop kmap_atomic in interrupt handlers. With the kmap_atomic_pte changes, the whole interrupt disable jibberish goes away.

Zach

#7 lore raw
Quoted 3 lines · Zachary Amsden

No, they are totally dependent. The reason interrupts are disabled is to stop kmap_atomic in interrupt handlers. With the kmap_atomic_pte changes, the whole interrupt disable jibberish goes away.

But kmap_atomic_pte is a special case of kmap_atomic for ptes. Interrupt routines can still use plain kmap_atomic for bouncebuffers and so on.

A more general patch would be to make kmap/unmap_atomic pv_ops, and then they can all be rolled together. I.e: check the type to see if special pte handling needs to happen, etc.

J

author
#8 lore raw
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 4 lines · Zachary Amsden

No, they are totally dependent. The reason interrupts are disabled is to stop kmap_atomic in interrupt handlers. With the kmap_atomic_pte changes, the whole interrupt disable jibberish goes away.

But kmap_atomic_pte is a special case of kmap_atomic for ptes. Interrupt routines can still use plain kmap_atomic for bouncebuffers and so on.

Ah, yes.

Quoted 4 lines

A more general patch would be to make kmap/unmap_atomic pv_ops, and then they can all be rolled together. I.e: check the type to see if special pte handling needs to happen, etc.

So the clean fix for this is still even further out. I don't think I want to hook kmap/unmap as paravirt-ops.

Zach

#9 lore raw
Quoted 2 lines · Zachary Amsden

So the clean fix for this is still even further out. I don't think I want to hook kmap/unmap as paravirt-ops.

Yes, it seems like overkill.

How about something like adding PARAVIRT_LAZY_FLUSH as an argument to set_lazy_mode? It would be valid to use at any time, and it would flush any pending work while still remaining in whatever lazy mode its currently in. That way kmap_atomic can flush anything pending without having to muck around with the current lazy state.

J

author
#10 lore raw
Quoted 6 lines · Jeremy Fitzhardinge
Quoted 3 lines · Zachary Amsden

So the clean fix for this is still even further out. I don't think I want to hook kmap/unmap as paravirt-ops.

Quoted 8 lines

Yes, it seems like overkill.

How about something like adding PARAVIRT_LAZY_FLUSH as an argument to set_lazy_mode? It would be valid to use at any time, and it would flush any pending work while still remaining in whatever lazy mode its currently in. That way kmap_atomic can flush anything pending without having to muck around with the current lazy state.

Yes, thought about several solutions, and this seems the best. But it requires a new paravirt-op.

Zach

#11 lore raw
Quoted 2 lines · Zachary Amsden

Yes, thought about several solutions, and this seems the best. But it requires a new paravirt-op.

Not with the power of multiplexing. Something like this, perhaps?

J

arch/i386/mm/highmem.c +2 -0
diff -r 5be4a5ff8e6b arch/i386/mm/highmem.c
@@ -42,6 +42,8 @@ void *kmap_atomic_prot(struct page *page
 
 	vaddr = __fix_to_virt(FIX_KMAP_BEGIN + idx);
 	set_pte(kmap_pte-idx, mk_pte(page, prot));
+
+	arch_flush_lazy_mmu_mode();
 
 	return (void*) vaddr;
 }
include/asm-generic/pgtable.h +2 -0
diff -r 5be4a5ff8e6b include/asm-generic/pgtable.h
@@ -180,6 +180,7 @@ static inline void ptep_set_wrprotect(st
 #ifndef __HAVE_ARCH_ENTER_LAZY_MMU_MODE
 #define arch_enter_lazy_mmu_mode()	do {} while (0)
 #define arch_leave_lazy_mmu_mode()	do {} while (0)
+#define arch_flush_lazy_mmu_mode()	do {} while (0)
 #endif
 
 /*

@@ -193,6 +194,7 @@ static inline void ptep_set_wrprotect(st
 #ifndef __HAVE_ARCH_ENTER_LAZY_CPU_MODE
 #define arch_enter_lazy_cpu_mode()	do {} while (0)
 #define arch_leave_lazy_cpu_mode()	do {} while (0)
+#define arch_flush_lazy_cpu_mode()	do {} while (0)
 #endif
 
 /*
include/asm-i386/paravirt.h +12 -3
diff -r 5be4a5ff8e6b include/asm-i386/paravirt.h
@@ -27,9 +27,10 @@ struct desc_struct;
 
 /* Lazy mode for batching updates / context switch */
 enum paravirt_lazy_mode {
-	PARAVIRT_LAZY_NONE = 0,
-	PARAVIRT_LAZY_MMU = 1,
-	PARAVIRT_LAZY_CPU = 2,
+	PARAVIRT_LAZY_NONE = 0,		/* exit lazy mode */
+	PARAVIRT_LAZY_MMU = 1,		/* lazy mmu updates */
+	PARAVIRT_LAZY_CPU = 2,		/* lazy cpu state updates */
+	PARAVIRT_LAZY_FLUSH = 3,	/* flush pending changes, if any */
 };
 
 struct paravirt_ops

@@ -1044,6 +1045,10 @@ static inline void arch_leave_lazy_cpu_m
 {
 	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_NONE);
 }
+static inline void arch_flush_lazy_cpu_mode(void)
+{
+	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_FLUSH);
+}
 
 #define  __HAVE_ARCH_ENTER_LAZY_MMU_MODE
 static inline void arch_enter_lazy_mmu_mode(void)

@@ -1053,6 +1058,10 @@ static inline void arch_leave_lazy_mmu_m
 static inline void arch_leave_lazy_mmu_mode(void)
 {
 	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_NONE);
+}
+static inline void arch_flush_lazy_mmu_mode(void)
+{
+	PVOP_VCALL1(set_lazy_mode, PARAVIRT_LAZY_FLUSH);
 }
 
 void _paravirt_nop(void);
author
#12 lore raw
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 3 lines · Zachary Amsden

Yes, thought about several solutions, and this seems the best. But it requires a new paravirt-op.

Not with the power of multiplexing. Something like this, perhaps?

Throw it in the queue; I'll slide in after it.

Zach

author
#17 lore raw
In reply to Jeremy Fitzhardinge
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 3 lines · Zachary Amsden

Yes, thought about several solutions, and this seems the best. But it requires a new paravirt-op.

Not with the power of multiplexing. Something like this, perhaps?

Ok, I tried that and I got a nice clean fix. For 2.6.22. Backporting this to 2.6.21 creates havoc, as a number of cleanup patches as well as changes to highmem code get in the way.

Andi, do you really want to deal with the conflicts this will create for the paravirt queue for 2.6.22, or would you rather apply the dumb yet simple and non-confrontational workaround I have been trying to get applied to 2.6.21?

Zach

author
#14 lore raw
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 2 lines · Zachary Amsden

Throw it in the queue; I'll slide in after it.

I've pushed it up. I added a few missing cases to the patch (kmap_atomic_pte, kunmap_atomic).

Do you mean kmap_atomic_pfn? kunmap_atomic can stay lazy (at least for VMI), actually, but it doesn't help since it happens outside the spin lock.

#15 lore raw
Quoted 1 line · Zachary Amsden

Do you mean kmap_atomic_pfn?

Yes.

Quoted 2 lines

kunmap_atomic can stay lazy (at least for VMI), actually, but it
doesn't help since it happens outside the spin lock.

May as well be consistent. Or do you mean you can't flush outside the spinlock, even if there's nothing pending?

J

author
#16 lore raw
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 2 lines · Zachary Amsden

Do you mean kmap_atomic_pfn?

Yes.

Quoted 7 lines
Quoted 3 lines

kunmap_atomic can stay lazy (at least for VMI), actually, but it
doesn't help since it happens outside the spin lock.

May as well be consistent. Or do you mean you can't flush outside the spinlock, even if there's nothing pending?

Consistency is good. Flush is always fine, just an extra function call.

Zach