Hi,
I was getting this BUG while running into a GPF:
BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-x86/2248
caller is arch_trigger_all_cpu_backtrace_handler+0x1d/0xf7
Pid: 2248, comm: qemu-system-x86 Not tainted 2.6.36+ #12
Call Trace:
[<ffffffff81203194>] debug_smp_processor_id+0xd8/0xf4
[<ffffffff813803ab>] arch_trigger_all_cpu_backtrace_handler+0x1d/0xf7
[<ffffffff81381e64>] notifier_call_chain+0xa4/0xdb
[<ffffffff81381efe>] __atomic_notifier_call_chain+0x63/0x95
[<ffffffff81381e9b>] ? __atomic_notifier_call_chain+0x0/0x95
[<ffffffff81381cfb>] ? sub_preempt_count+0x97/0xaa
[<ffffffff8121de8c>] ? pfn_to_dma_pte+0x73/0x190
[<ffffffff81381f44>] atomic_notifier_call_chain+0x14/0x16
[<ffffffff81381f74>] notify_die+0x2e/0x30
[<ffffffff8137f498>] do_general_protection+0x121/0x142
[<ffffffff8137ec04>] ? irq_return+0x0/0xc
[<ffffffff8137ede5>] general_protection+0x25/0x30
[<ffffffff8121de8c>] ? pfn_to_dma_pte+0x73/0x190
[<ffffffff8121df5b>] ? pfn_to_dma_pte+0x142/0x190
[<ffffffff8121dfbe>] intel_iommu_iova_to_phys+0x15/0x2a
[<ffffffff812a9573>] iommu_iova_to_phys+0x13/0x15
[<ffffffffa04e91d0>] kvm_iommu_map_pages+0x77/0x194 [kvm]
[<ffffffff8111404d>] ? __vmalloc_node+0x86/0x9b
[<ffffffffa04e30e2>] __kvm_set_memory_region+0x4e5/0x787 [kvm]
[<ffffffff81081ee8>] ? mark_held_locks+0x50/0x72
[<ffffffff8137c983>] ? mutex_lock_nested+0x325/0x34d
[<ffffffffa04e33bb>] kvm_set_memory_region+0x37/0x50 [kvm]
[<ffffffffa04e4c15>] kvm_vm_ioctl_set_memory_region+0x18/0x1a [kvm]
[<ffffffffa04e4e44>] kvm_vm_ioctl+0x22d/0x3b1 [kvm]
[<ffffffff811355aa>] ? fget_light+0x17b/0x31f
[<ffffffff81143bd7>] do_vfs_ioctl+0x4c6/0x507
[<ffffffff81135732>] ? fget_light+0x303/0x31f
[<ffffffff811355aa>] ? fget_light+0x17b/0x31f
[<ffffffff8137ebb9>] ? retint_swapgs+0x13/0x1b
[<ffffffff81143c6e>] sys_ioctl+0x56/0x7c
[<ffffffff81002df2>] system_call_fastpath+0x16/0x1b
I guess this is not intended to trigger here, specifically as it showed
up first and may be misinterpreted as the core of the issue.
Jan
On Mon, Nov 01, 2010 at 01:39:56PM +0100, Jan Kiszka wrote:
> Hi,
>
> I was getting this BUG while running into a GPF:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-x86/2248
> caller is arch_trigger_all_cpu_backtrace_handler+0x1d/0xf7
> Pid: 2248, comm: qemu-system-x86 Not tainted 2.6.36+ #12
> Call Trace:
> [<ffffffff81203194>] debug_smp_processor_id+0xd8/0xf4
> [<ffffffff813803ab>] arch_trigger_all_cpu_backtrace_handler+0x1d/0xf7
...
Thanks for report Jan. I'll take a look as only get ability.
Cyrill
On Mon, Nov 01, 2010 at 01:39:56PM +0100, Jan Kiszka wrote:
> Hi,
>
> I was getting this BUG while running into a GPF:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: qemu-system-x86/2248
> caller is arch_trigger_all_cpu_backtrace_handler+0x1d/0xf7
> Pid: 2248, comm: qemu-system-x86 Not tainted 2.6.36+ #12
> Call Trace:
> [<ffffffff81203194>] debug_smp_processor_id+0xd8/0xf4
> [<ffffffff813803ab>] arch_trigger_all_cpu_backtrace_handler+0x1d/0xf7
> [<ffffffff81381e64>] notifier_call_chain+0xa4/0xdb
> [<ffffffff81381efe>] __atomic_notifier_call_chain+0x63/0x95
> [<ffffffff81381e9b>] ? __atomic_notifier_call_chain+0x0/0x95
> [<ffffffff81381cfb>] ? sub_preempt_count+0x97/0xaa
> [<ffffffff8121de8c>] ? pfn_to_dma_pte+0x73/0x190
> [<ffffffff81381f44>] atomic_notifier_call_chain+0x14/0x16
> [<ffffffff81381f74>] notify_die+0x2e/0x30
> [<ffffffff8137f498>] do_general_protection+0x121/0x142
> [<ffffffff81002df2>] system_call_fastpath+0x16/0x1b
<snip>
>
> I guess this is not intended to trigger here, specifically as it showed
> up first and may be misinterpreted as the core of the issue.
Heh. Yeah when I migrated the code, I completely forgot the notifier
chain could be called from a preemptible context (ie not NMI).
This patch should fix it and I think it is the correct fix. Let me know
how it works out.
Cheers,
Don
diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
index c7c9ae4..1bdd0b5 100644
--- a/arch/x86/kernel/apic/hw_nmi.c
+++ b/arch/x86/kernel/apic/hw_nmi.c
@@ -63,7 +63,7 @@ arch_trigger_all_cpu_backtrace_handler(struct notifier_block *self,
{
struct die_args *args = __args;
struct pt_regs *regs;
- int cpu = smp_processor_id();
+ int cpu;
switch (cmd) {
case DIE_NMI:
@@ -74,6 +74,7 @@ arch_trigger_all_cpu_backtrace_handler(struct notifier_block *self,
}
regs = args->regs;
+ cpu = smp_processor_id();
if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED;
On Mon, Nov 01, 2010 at 11:45:36AM -0400, Don Zickus wrote:
...
> Heh. Yeah when I migrated the code, I completely forgot the notifier
> chain could be called from a preemptible context (ie not NMI).
>
> This patch should fix it and I think it is the correct fix. Let me know
> how it works out.
>
> Cheers,
> Don
>
> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
> index c7c9ae4..1bdd0b5 100644
> --- a/arch/x86/kernel/apic/hw_nmi.c
> +++ b/arch/x86/kernel/apic/hw_nmi.c
> @@ -63,7 +63,7 @@ arch_trigger_all_cpu_backtrace_handler(struct notifier_block *self,
> {
> struct die_args *args = __args;
> struct pt_regs *regs;
> - int cpu = smp_processor_id();
> + int cpu;
>
> switch (cmd) {
> case DIE_NMI:
> @@ -74,6 +74,7 @@ arch_trigger_all_cpu_backtrace_handler(struct notifier_block *self,
> }
>
> regs = args->regs;
> + cpu = smp_processor_id();
>
> if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
> static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED;
>
yup, this will do the trick for a while. In general I believe we might have
kind of NMI exclusive chain so we wouldn't need the 'case:'s.
Cyrill
2010/11/1 Cyrill Gorcunov <[email protected]>:
> On Mon, Nov 01, 2010 at 11:45:36AM -0400, Don Zickus wrote:
> ...
>> Heh. ?Yeah when I migrated the code, I completely forgot the notifier
>> chain could be called from a preemptible context (ie not NMI).
>>
>> This patch should fix it and I think it is the correct fix. ?Let me know
>> how it works out.
>>
>> Cheers,
>> Don
>>
>> diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c
>> index c7c9ae4..1bdd0b5 100644
>> --- a/arch/x86/kernel/apic/hw_nmi.c
>> +++ b/arch/x86/kernel/apic/hw_nmi.c
>> @@ -63,7 +63,7 @@ arch_trigger_all_cpu_backtrace_handler(struct notifier_block *self,
>> ?{
>> ? ? ? struct die_args *args = __args;
>> ? ? ? struct pt_regs *regs;
>> - ? ? int cpu = smp_processor_id();
>> + ? ? int cpu;
>>
>> ? ? ? switch (cmd) {
>> ? ? ? case DIE_NMI:
>> @@ -74,6 +74,7 @@ arch_trigger_all_cpu_backtrace_handler(struct notifier_block *self,
>> ? ? ? }
>>
>> ? ? ? regs = args->regs;
>> + ? ? cpu = smp_processor_id();
>>
>> ? ? ? if (cpumask_test_cpu(cpu, to_cpumask(backtrace_mask))) {
>> ? ? ? ? ? ? ? static arch_spinlock_t lock = __ARCH_SPIN_LOCK_UNLOCKED;
>>
>
> ?yup, this will do the trick for a while. In general I believe we might have
> kind of NMI exclusive chain so we wouldn't need the 'case:'s.
Yeah. And seperating NMIs from the rest of the DIE notifiers would
probably improve
performance of things like PMI handling.
And I've always been confused with this "die" notifier semantic. We
are not dying when
we handle a counter overflow interrupt.
The same applies to DIE_INT3, DIE_TRAP, DIE_DEBUG, ....
But until then, as having a seperate notifier is quite a refactoring,
we should enqueue Don's fix.
Don, can you resend it with usual SOB and changelog?
Thanks.
On Fri, Nov 05, 2010 at 05:06:55PM +0100, Frederic Weisbecker wrote:
> 2010/11/1 Cyrill Gorcunov <[email protected]>:
...
> > ?yup, this will do the trick for a while. In general I believe we might have
> > kind of NMI exclusive chain so we wouldn't need the 'case:'s.
>
> Yeah. And seperating NMIs from the rest of the DIE notifiers would
> probably improve
> performance of things like PMI handling.
>
It will make it more straight and clean as minimum which is already
quite a benefit ;) There were patches floating from Don with notify
priorities which are good start. Though I didn't manage to look into
most of patches I've been CC'ed yet :(
> And I've always been confused with this "die" notifier semantic. We
> are not dying when
> we handle a counter overflow interrupt.
> The same applies to DIE_INT3, DIE_TRAP, DIE_DEBUG, ....
>
As the comment says on top of the enum they are "Grossly misnamed" ;)
Though we could consider them not as "dying" but rather in terms of say
on-die signals (or something like that).
> But until then, as having a seperate notifier is quite a refactoring,
> we should enqueue Don's fix.
> Don, can you resend it with usual SOB and changelog?
>
> Thanks.
>
Cyrill