Re: [Qemu-devel] [RESEND PATCH v8 2/4] hw: add a wrapper for registering reset handler

[Zhu Guihua]

On 06/30/2015 05:21 PM, Igor Mammedov wrote:
> On Tue, 30 Jun 2015 14:31:50 +0800
> Zhu Guihua <address@hidden> wrote:
>
> > On 06/26/2015 01:28 AM, Andreas Färber wrote:
> > > Am 25.06.2015 um 19:00 schrieb Paolo Bonzini:
> > > > On 25/06/2015 04:17, Zhu Guihua wrote:
> > > > > Add a wrapper to specify reset order when registering reset handler,
> > > > > instead of non-obvious initiazation code ordering.
> > > > >
> > > > > Signed-off-by: Zhu Guihua <address@hidden>
> > > > I'm sorry, this is not really acceptable.  The initialization code
> > > > ordering is good because it should be okay to run reset handlers in the
> > > > same order as code is run.  If there are dependencies between reset
> > > > handlers, a random integer is not a maintainable way to maintain them.
> > > >
> > > > Instead, you should have a single reset handler that calls the reset
> > > > handlers in the right order; for example a qdev bus such as icc_bus
> > > > always resets children before parents.
> > > >
> > > > Are you sure that you want to remove the icc_bus?... What are you
> > > > gaining exactly by doing so?
> > > >From my view we would be gaining by making the APIC an integral part
> > > (child<>) of the CPU in a follow-up step (there's a TODO to make things
> > > link<>s).
> > >
> > > But either way the CPU's existing reset handler should be able to handle
> > > CPU/APIC interdependencies just fine, somehow. Which is what Eduardo
> > > said on v6 and v7. (Another alternative he raised was a machine init
> > > notifier, but I see no code for that after its mention on v7?)
> > According to Eduardo's suggestions on v7, the simpler way is to add a
> > ordering parameter
> > to qemu_register_reset(), so that we can ensure the order of apic reset
> > handler(apic reset
> > must be after the other devices' reset on x86).
> >
> > This way will  not influence the initialization code ordering expect
> > apic reset.
> > Can we take this way? or someone have a better one?
> could you explain once more why apic->reset() doesn't work
> when it's called from cpu->reset(), please?

Originally, there are some devices (such as hpet, rtc) reset before apic 
reset.
When these devices reset, they would send irq to apic.
As apic reset is behind these devices reset, the apic register could be 
set to
default values.

If apic->reset() is called from cpu->reset(),  cpu reset is before some 
devices
reset, it lead to apic reset is before them too, so the apic register 
could not be set
to default values.
But before guest boots up, the irq request should be rejected. So when linux
enables local apic, it would find there are irr requests, then it will 
cause the
following warn_on.

 [    1.073487] ------------[ cut here ]------------
[    1.074019] WARNING: at arch/x86/kernel/apic/apic.c:1401 
setup_local_APIC+0x268/0x320()
[    1.075011] Modules linked in:
[    1.076474] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.10.0.sort+ #100
[    1.077012] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), 
BIOS rel-1.8.1-0-g4adadbd-20150316_085822-nilsson.home.kraxel.org 
04/01/2014
[    1.078011]  0000000000000000 00000000d1b49dbb ffff88007c787da8 
ffffffff81649983
[    1.082011]  ffff88007c787de0 ffffffff810b3241 0000000000000001 
0000000000000000
[    1.085012]  00000000000000f0 0000000000000000 00000000ffffffff 
ffff88007c787df0
[    1.088012] Call Trace:
[    1.089019]  [<ffffffff81649983>] dump_stack+0x19/0x1b
[    1.090017]  [<ffffffff810b3241>] warn_slowpath_common+0x61/0x80
[    1.091015]  [<ffffffff810b336a>] warn_slowpath_null+0x1a/0x20
[    1.092016]  [<ffffffff81089ae8>] setup_local_APIC+0x268/0x320
[    1.093019]  [<ffffffff81ad4f02>] native_smp_prepare_cpus+0x294/0x35b
[    1.094018]  [<ffffffff81ac1133>] kernel_init_freeable+0xbb/0x217
[    1.095017]  [<ffffffff81636fe0>] ? rest_init+0x80/0x80
[    1.096015]  [<ffffffff81636fee>] kernel_init+0xe/0x180
[    1.097016]  [<ffffffff816598fc>] ret_from_fork+0x7c/0xb0
[    1.098016]  [<ffffffff81636fe0>] ? rest_init+0x80/0x80
[    1.099017] ---[ end trace d99eba50bffa17c5 ]---

> > Thanks,
> > Zhu
> >
> > > Cheers,
> > > Andreas
> .