Re: [PATCH] x86: make SMP locks handling interact properly with CONFIG_DEBUG_RODATA (2nd try)

Andi Kleen
2007-07-17T17:54:50Z

On Wednesday 04 July 2007 08:33:24 Jan Beulich wrote:

Sorry for late feedback.

> +#ifdef CONFIG_DEBUG_RODATA
> +
> +#ifdef CONFIG_X86_32
> +#include <asm/highmem.h>
> +#define MODULES_VADDR VMALLOC_START
> +#endif
> +
> +static inline void make_writable(const void *instr, unsigned int len)
> +{
> + unsigned long va = (unsigned long)instr;
> +
> + if (va < MODULES_VADDR) {
> + change_page_attr(virt_to_page(instr),
> + PFN_UP(va + len) - PFN_DOWN(va),
> + PAGE_KERNEL_EXEC);
> + global_flush_tlb();
> + }
> +}
> +
> +static inline void make_readonly(const void *instr, unsigned int len)
> +{
> + unsigned long va = (unsigned long)instr;
> +
> + if (va < MODULES_VADDR) {
> + change_page_attr(virt_to_page(instr),
> + PFN_UP(va + len) - PFN_DOWN(va),
> +#ifdef CONFIG_X86_64
> + PAGE_KERNEL_RO);
> +#else
> + PAGE_KERNEL_RX);
> +#endif
> + global_flush_tlb();
> + }
> +}

Can you please move those into the respective pageattr.cs without ifdefs?
Also the va < MODULES_VADDR test seems overly magic; while it may work right now
it seems fragile. It would be better to test the exact ranges in the respective
architectures

> void alternatives_smp_module_del(struct module *mod)
> {
> struct smp_alt_module *item;
> - unsigned long flags;
>
> if (smp_alt_once || noreplace_smp)
> return;
>
> - spin_lock_irqsave(&smp_alt, flags);
> + spin_lock(&smp_alt);

Unrelated change? Why? Should probably be separate patch.

> --- linux-2.6.22-rc7/arch/x86_64/kernel/vmlinux.lds.S 2007-07-03 10:57:39.000000000 +0200
> +++ 2.6.22-rc7-x86-alt-page-attr/arch/x86_64/kernel/vmlinux.lds.S 2007-07-02 14:40:15.000000000 +0200
> @@ -131,20 +131,11 @@ SECTIONS
> /* might get freed after init */
> . = ALIGN(4096);
> __smp_alt_begin = .;
> - __smp_alt_instructions = .;
> - .smp_altinstructions : AT(ADDR(.smp_altinstructions) - LOAD_OFFSET) {
> - *(.smp_altinstructions)
> - }
> - __smp_alt_instructions_end = .;
> - . = ALIGN(8);
> __smp_locks = .;
> .smp_locks : AT(ADDR(.smp_locks) - LOAD_OFFSET) {
> *(.smp_locks)
> }
> __smp_locks_end = .;
> - .smp_altinstr_replacement : AT(ADDR(.smp_altinstr_replacement) - LOAD_OFFSET) {
> - *(.smp_altinstr_replacement)
> - }

While ok too it should be also separate please.

-Andi