[BUG/PATCH] x86 mmiotrace: dynamically disable non-boot CPUs - Kernel

This is a discussion on [BUG/PATCH] x86 mmiotrace: dynamically disable non-boot CPUs - Kernel ; From 35922163c8054bd8d12df78ebae6a6a603760206 Mon Sep 17 00:00:00 2001 From: Pekka Paalanen Date: Mon, 21 Apr 2008 21:36:09 +0300 Subject: [PATCH] x86_64: fix kernel rodata NX setting Without CONFIG_DYNAMIC_FTRACE, mark_rodata_ro() would mark a wrong number of pages as no-execute. The bug was ...

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2
Results 21 to 29 of 29

Thread: [BUG/PATCH] x86 mmiotrace: dynamically disable non-boot CPUs

  1. [PATCH] x86_64: fix kernel rodata NX setting

    From 35922163c8054bd8d12df78ebae6a6a603760206 Mon Sep 17 00:00:00 2001
    From: Pekka Paalanen
    Date: Mon, 21 Apr 2008 21:36:09 +0300
    Subject: [PATCH] x86_64: fix kernel rodata NX setting

    Without CONFIG_DYNAMIC_FTRACE, mark_rodata_ro() would mark a wrong
    number of pages as no-execute. The bug was introduced in the patch
    "ftrace: dont write protect kernel text". The symptom was machine reboot
    after a CPU hotplug.

    Signed-off-by: Pekka Paalanen
    ---

    Is this ok? At least it works for me:

    [ 294.564032] CPU 1 is now offline
    [ 294.564359] lockdep: fixing up alternatives.
    [ 294.565799] SMP alternatives: switching to UP code
    [ 306.626760] lockdep: fixing up alternatives.
    [ 306.627080] SMP alternatives: switching to SMP code
    [ 306.638566] Booting processor 1/1 ip 6000
    [ 306.653079] Initializing CPU#1
    [ 306.736556] Calibrating delay using timer specific routine.. 3991.08 BogoMIPS (lpj=6649734)
    [ 306.739889] CPU: L1 I cache: 32K, L1 D cache: 32K
    [ 306.739889] CPU: L2 cache: 4096K
    [ 306.739889] CPU: Physical Processor ID: 0
    [ 306.739889] CPU: Processor Core ID: 1
    [ 306.739889] x86: PAT support disabled.
    [ 306.741127] CPU1: <6>Clockevents: could not switch to one-shot mode: lapic is not functional.
    [ 306.743865] Could not switch to high resolution mode on CPU 1
    [ 306.750580] Intel(R) Core(TM)2 Duo CPU T7300 @ 2.00GHz stepping 0a

    arch/x86/mm/init_64.c | 2 +-
    1 files changed, 1 insertions(+), 1 deletions(-)

    diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
    index 7851773..d7ae30a 100644
    --- a/arch/x86/mm/init_64.c
    +++ b/arch/x86/mm/init_64.c
    @@ -792,7 +792,7 @@ void mark_rodata_ro(void)
    * The rodata section (but not the kernel text!) should also be
    * not-executable.
    */
    - set_memory_nx(rodata_start, (end - start) >> PAGE_SHIFT);
    + set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);

    rodata_test();

    --
    1.5.3.7

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] x86_64: fix kernel rodata NX setting


    On Mon, 21 Apr 2008, Pekka Paalanen wrote:

    > From 35922163c8054bd8d12df78ebae6a6a603760206 Mon Sep 17 00:00:00 2001
    > From: Pekka Paalanen
    > Date: Mon, 21 Apr 2008 21:36:09 +0300
    > Subject: [PATCH] x86_64: fix kernel rodata NX setting
    >
    > Without CONFIG_DYNAMIC_FTRACE, mark_rodata_ro() would mark a wrong
    > number of pages as no-execute. The bug was introduced in the patch
    > "ftrace: dont write protect kernel text". The symptom was machine reboot
    > after a CPU hotplug.
    >
    > Signed-off-by: Pekka Paalanen


    Good catch!

    Acked-by: Steven Rostedt

    -- Steve

    > arch/x86/mm/init_64.c | 2 +-
    > 1 files changed, 1 insertions(+), 1 deletions(-)
    >
    > diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
    > index 7851773..d7ae30a 100644
    > --- a/arch/x86/mm/init_64.c
    > +++ b/arch/x86/mm/init_64.c
    > @@ -792,7 +792,7 @@ void mark_rodata_ro(void)
    > * The rodata section (but not the kernel text!) should also be
    > * not-executable.
    > */
    > - set_memory_nx(rodata_start, (end - start) >> PAGE_SHIFT);
    > + set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
    >
    > rodata_test();
    >
    > --
    > 1.5.3.7
    >
    >

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] x86_64: fix kernel rodata NX setting


    * Pekka Paalanen wrote:

    > From 35922163c8054bd8d12df78ebae6a6a603760206 Mon Sep 17 00:00:00 2001
    > From: Pekka Paalanen
    > Date: Mon, 21 Apr 2008 21:36:09 +0300
    > Subject: [PATCH] x86_64: fix kernel rodata NX setting
    >
    > Without CONFIG_DYNAMIC_FTRACE, mark_rodata_ro() would mark a wrong
    > number of pages as no-execute. The bug was introduced in the patch
    > "ftrace: dont write protect kernel text". The symptom was machine
    > reboot after a CPU hotplug.


    thanks Pekka, nice fix!

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  4. [repost PATCH] Fix SMP alternatives : use mutex instead of spinlock, text_poke is sleepable

    From 58390e93507669d860ef2d178b60ad447d5260c4 Mon Sep 17 00:00:00 2001
    From: Pekka Paalanen
    Date: Sat, 19 Apr 2008 23:22:11 +0300
    Subject: [PATCH] x86: Fix SMP alternatives: use mutex instead of spinlock.

    text_poke is sleepable.
    The original fix by Mathieu Desnoyers .

    Signed-off-by: Pekka Paalanen
    ---

    > Mathieu,
    >
    > thank you for the quick reply. Your patch did not apply on sched-devel/latest
    > so I made the changes by hand, resulting patch here. The change was in the
    > context lines, which made `patch' fail, e.g. __FUNCTION__ -> __func__.
    > Now my enter_uniprocessor(), that disables all but the first cpu, works fine.
    >
    > My next quest is, why attempting to re-enable the cpus makes the whole
    > machine reboot after a short hang.


    Ingo,

    I see you applied the NX fix. This patch is the other one needed to make
    CPU hotplug working. Could you apply this, unless Mathieu has something
    against it?

    After this one I can resubmit the mmiotrace CPU auto-disable patch
    (modified a bit). Though it will be a couple of days before I can get
    back to coding.


    Thanks.

    arch/x86/kernel/alternative.c | 18 +++++++++---------
    1 files changed, 9 insertions(+), 9 deletions(-)

    diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
    index 5412fd7..7ce0939 100644
    --- a/arch/x86/kernel/alternative.c
    +++ b/arch/x86/kernel/alternative.c
    @@ -1,6 +1,6 @@
    #include
    #include
    -#include
    +#include
    #include
    #include
    #include
    @@ -279,7 +279,7 @@ struct smp_alt_module {
    struct list_head next;
    };
    static LIST_HEAD(smp_alt_modules);
    -static DEFINE_SPINLOCK(smp_alt);
    +static DEFINE_MUTEX(smp_alt);
    static int smp_mode = 1; /* protected by smp_alt */

    void alternatives_smp_module_add(struct module *mod, char *name,
    @@ -312,12 +312,12 @@ void alternatives_smp_module_add(struct module *mod, char *name,
    __func__, smp->locks, smp->locks_end,
    smp->text, smp->text_end, smp->name);

    - spin_lock(&smp_alt);
    + mutex_lock(&smp_alt);
    list_add_tail(&smp->next, &smp_alt_modules);
    if (boot_cpu_has(X86_FEATURE_UP))
    alternatives_smp_unlock(smp->locks, smp->locks_end,
    smp->text, smp->text_end);
    - spin_unlock(&smp_alt);
    + mutex_unlock(&smp_alt);
    }

    void alternatives_smp_module_del(struct module *mod)
    @@ -327,17 +327,17 @@ void alternatives_smp_module_del(struct module *mod)
    if (smp_alt_once || noreplace_smp)
    return;

    - spin_lock(&smp_alt);
    + mutex_lock(&smp_alt);
    list_for_each_entry(item, &smp_alt_modules, next) {
    if (mod != item->mod)
    continue;
    list_del(&item->next);
    - spin_unlock(&smp_alt);
    + mutex_unlock(&smp_alt);
    DPRINTK("%s: %s\n", __func__, item->name);
    kfree(item);
    return;
    }
    - spin_unlock(&smp_alt);
    + mutex_unlock(&smp_alt);
    }

    void alternatives_smp_switch(int smp)
    @@ -359,7 +359,7 @@ void alternatives_smp_switch(int smp)
    return;
    BUG_ON(!smp && (num_online_cpus() > 1));

    - spin_lock(&smp_alt);
    + mutex_lock(&smp_alt);

    /*
    * Avoid unnecessary switches because it forces JIT based VMs to
    @@ -383,7 +383,7 @@ void alternatives_smp_switch(int smp)
    mod->text, mod->text_end);
    }
    smp_mode = smp;
    - spin_unlock(&smp_alt);
    + mutex_unlock(&smp_alt);
    }

    #endif
    --
    1.5.3.7

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  5. Re: [repost PATCH] Fix SMP alternatives : use mutex instead of spinlock, text_poke is sleepable


    * Pekka Paalanen wrote:

    > Ingo,
    >
    > I see you applied the NX fix. This patch is the other one needed to
    > make CPU hotplug working. Could you apply this, unless Mathieu has
    > something against it?


    sure, patch looks sane - applied it and pushed it out. Thanks,

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  6. Re: [repost PATCH] Fix SMP alternatives : use mutex instead of spinlock, text_poke is sleepable

    * Ingo Molnar (mingo@elte.hu) wrote:
    >
    > * Pekka Paalanen wrote:
    >
    > > Ingo,
    > >
    > > I see you applied the NX fix. This patch is the other one needed to
    > > make CPU hotplug working. Could you apply this, unless Mathieu has
    > > something against it?

    >
    > sure, patch looks sane - applied it and pushed it out. Thanks,
    >
    > Ingo


    Ingo,

    The patch found there should also be pulled (at least the
    is_vmalloc_addr -> !core_kernel_text bit) :

    http://lkml.org/lkml/2008/4/20/192

    On x86_64, module code is not in is_vmalloc_addr range, so we can end up
    using virt_to_page on module text addresses.

    Mathieu

    --
    Mathieu Desnoyers
    Computer Engineering Ph.D. Student, Ecole Polytechnique de Montreal
    OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  7. [PATCH v2] x86 mmiotrace: dynamically disable non-boot CPUs

    From 8979ee55cb6a429c4edd72ebec2244b849f6a79a Mon Sep 17 00:00:00 2001
    From: Pekka Paalanen
    Date: Sat, 12 Apr 2008 00:18:57 +0300
    Subject: [PATCH] x86 mmiotrace: dynamically disable non-boot CPUs

    Mmiotrace is not reliable with multiple CPUs and may
    miss events. Drop to single CPU when mmiotrace is activated.

    Signed-off-by: Pekka Paalanen
    ---

    I've tested this patch on my Core 2 Duo laptop, and with
    "[PATCH v2] Check for breakpoint in text_poke to eliminate bug_on"
    cpu hotplug seems to work fine on a sched-devel/latest SMP kernel
    both with and without CONFIG_HOTPLUG_CPU.

    One strange thing was a stack trace printed just before the CPU resets
    for "shutdown -r now", but I could not see what it was about. I'll ignore
    that for now, hopefully it's something unrelated.

    arch/x86/mm/mmio-mod.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++
    1 files changed, 61 insertions(+), 0 deletions(-)

    diff --git a/arch/x86/mm/mmio-mod.c b/arch/x86/mm/mmio-mod.c
    index 6d6cac8..1f77d85 100644
    --- a/arch/x86/mm/mmio-mod.c
    +++ b/arch/x86/mm/mmio-mod.c
    @@ -32,6 +32,7 @@
    #include /* for ISA_START_ADDRESS */
    #include
    #include
    +#include

    #include "pf_in.h"

    @@ -400,6 +401,64 @@ static void clear_trace_list(void)
    }
    }

    +#ifdef CONFIG_HOTPLUG_CPU
    +static cpumask_t downed_cpus;
    +
    +static void enter_uniprocessor(void)
    +{
    + int cpu;
    + int err;
    +
    + get_online_cpus();
    + downed_cpus = cpu_online_map;
    + cpu_clear(first_cpu(cpu_online_map), downed_cpus);
    + if (num_online_cpus() > 1)
    + pr_notice(NAME "Disabling non-boot CPUs...\n");
    + put_online_cpus();
    +
    + for_each_cpu_mask(cpu, downed_cpus) {
    + err = cpu_down(cpu);
    + if (!err) {
    + pr_info(NAME "CPU%d is down.\n", cpu);
    + } else {
    + pr_err(NAME "Error taking CPU%d down: %d\n", cpu, err);
    + }
    + }
    + if (num_online_cpus() > 1)
    + pr_warning(NAME "multiple CPUs still online, "
    + "may miss events.\n");
    +}
    +
    +static void leave_uniprocessor(void)
    +{
    + int cpu;
    + int err;
    +
    + if (cpus_weight(downed_cpus) == 0)
    + return;
    + pr_notice(NAME "Re-enabling CPUs...\n");
    + for_each_cpu_mask(cpu, downed_cpus) {
    + err = cpu_up(cpu);
    + if (!err)
    + pr_info(NAME "enabled CPU%d.\n", cpu);
    + else
    + pr_err(NAME "cannot re-enable CPU%d: %d\n", cpu, err);
    + }
    +}
    +
    +#else /* !CONFIG_HOTPLUG_CPU */
    +static void enter_uniprocessor(void)
    +{
    + if (num_online_cpus() > 1)
    + pr_warning(NAME "multiple CPUs are online, may miss events. "
    + "Suggest booting with maxcpus=1 kernel argument.\n");
    +}
    +
    +static void leave_uniprocessor(void)
    +{
    +}
    +#endif
    +
    #if 0 /* XXX: out of order */
    static struct file_operations fops_marker = {
    .owner = THIS_MODULE,
    @@ -422,6 +481,7 @@ void enable_mmiotrace(void)

    if (nommiotrace)
    pr_info(NAME "MMIO tracing disabled.\n");
    + enter_uniprocessor();
    spin_lock_irq(&trace_lock);
    atomic_inc(&mmiotrace_enabled);
    spin_unlock_irq(&trace_lock);
    @@ -442,6 +502,7 @@ void disable_mmiotrace(void)
    spin_unlock_irq(&trace_lock);

    clear_trace_list(); /* guarantees: no more kmmio callbacks */
    + leave_uniprocessor();
    if (marker_file) {
    debugfs_remove(marker_file);
    marker_file = NULL;
    --
    1.5.3.7

    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  8. Re: [PATCH v2] x86 mmiotrace: dynamically disable non-boot CPUs


    * Pekka Paalanen wrote:

    > From 8979ee55cb6a429c4edd72ebec2244b849f6a79a Mon Sep 17 00:00:00 2001
    > From: Pekka Paalanen
    > Date: Sat, 12 Apr 2008 00:18:57 +0300
    > Subject: [PATCH] x86 mmiotrace: dynamically disable non-boot CPUs
    >
    > Mmiotrace is not reliable with multiple CPUs and may miss events. Drop
    > to single CPU when mmiotrace is activated.
    >
    > Signed-off-by: Pekka Paalanen
    > ---
    >
    > I've tested this patch on my Core 2 Duo laptop, and with "[PATCH v2]
    > Check for breakpoint in text_poke to eliminate bug_on" cpu hotplug
    > seems to work fine on a sched-devel/latest SMP kernel both with and
    > without CONFIG_HOTPLUG_CPU.


    thanks Pekka, applied.

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  9. Re: [Nouveau] [BUG/PATCH] x86 mmiotrace: dynamically disable non-boot CPUs

    On Wed, Apr 16, 2008 at 6:59 PM, Pekka Paalanen wrote:
    > On Wed, 16 Apr 2008 13:46:09 +0200
    > Ingo Molnar wrote:
    >
    >>
    >> * Pekka Paalanen wrote:
    >>
    >> > > we should fix this restriction ASAP. Forcibly dropping to UP will
    >> > > cause mmiotrace to be much less useful for diagnostic purposes of
    >> > > Linux
    >> >
    >> > Ok, how do you propose we solve this?
    >> >
    >> > I have asked the question before, and then I had two ideas. Well, the
    >> > first one was actually your idea (so I hear) to solve the same problem for
    >> > kmemcheck.
    >> > - per-cpu page tables
    >> > - instead of single-stepping, emulate the faulting instruction and never
    >> > disarm pages during tracing. (Use and modify code from KVM.)
    >> >
    >> > I don't believe either of these is easy or fast to implement. Given
    >> > some months, I might be able to achieve emulation. Page tables are
    >> > still magic to me.

    >>
    >> yeah - it looks complex. Not a showstopper for now :-)
    >>
    >> but given that Xorg is usually just a single task, do we _really_ need
    >> this?

    >
    > We're not tracing Xorg at all. Mmiotrace still cannot catch accesses
    > originating in user space. It is tracing MMIO accesses from within
    > the kernel, and this means that IRQ services and device syscalls
    > may be accessing the hardware at the same time. Vblank interrupts
    > happen quite often, some GPU commands are actually emulated in
    > kernel via interrupts and whatnot. The nvidia proprietary kernel blob
    > is many times bigger than my bzImage!
    >
    > (A simple X startup and quit creates in the order of 1-2 million
    > MMIO events.)
    >
    > As do we really need this, I think it might save a lot of head
    > scratching when someone is reverse engineering a feature and gets
    > every time a different trace due to some events being missed.
    > But this is theory. So far everyone has been tracing with UP,
    > and this has not been a problem. I have no idea if it would make
    > a real difference.
    >
    > [Recap for nouveau@ list:
    > mmiotrace has a race on SMP, where during instruction single stepping
    > other CPUs can run freely on the page which the faulted instruction
    > accessed. This causes some of the simultaneous accesses to the same
    > page of the same iomem-mapping to be missed.]
    >
    > It does sound very rare. Nouveau people, what do you think, can this
    > be a problem?
    >


    In the nvidia case, I don't think this would happen. The register
    ranges for different purposes are set apart by more than 1 page
    usually, so the risk of accessing a page that's been faulted in is
    probably extremely low. Not to mention that the design of the binary
    module doesn't use threads currently (only tasklets for interrupt
    handlers, this might be the corner case but again the interrupt
    handler doesn't touch the same reg families).

    Stephane
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread
Page 2 of 2 FirstFirst 1 2