On Thu, 31 Jul 2008 23:39:26 +1000 Michael Ellerman <michael@ellerman.id.au=

wrote:

On Thu, 2008-07-31 at 15:26 +0200, Sebastien Dugue wrote:

quoted

On Thu, 31 Jul 2008 23:01:39 +1000 Michael Ellerman <michael@ellerman.i=

d.au> wrote:

quoted

=20

quoted

On Thu, 2008-07-31 at 22:58 +1000, Michael Ellerman wrote:

quoted

On Thu, 2008-07-31 at 14:00 +0200, Sebastien Dugue wrote:

quoted

On Thu, 31 Jul 2008 21:40:56 +1000 Michael Ellerman <michael@elle=

rman.id.au> wrote:

quoted

quoted

quoted

quoted

quoted

=20
This boot ordering stuff is pretty hairy, so I might have missed
something, but this is how the code is ordered AFAICT:
=EF=BB=BF
start_kernel()
	init_IRQ()
	...
	local_irq_enable()
	...
	rest_init()
		kernel_thread()
			kernel_init()
				smp_prepare_cpus()
					smp_xics_probe()	(via smp_ops->probe())
=20
=20
What's stopping us from taking an irq between local_irq_enable(=

) and

quoted

quoted

quoted

quoted

quoted

smp_xics_probe() ?  Is it just that no one's request_irq()'ed t=

hem yet?

quoted

quoted

quoted

quoted

=20
  It's hairy, I agree, but as you've mentioned no one has done a =

request_irq()

quoted

quoted

quoted

quoted

at that point. The first one to do it is smp_xics_probe() for the=

 IPI.

quoted

quoted

quoted

=20
Hmm, I don't think that's strong enough. I can trivially cause irqs=

 to

quoted

quoted

quoted

fire during a kexec reboot just by mashing the keyboard.
=20
And during a kdump boot all sorts of stuff could be firing. Even du=

ring

quoted

quoted

quoted

a clean boot, from firmware, I don't think we can guarantee that
nothing's going to fire.
=20
.. after a bit of testing ..
=20
It seems it actually works (sort of).=20
=20
xics_remap_irq() calls irq_radix_revmap_lookup(), which calls:
=20
ptr =3D radix_tree_lookup(&host->revmap_data.tree, hwirq);
=20
And because =EF=BB=BFhost->revmap_data.tree was zalloc'ed we trip o=

n the first

quoted

quoted

quoted

check here:

=20
@#$% ctrl-enter =3D=3D send!
=20
Continuing ...
=20
void *radix_tree_lookup(struct radix_tree_root *root, unsigned long i=

ndex)

quoted

quoted

{
        unsigned int height, shift;
        struct radix_tree_node *node, **slot;
=20
        node =3D rcu_dereference(root->rnode);
        if (node =3D=3D NULL)
                return NULL;
=20
Which means =EF=BB=BFirq_radix_revmap_lookup() will return NO_IRQ, wh=

ich is cool.

quoted

=20
  Which is what I intended so that as long as no IRQ is registered we
return NO_IRQ.
=20

quoted

=20
=20
So I think it can fly, as long as we're happy that we can't reverse m=

ap

quoted

quoted

anything until smp_xics_probe() - and I think that's true, as any irq=

 we

quoted

quoted

take will be invalid.

=20
  That's true as no IRQs are registered before smp_xics_probe() and for=

 any

quoted

interrupt we might get before that, irq_radix_revmap_lookup() will retu=

rn

quoted

NO_IRQ.

=20
Cool, we agree :)=20
=20
My only worry is that we might be relying on on the particular radix
tree implementation a bit too much.

  Well maybe we could revert back to testing a flag just like we
do for host->revmap_data.tree.gfp_mask !=3D 0. Dunno.

Is it documented somewhere that
the /very/ first check is for =EF=BB=BFroot->rnode !=3D NULL, and the res=

t of the

root may be unintialised?

  Not in anything I could read except in looking at the code.

=20
And I think it needs a big fat comment in the irq code saying that it's
safe because revmap_data is zalloc'ed, and that means the radix lookup
will fail (safely).

  Yep, right. Will advertise this properly for the next round if this
remains the prefered solution.

  Thanks,

  Sebastien.