v3.9 - CPU hotplug and microcode earlier loading hits a mutex deadlock (x86_cpu_hotplug_driver_mutex)

DISCUSSION Konrad Rzeszutek Wilk 6 participants 13 messages May 6, 2013 – May 9, 2013 ACPI

lore

Share & tools

lore

Konrad Rzeszutek Wilk

author

2013-05-06 12:59 UTC #0 lore raw

Hey,

As I was fixing the PV HVM's broken CPU hotplug mechanism I discovered a deadlock in the microcode and generic code.

I am not sure if the ACPI generic mechanism would expose this, but looking at the flow (arch_register_cpu, then expecting user-space to call cpu_up), it should trigger this.

Anyhow, this can easily be triggered if a new CPU is added and from user-space do:

echo 1 > /sys/devices/system/cpu/cpu3/online

on a newly appeared CPU. The deadlock is that the "store_online" in drivers/base/cpu.c takes the cpu_hotplug_driver_lock() lock, then calls "cpu_up". "cpu_up" eventually ends up calling "save_mc_for_early" which also takes the cpu_hotplug_driver_lock() lock.

And here is that kernel thinks of it:

smpboot: Stack at about ffff880075c39f44
smpboot: CPU3: has booted.
microcode: CPU3 sig=0x206a7, pf=0x2, revision=0x25

=============================================
[ INFO: possible recursive locking detected ]
3.9.0upstream-10129-g167af0e #1 Not tainted
---------------------------------------------
sh/2487 is trying to acquire lock:
(x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20

but task is already holding lock:
(x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock(x86_cpu_hotplug_driver_mutex);
lock(x86_cpu_hotplug_driver_mutex);

*** DEADLOCK ***

May be due to missing lock nesting notation

6 locks held by sh/2487:

 #0:  (sb_writers#5){.+.+.+}, at: [<ffffffff811ca48d>] vfs_write+0x17d/0x190
 #1:  (&buffer->mutex){+.+.+.}, at: [<ffffffff812464ef>] sysfs_write_file+0x3f/0x160
 #2:  (s_active#20){.+.+.+}, at: [<ffffffff81246578>] sysfs_write_file+0xc8/0x160
 #3:  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
 #4:  (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810961c2>] cpu_maps_update_begin+0x12/0x20
 #5:  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff810962a7>] cpu_hotplug_begin+0x27/0x60

stack backtrace:
CPU: 1 PID: 2487 Comm: sh Not tainted 3.9.0upstream-10129-g167af0e #1
Hardware name: Xen HVM domU, BIOS 4.3-unstable 05/03/2013
 ffffffff8229b710 ffff880064e75538 ffffffff816fe47e ffff880064e75608
 ffffffff81100cb6 ffff880064e75560 ffff88006670e290 ffff880064e75588
 ffffffff00000000 ffff88006670e9b8 21710c800c83a1f5 ffff880064e75598
Call Trace:
 [<ffffffff816fe47e>] dump_stack+0x19/0x1b
 [<ffffffff81100cb6>] __lock_acquire+0x726/0x1890
 [<ffffffff8110b352>] ? is_module_text_address+0x22/0x40
 [<ffffffff810bbff8>] ? __kernel_text_address+0x58/0x80
 [<ffffffff81101eca>] lock_acquire+0xaa/0x190
 [<ffffffff81075512>] ? cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff816fee1e>] __mutex_lock_common+0x5e/0x450
 [<ffffffff81075512>] ? cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff810d4225>] ? sched_clock_local+0x25/0x90
 [<ffffffff81075512>] ? cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff816ff340>] mutex_lock_nested+0x40/0x50
 [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff81080757>] save_mc_for_early+0x27/0xf0
 [<ffffffff810ffd30>] ? mark_held_locks+0x90/0x150
 [<ffffffff81176a5d>] ? get_page_from_freelist+0x46d/0x8e0
 [<ffffffff8110029d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81056a39>] ? sched_clock+0x9/0x10
 [<ffffffff81177275>] ? __alloc_pages_nodemask+0x165/0xa30
 [<ffffffff810d4225>] ? sched_clock_local+0x25/0x90
 [<ffffffff810d4348>] ? sched_clock_cpu+0xb8/0x110
 [<ffffffff810fb88d>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff811a5e79>] ? vmap_page_range_noflush+0x279/0x370
 [<ffffffff811a5f9d>] ? map_vm_area+0x2d/0x50
 [<ffffffff811a7dce>] ? __vmalloc_node_range+0x18e/0x260
 [<ffffffff810812a8>] ? generic_load_microcode+0xb8/0x1c0
 [<ffffffff8108135c>] generic_load_microcode+0x16c/0x1c0
 [<ffffffff8110917e>] ? generic_exec_single+0x7e/0xb0
 [<ffffffff81081470>] ? request_microcode_user+0x20/0x20
 [<ffffffff8108142f>] request_microcode_fw+0x7f/0xa0
 [<ffffffff813356ab>] ? kobject_uevent+0xb/0x10
 [<ffffffff81081004>] microcode_init_cpu+0xf4/0x110
 [<ffffffff816f6b8c>] mc_cpu_callback+0x5b/0xb3
 [<ffffffff81706d7c>] notifier_call_chain+0x5c/0x120
 [<ffffffff810c6359>] __raw_notifier_call_chain+0x9/0x10
 [<ffffffff8109616b>] __cpu_notify+0x1b/0x30
 [<ffffffff816f72e1>] _cpu_up+0x103/0x14b
 [<ffffffff816f7404>] cpu_up+0xdb/0xee
 [<ffffffff816eda0a>] store_online+0xba/0x120
 [<ffffffff8145f08b>] dev_attr_store+0x1b/0x20
 [<ffffffff81246591>] sysfs_write_file+0xe1/0x160
 [<ffffffff811ca3ef>] vfs_write+0xdf/0x190
 [<ffffffff811ca96d>] SyS_write+0x5d/0xa0
 [<ffffffff8133f4fe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff8170b7a9>] system_call_fastpath+0x16/0x1b

Thoughts?

Konrad Rzeszutek Wilk

author

2013-05-07 19:00 UTC #1 lore raw

Quoted 9 lines · Konrad Rzeszutek Wilk

Hey,
As I was fixing the PV HVM's broken CPU hotplug mechanism I discovered a deadlock in the microcode and generic code.
I am not sure if the ACPI generic mechanism would expose this, but looking at the flow (arch_register_cpu, then expecting user-space to call cpu_up), it should trigger this.

Fenghua,

I dug deeper in how QEMU does it and it looks to be actually doing the right thing. It triggers the ACPI SCI, the method that figures out the CPU online/offline bits kicks off the right OSPM notification and everything is going through ACPI (so _STA is on the processor is checked, returns 0x2 (ACPI_STA_DEVICE_PRESENT), MADT has now the CPU marked as enabled).

I am now 99% sure you would be able to reproduce this on baremetal with ACPI hotplug where the CPUs at bootup are marked as disabled in MADT. (lapic->lapic_flags == 0).

The comment for calling save_mc_for_early says:

/*
* If early loading microcode is supported, save this mc into
* permanent memory. So it will be loaded early when a CPU is hot added
* or resumes.
*/

Do you by any chance recall the testing you did on that? And how the ACPI CPU hotplug mechanism worked such that this deadlock would not have occured?

Thanks.

Quoted 100 lines

Anyhow, this can easily be triggered if a new CPU is added and from user-space do:

echo 1 > /sys/devices/system/cpu/cpu3/online

And here is that kernel thinks of it:

smpboot: Stack at about ffff880075c39f44
smpboot: CPU3: has booted.
microcode: CPU3 sig=0x206a7, pf=0x2, revision=0x25

but task is already holding lock:
(x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock(x86_cpu_hotplug_driver_mutex);
lock(x86_cpu_hotplug_driver_mutex);

*** DEADLOCK ***

May be due to missing lock nesting notation

6 locks held by sh/2487:

 #0:  (sb_writers#5){.+.+.+}, at: [<ffffffff811ca48d>] vfs_write+0x17d/0x190
 #1:  (&buffer->mutex){+.+.+.}, at: [<ffffffff812464ef>] sysfs_write_file+0x3f/0x160
 #2:  (s_active#20){.+.+.+}, at: [<ffffffff81246578>] sysfs_write_file+0xc8/0x160
 #3:  (x86_cpu_hotplug_driver_mutex){+.+.+.}, at: [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
 #4:  (cpu_add_remove_lock){+.+.+.}, at: [<ffffffff810961c2>] cpu_maps_update_begin+0x12/0x20
 #5:  (cpu_hotplug.lock){+.+.+.}, at: [<ffffffff810962a7>] cpu_hotplug_begin+0x27/0x60

stack backtrace:
CPU: 1 PID: 2487 Comm: sh Not tainted 3.9.0upstream-10129-g167af0e #1
Hardware name: Xen HVM domU, BIOS 4.3-unstable 05/03/2013
 ffffffff8229b710 ffff880064e75538 ffffffff816fe47e ffff880064e75608
 ffffffff81100cb6 ffff880064e75560 ffff88006670e290 ffff880064e75588
 ffffffff00000000 ffff88006670e9b8 21710c800c83a1f5 ffff880064e75598
Call Trace:
 [<ffffffff816fe47e>] dump_stack+0x19/0x1b
 [<ffffffff81100cb6>] __lock_acquire+0x726/0x1890
 [<ffffffff8110b352>] ? is_module_text_address+0x22/0x40
 [<ffffffff810bbff8>] ? __kernel_text_address+0x58/0x80
 [<ffffffff81101eca>] lock_acquire+0xaa/0x190
 [<ffffffff81075512>] ? cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff816fee1e>] __mutex_lock_common+0x5e/0x450
 [<ffffffff81075512>] ? cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff810d4225>] ? sched_clock_local+0x25/0x90
 [<ffffffff81075512>] ? cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff816ff340>] mutex_lock_nested+0x40/0x50
 [<ffffffff81075512>] cpu_hotplug_driver_lock+0x12/0x20
 [<ffffffff81080757>] save_mc_for_early+0x27/0xf0
 [<ffffffff810ffd30>] ? mark_held_locks+0x90/0x150
 [<ffffffff81176a5d>] ? get_page_from_freelist+0x46d/0x8e0
 [<ffffffff8110029d>] ? trace_hardirqs_on+0xd/0x10
 [<ffffffff81056a39>] ? sched_clock+0x9/0x10
 [<ffffffff81177275>] ? __alloc_pages_nodemask+0x165/0xa30
 [<ffffffff810d4225>] ? sched_clock_local+0x25/0x90
 [<ffffffff810d4348>] ? sched_clock_cpu+0xb8/0x110
 [<ffffffff810fb88d>] ? trace_hardirqs_off+0xd/0x10
 [<ffffffff811a5e79>] ? vmap_page_range_noflush+0x279/0x370
 [<ffffffff811a5f9d>] ? map_vm_area+0x2d/0x50
 [<ffffffff811a7dce>] ? __vmalloc_node_range+0x18e/0x260
 [<ffffffff810812a8>] ? generic_load_microcode+0xb8/0x1c0
 [<ffffffff8108135c>] generic_load_microcode+0x16c/0x1c0
 [<ffffffff8110917e>] ? generic_exec_single+0x7e/0xb0
 [<ffffffff81081470>] ? request_microcode_user+0x20/0x20
 [<ffffffff8108142f>] request_microcode_fw+0x7f/0xa0
 [<ffffffff813356ab>] ? kobject_uevent+0xb/0x10
 [<ffffffff81081004>] microcode_init_cpu+0xf4/0x110
 [<ffffffff816f6b8c>] mc_cpu_callback+0x5b/0xb3
 [<ffffffff81706d7c>] notifier_call_chain+0x5c/0x120
 [<ffffffff810c6359>] __raw_notifier_call_chain+0x9/0x10
 [<ffffffff8109616b>] __cpu_notify+0x1b/0x30
 [<ffffffff816f72e1>] _cpu_up+0x103/0x14b
 [<ffffffff816f7404>] cpu_up+0xdb/0xee
 [<ffffffff816eda0a>] store_online+0xba/0x120
 [<ffffffff8145f08b>] dev_attr_store+0x1b/0x20
 [<ffffffff81246591>] sysfs_write_file+0xe1/0x160
 [<ffffffff811ca3ef>] vfs_write+0xdf/0x190
 [<ffffffff811ca96d>] SyS_write+0x5d/0xa0
 [<ffffffff8133f4fe>] ? trace_hardirqs_on_thunk+0x3a/0x3f
 [<ffffffff8170b7a9>] system_call_fastpath+0x16/0x1b

Thoughts?

Borislav Petkov

2013-05-08 12:53 UTC #2 lore raw

Quoted 6 lines · Konrad Rzeszutek Wilk

I dug deeper in how QEMU does it and it looks to be actually doing the right thing. It triggers the ACPI SCI, the method that figures out the CPU online/offline bits kicks off the right OSPM notification and everything is going through ACPI (so _STA is on the processor is checked, returns 0x2 (ACPI_STA_DEVICE_PRESENT), MADT has now the CPU marked as enabled).

AFAIUC, you mean physical hotplug here which is done with ACPI, right? And, if so, we actually need an x86 machine which supports that to test it on. Also, is this how physical hotplug is done? Put the number of max. supported CPUs in MADT and those which are not present are marked as disabled?

Then, when they're physically hotplugged, ACPI marks them as enabled? Questions over questions...?

Quoted 5 lines

I am now 99% sure you would be able to reproduce this on baremetal with ACPI hotplug where the CPUs at bootup are marked as disabled in MADT. (lapic->lapic_flags == 0).
The comment for calling save_mc_for_early says:

Looks like save_mc_for_early would need another, local mutex to fix that.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Konrad Rzeszutek Wilk

author

2013-05-08 14:03 UTC #3 lore raw

Quoted 9 lines · Borislav Petkov

Quoted 6 lines · Konrad Rzeszutek Wilk
I dug deeper in how QEMU does it and it looks to be actually doing the right thing. It triggers the ACPI SCI, the method that figures out the CPU online/offline bits kicks off the right OSPM notification and everything is going through ACPI (so _STA is on the processor is checked, returns 0x2 (ACPI_STA_DEVICE_PRESENT), MADT has now the CPU marked as enabled).
AFAIUC, you mean physical hotplug here which is done with ACPI, right?

Yes.

Quoted 6 lines

And, if so, we actually need an x86 machine which supports that to test it on. Also, is this how physical hotplug is done? Put the number of max. supported CPUs in MADT and those which are not present are marked as disabled?
Then, when they're physically hotplugged, ACPI marks them as enabled?

Yes. The GPE is raised (by QEMU), the ACPI Method PRSC is invoked:

Scope (_GPE)

    {                                                                           
        Method (_L02, 0, NotSerialized)                                         
        {                                                                       
            Return (\_SB.PRSC ())                                               
        }                                                                       
    }

Which iterates over the AXF00 (32 bytes) and checks each bit to see if it is enabled (so CPU is on) or disabled. Then if it is different from the MADT.FLG entry (so the 'flags' entry in the MADT), it updates the MADT entry to have one (or zero if it has been disabled). And then Notifies the Processor. Here is what the Processor entry looks like:

Processor (PR02, 0x02, 0x0000B010, 0x06)

        {                                                                       
            Name (_HID, "ACPI0007")                                             
            OperationRegion (MATR, SystemMemory, Add (MAPA, 0x10), 0x08)

[MAPA is the physical address to the MADT, the 0x10 increases by eight bytes for each CPU]

            Field (MATR, ByteAcc, NoLock, Preserve)                             
            {                                                                   
                MAT,    64                                                      
            }

[so it is 64 bits, the MAT is used in the '_STA' method to return the whole contents of said memory location]

            Field (MATR, ByteAcc, NoLock, Preserve)                             
            {                                                                   
                        Offset (0x04),                                          
                FLG,    1                                                       
            }

[and FLG is at offset 4 (out of 8 bytes), which means it lands on the lapic->flags entry]

The PRSC method does what I mentioned above.

        OperationRegion (PRST, SystemIO, 0xAF00, 0x20)                          
        Field (PRST, ByteAcc, NoLock, Preserve)                                 
        {                                                                       
            PRS,    15                                                          
        }                                                                       
                                                                                
        Method (PRSC, 0, NotSerialized)                                         
        {                                                                       
            Store (ToBuffer (PRS), Local0)

[Local0 has now the 32 bytes of data]

Store (DerefOf (Index (Local0, Zero)), Local1)

[Local1 has now the zero-th byte of the 32-bytes. Each bit is one CPU, so it contains the value of eight CPUs]

And (Local1, One, Local2)

[Local2 = gpe_state.cpu_sts[i] & 1, aka first CPU]

            If (LNotEqual (Local2, ^PR00.FLG))                                  
            {                                                                   
                Store (Local2, ^PR00.FLG)

[Write the bit in the PR00.FLG, so at offset four in the MADT]

If (LEqual (Local2, One))
{
[If it was enabled, and now is disabled, then notify with 1]
Notify (PR00, One)
Subtract (MSU, One, MSU)
[fix up the checksum]

                }                                                               
                Else                                                            
                {                                                               
                    Notify (PR00, 0x03)

[if it was disabled, and now enabled, then notify with 0x3]
Add (MSU, One, MSU)
[again, fix up the checksum]

                }                                                               
            }                                                                   
                                                                                
            ShiftRight (Local1, One, Local1)

[here it shifts and continues on testing each CPU bit]

Quoted 1 line

Questions over questions...?

I probably went overboard with my answers :-)

Quoted 8 lines

Quoted 6 lines
I am now 99% sure you would be able to reproduce this on baremetal with ACPI hotplug where the CPUs at bootup are marked as disabled in MADT. (lapic->lapic_flags == 0).
The comment for calling save_mc_for_early says:
Looks like save_mc_for_early would need another, local mutex to fix that.

Let me try that. Thanks for the suggestion.

Quoted 7 lines

Signature · 6 lines

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Borislav Petkov

2013-05-08 14:29 UTC #4 lore raw

On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:

[ … snip some funky BIOS code ]

Quoted 5 lines

[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?
I probably went overboard with my answers :-)

Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)

I think I know who I can sling BIOS issues now to.

Quoted 3 lines

Quoted 1 line
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.

Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.

About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?

All I'm saying is, let's not hurry too much before we actually can really trigger this on baremetal.

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

H. Peter Anvin

2013-05-08 15:20 UTC #5 lore raw

Quoted 8 lines · Borislav Petkov

On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 4 lines
[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?

Quoted 8 lines

Quoted 1 line
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.

Nah, that was just the ACPI AML. Real BIOS code is much scarier.

-hpa

Ross Philipson

2013-05-08 15:41 UTC #6 lore raw

In reply to Borislav Petkov

Quoted 32 lines

-----Original Message-----
From: [email protected] [mailto:xen-devel- [email protected]] On Behalf Of Borislav Petkov
Sent: Wednesday, May 08, 2013 7:30 AM
To: Konrad Rzeszutek Wilk
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [Xen-devel] v3.9 - CPU hotplug and microcode earlier loading hits a mutex deadlock (x86_cpu_hotplug_driver_mutex)
On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 5 lines
[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.
Quoted 1 line
Quoted 1 line
Looks like save_mc_for_early would need another, local mutex to fix
that.
Quoted 2 lines
Let me try that. Thanks for the suggestion.

Quoted 8 lines

Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.
About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?

The ASL Konrad traced through is to make the ACPI bits a guest interoperate with qemu etc. On bare metal it will be much more opaque and ugly; the processor related AML will probably burrow down into an SMI to do the real work.

Ross

Quoted 17 lines

All I'm saying is, let's not hurry too much before we actually can really trigger this on baremetal.
Thanks.
--
Regards/Gruss,
Boris.
Sent from a fat crate under my desk. Formatting is fine. --
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xen.org/xen-devel

Konrad Rzeszutek Wilk

author

2013-05-08 16:32 UTC #7 lore raw

In reply to Borislav Petkov

Quoted 8 lines · Borislav Petkov

On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 4 lines
[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?

Quoted 7 lines

Quoted 1 line
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.

Great .. :-)

Quoted 5 lines

Quoted 4 lines
Quoted 2 lines
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.

Quoted 8 lines

Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.
About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?

Unfortunatly not. I am getting an IvyTown box so hopefully that has this support. But I thought that Fenghua did since he mentioned in the patch.

Besides that I think this can also appear on VMWare if one is doing CPU hotplug and on some HP machines - let me CC the relevant people extracted from drivers/acpi/processor_driver.c.
(see https://lkml.org/lkml/2013/5/7/588 for the thread)

Rafael, do you know of any specific Intel hardware that has this implemented?

Quoted 12 lines

All I'm saying is, let's not hurry too much before we actually can really trigger this on baremetal.
Thanks.
Signature · 5 lines
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Toshi Kani

2013-05-08 18:19 UTC #8 lore raw

Quoted 32 lines · Konrad Rzeszutek Wilk

Quoted 8 lines · Borislav Petkov
On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 4 lines
[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?
Quoted 7 lines
Quoted 1 line
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.
Great .. :-)
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.
Quoted 8 lines
Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.
About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?

Quoted 7 lines

Unfortunatly not. I am getting an IvyTown box so hopefully that has this support. But I thought that Fenghua did since he mentioned in the patch.
Besides that I think this can also appear on VMWare if one is doing CPU hotplug and on some HP machines - let me CC the relevant people extracted from drivers/acpi/processor_driver.c.
(see https://lkml.org/lkml/2013/5/7/588 for the thread)

From the stack trace, it looks like a bug in save_mc_for_early(). This function may not call cpu_hotplug_driver_lock() during CPU online. I suppose it intends to protect from CPU offline when microcode is updated outside of the boot/CPU online context. If it indeed supports updating the microcode without using reboot/cpu hotplug, the lock should be held when such update request is made.

Thanks,
-Toshi

Konrad Rzeszutek Wilk

author

2013-05-08 18:42 UTC #9 lore raw

Quoted 41 lines · Toshi Kani

Quoted 39 lines · Konrad Rzeszutek Wilk
Quoted 8 lines · Borislav Petkov
On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 4 lines
[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?
Quoted 7 lines
Quoted 1 line
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.
Great .. :-)
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.
Quoted 8 lines
Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.
About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?
Unfortunatly not. I am getting an IvyTown box so hopefully that has this support. But I thought that Fenghua did since he mentioned in the patch.
Besides that I think this can also appear on VMWare if one is doing CPU hotplug and on some HP machines - let me CC the relevant people extracted from drivers/acpi/processor_driver.c.
(see https://lkml.org/lkml/2013/5/7/588 for the thread)

Quoted 6 lines

Quoted 1 line
From the stack trace, it looks like a bug in save_mc_for_early(). This
function may not call cpu_hotplug_driver_lock() during CPU online. I suppose it intends to protect from CPU offline when microcode is updated outside of the boot/CPU online context. If it indeed supports updating the microcode without using reboot/cpu hotplug, the lock should be held when such update request is made.

Hey Toshi,

The fix for this I have posted, but I am more curious whether you have seen on baremetal this issue? Meaning using CPU ACPI hotplug on v3.9?

Quoted 4 lines

Thanks,
-Toshi

Toshi Kani

2013-05-08 18:59 UTC #10 lore raw

Quoted 50 lines · Konrad Rzeszutek Wilk

Quoted 41 lines · Toshi Kani
Quoted 39 lines · Konrad Rzeszutek Wilk
Quoted 8 lines · Borislav Petkov
On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 4 lines
[here it shifts and continues on testing each CPU bit]
Quoted 1 line
Questions over questions...?
Quoted 7 lines
Quoted 1 line
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.
Great .. :-)
Quoted 5 lines
Quoted 4 lines
Quoted 2 lines
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.
Quoted 8 lines
Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.
About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?
Unfortunatly not. I am getting an IvyTown box so hopefully that has this support. But I thought that Fenghua did since he mentioned in the patch.
Besides that I think this can also appear on VMWare if one is doing CPU hotplug and on some HP machines - let me CC the relevant people extracted from drivers/acpi/processor_driver.c.
(see https://lkml.org/lkml/2013/5/7/588 for the thread)
Quoted 6 lines
Quoted 1 line
From the stack trace, it looks like a bug in save_mc_for_early(). This
function may not call cpu_hotplug_driver_lock() during CPU online. I suppose it intends to protect from CPU offline when microcode is updated outside of the boot/CPU online context. If it indeed supports updating the microcode without using reboot/cpu hotplug, the lock should be held when such update request is made.

Quoted 4 lines

Hey Toshi,
The fix for this I have posted, but I am more curious whether you have seen on baremetal this issue? Meaning using CPU ACPI hotplug on v3.9?

Oh, I see. No, I do not have a beremetal test machine that supports CPU hotplug yet. My initial target is to support CPU hotplug on VMs. I did not hit this issue since my test env was limited to CPU hot-delete followed by CPU hot-add. In such case, uci->valid is still set during CPU hot-add and does not get into this code path. I am not sure if uci->valid is supposed to be set after CPU hot-delete, though.

Thanks,
-Toshi

Rafael J. Wysocki

2013-05-09 00:23 UTC #11 lore raw

In reply to Konrad Rzeszutek Wilk

Quoted 28 lines · Konrad Rzeszutek Wilk

Quoted 6 lines · Borislav Petkov
On Wed, May 08, 2013 at 10:03:42AM -0400, Konrad Rzeszutek Wilk wrote:
[ … snip some funky BIOS code ]
Quoted 2 lines
[here it shifts and continues on testing each CPU bit]
Quoted 7 lines
Quoted 2 lines
Quoted 1 line
Questions over questions...?
I probably went overboard with my answers :-)
Konrad, you're killing me! :-) You actually went and looked at the BIOS disassembly voluntarily. You must be insane, I think you should immediately go to the doctor now for a thorough checkup. :-)
I think I know who I can sling BIOS issues now to.
Great .. :-)
Quoted 10 lines
Quoted 2 lines
Quoted 1 line
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.
Ok, seriously now: yeah, this was just an idea, it should at least get the nesting out of the way.
About the BIOS deal: you're probably staring at some BIOS out there but is this the way that it is actually going to be implemented on the physical hotplug BIOS? I mean, I've only heard rumors about IVB supporting physical hotplug but do you even have access to such BIOS to verify?
Unfortunatly not. I am getting an IvyTown box so hopefully that has this support. But I thought that Fenghua did since he mentioned in the patch.

Quoted 6 lines

Besides that I think this can also appear on VMWare if one is doing CPU hotplug and on some HP machines - let me CC the relevant people extracted from drivers/acpi/processor_driver.c.
(see https://lkml.org/lkml/2013/5/7/588 for the thread)
Rafael, do you know of any specific Intel hardware that has this implemented?

No, I don't have this information.

By the way, here's a general request.

Please CC any messages having the word "ACPI" anywhere in the body or subject to [email protected]. I'm really tired of chasing ACPI-related threads on the LKML just because people don't care about CCing relevant lists and I'm going to NAK all patches touching ACPI in any way if they haven't been CCed to the ACPI list. Seriously.

This particular item may or may not conflict with some ongoing work taking place in there (and patches present in my tree).

And you've already had problems with ACPI-related code conflicts, so that's not like it's never happened.

Thanks,
Rafael

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
z siedziba w Gdansku
ul. Slowackiego 173
80-298 Gdansk

Sad Rejonowy Gdansk Polnoc w Gdansku, VII Wydzial Gospodarczy Krajowego Rejestru Sadowego, numer KRS 101882

NIP 957-07-52-316
Kapital zakladowy 200.000 zl

This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.

Konrad Rzeszutek Wilk

author

2013-05-08 16:13 UTC #12 lore raw

In reply to Konrad Rzeszutek Wilk

Quoted 6 lines

Quoted 4 lines
Quoted 2 lines
The comment for calling save_mc_for_early says:
Looks like save_mc_for_early would need another, local mutex to fix that.
Let me try that. Thanks for the suggestion.

Looks to work nicely. Just posted a patch for this.

Quoted 7 lines

Signature · 6 lines

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--