[PATCH] Software Suspend 2 for ppc64 (1/2 - core)

[Benjamin Herrenschmidt]

> 
> Do you mean that you don't like the separation between
> __save_processor_state() and __smp_suspend_lowlevel()?...

Yup, I'm not really a fan of it.

> > It makes little sense. You assume gcc won't play
> > with registers behind your back, fairly unsafe.
> 
> yeah... gcc *did* play with the registers on me and I fixed that by 
> adding a constraint to one of the asm calls... pretty silly of me...
> 
> >>+		__restore_processor_state(suspend2_saved_contexts + 
> >>+					 _smp_processor_id());
> >>+		local_flush_tlb();
> > 
> > 
> > I'm not sure calling local_flush_tlb() here makes much sense. You need
> > to do more than just flushing the current batch here. 
> 
> Yeah, I'm aware... it's just that the thing worked before I started 
> playing with this function (which I copied from i386), so I went ahead 
> and released it to get people to play with it...

Ok, you are probably just getting lucky that things are similar between
the "loader" kernel and the resumed kernel :) You need to be more
careful about it though to be really reliable and you certainly want to
force restoring the SLB

> > You actually need
> > to invalidate the entire TLB which is not necessarily easy,
> 
> why is it "not necessarily easy"?... isn't executing a tlbia enough?

tlbia is not implemented on most CPUs

> > and you need to take care of the SLB as well.
> 
> yep...
> 
> >>+		/* 
> >>+		 *Save context and go back to idling.
> >>+		 * Note that we cannot leave the processor
> >>+		 * here. It must be able to receive IPIs if
> >>+		 * the LZF compression driver (eg) does a
> >>+		 * vfree after compressing the kernel etc
> >>+		 */
> > Gack ? Can you explain the above comment a bit more ? Something is
> > playing with kernel virtual space while CPUs are locked into IPIs and/or
> > that kind of horror ? Doesn't seem like a very sane thing to do.
> 
> Sorry, I can't :)... This was copied from i386 with comments and all...

Hrmph... I'll have to poke Nigel then.

> >>+static inline void move_stack_to_nonconflicing_area(void)
> >>+{
> >>+	unsigned long old_stack, src;
> ...
> > In what context is the above called ? You are likely to die an horrible
> > death when moving the stack around if you take an SLB miss at the wrong
> > time. The kernel is careful about always locking the current kernel
> > stack segment in SLB, various bits make assumption that remains true at
> > all time.
> 
> Now that I realize, this function is not called by the latest ssusp2
> core (I did the development on an earlier version)... so i guess it's a
> moot point...

Ok, good.

> > In general, you seem to completely ignore the hash table and SLB. That
> > might work by luck, because your "loader" kernel is the same as your
> > "saved" kernel, you'll eventually end up with proper bolted down hash
> > entries, but it's very dodgy. 
> 
> yeah I know it is luck that it works without messing with the htab and 
> slb... I'll fix that...
> 
> > You are also ignoring the iommu,
> Well... when the drivers suspend, they should unmap all their dmaable 
> buffers and structs

No, not necessarily. At least it hasn't been a requirement so far... Oh,
and you may need to work on the interrupt controller too.

> ... when they resume, they have to map them, so I'm 
> not sure if it is necessary to do anything else the iommu...
> 
> > , and RTAs,
> yeah, I'll make sure that RTAS gets instantiated again...

Well, it does get instanciated by the loader kernel, you just have to be
extra careful about -where- it gets instanciated.... Or just maybe
save/restore rtas instance along with the kernel... not sure if that
works though, a bit gross but might be ok.

Ben.