Re: [PATCH] VMI paravirt-ops bugfix for 2.6.21

[Jeremy Fitzhardinge <jeremy@goop.org>]

Zachary Amsden wrote:
> 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.  This bug is only visible with >970
> megs of RAM and extreme memory pressure, but nontheless extremely serious.
>   

Why's that?  Don't some things get preferentially allocated out of highmem?

> 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 <zach@vmware.com>
>
> diff -r be8c61492e28 arch/i386/kernel/vmi.c
> --- a/arch/i386/kernel/vmi.c	Fri Mar 30 14:13:45 2007 -0700
> +++ b/arch/i386/kernel/vmi.c	Fri Mar 30 14:18:16 2007 -0700
> @@ -69,6 +69,7 @@ struct {
>  	void (*flush_tlb)(int);
>  	void (*set_initial_ap_state)(int, int);
>  	void (*halt)(void);
> + 	void (*set_lazy_mode)(int mode);
>  } vmi_ops;
>  
>  /* XXX move this to alternative.h */
> @@ -574,6 +575,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));
>   

It's probably better to use __get_cpu_var() rather than per_cpu(X,
smp_processor_id());  It's a real pity there's no ^^ operator...

> +	
> +	if (new_mode)
> +		local_irq_save(per_cpu(flags, cpu));
> +	else
> +		local_irq_restore(per_cpu(flags, cpu));
>   

The comment only talks about disabling interrupts for lazy_mmu, but this
seems to do it for lazy_cpu as well.  Is that OK?  What happens if
someone wants to change interrupt states under lazy_cpu; I can't think
of an inherent reason why that wouldn't be allowed (though I don't think
it happens now).

This kind of logic is a bit clunky anyway; would it be better to simply
have separate enable/disable functions?  Or at least separate functions
per mode?

Anyway,

Acked-by: Jeremy Fitzhardinge <jeremy@xensource.com>

    J