Re: [patch 05/10] Text Edit Lock - Alternative code for i386 and x86_64

[Andi Kleen <andi@firstfloor.org>]

On Thu, Sep 06, 2007 at 04:01:29PM -0400, Mathieu Desnoyers wrote:
> +	sync_core();
> +	/* Not strictly needed, but can speed CPU recovery up. */

That turned out to break on some VIA CPUs. Should be removed.

> +	if (cpu_has_clflush)
> +		for (faddr = addr; faddr < addr + len;
> +				faddr += boot_cpu_data.x86_clflush_size)
> +			asm("clflush (%0) " :: "r" (faddr) : "memory");
> +}
> +
> +void * text_poke_early(void *addr, const void *opcode,
> +					size_t len)
> +{
> +	memcpy(addr, opcode, len);

It would be best to copy __inline_memcpy from x86-64 to i386
and use that here. That avoids the dependency on a patched
memcpy and is slightly safer.

> +
> +	if (len > sizeof(long)) {
> +		printk(KERN_ERR "text_poke of len %zu too big (max %lu)\n",
> +			len, sizeof(long));
> +		BUG_ON(1);

In general BUG_ON only should be enough because these values can
be recovered from the registers. 

> +	}
> +	unaligned = (((long)addr + len - 1) & ~(sizeof(long) - 1))
> +		- ((long)addr & ~(sizeof(long) - 1));
> +	if (unlikely(unaligned)) {
> +		printk(KERN_ERR "text_poke of at addr %p of len %zu is "
> +				"unaligned (%d)\n",
> +			addr, len, unaligned);
> +		BUG_ON(1);
> +	}

The common code should be in a common function. In fact they're so 
similar that the caller could just pass a buffer for the text_set
case, couldn't it?


> +#define kernel_wp_save(cr0)					\

Is there a real reason this has to be an macro? It could 
be just a normal function. In fact a shared on in alternative.c.
That would also avoid adding more include dependencies.

> +	do {							\
> +		typecheck(unsigned long, cr0);			\

typecheck is probably overkill

> +		preempt_disable();				\

Should disable interrupts too just to be safer? 

-Andi