[coyotos-dev] SMP Issue: load correct kernel stack on trap

[Jeroen Visser]

On 10/22/07, Jonathan S. Shapiro <shap at eros-os.com> wrote:
> I think you were misled by a bunch of stale comments and
> documentation.

Probably :)

> Looking at the patch was very useful to me, so I don't want you to
> be discouraged, but we are not going to be able to use this patch directly.

I did not expect that it would be applicable immediately. I took many
shortcuts to get those first processes running. I merely made it
available for others to play with. The AP-boot trampoline should be
mostly alright though though it requires some cleanup.

> 0. Several parts of the patch are cosmetic changes or (in a few cases)
> temporary development changes. These should be separated or removed.

I hope these can be accepted piece-meal for commit.

> 1. Your code finds the current CPU from the current process. In the
> interrupt handler this is necessary, but in C code it is not the best
> way. We already had a mechanism for this. See target-hal/cpu.h and the
> revised definition of MY_CPU. We chose to get the current process from
> the current CPU.

I assume that when we have a correct per-CPU-stack established we can
simply get MY_CPU via its address on the bottom of the per-CPU stack?

> We can change this if there is a reason to do it, but we thought this
> approach would probably work better.

I had not particular reason other than already having figured out how
to get from curProc to CPU to kstack in the interrupt handler.

> 2. We chose to store the LEAST address of the stack in the CPU structure
> because that is where the current CPU address needs to go. The top of
> stack can be computed from cpu->stack+KSTACK_SIZE. You should only need
> that when you are initializing stack for the APs.  Oh. Now I see why you
> did it. There may be a better way, but that was certainly reasonable.

Looks like you changed this in tip.

> The addition of KSTACK_MASK and KSTACK_SIZE in asm-offsets.c should not
> have been necessary -- you should have been able to use them directly
> from config.h.

Ok. I got errors when accessing KSTACK_SIZE this way. I thought
because KSTACK_SIZE is defined as a C multiplication. In the interest
of expediency I added it to asm-offsets.c which allowed me to get on
with the rest.

> 3. We decided to move all CPU-local data into the CPU structure. The
> "percpu" region in ldscript is now empty, and will go away as we clean
> up the SMP implementation.

Aha!

> 4. We intend for all processors to have the same private page table and
> private page directory, so there is no need to allocate per-AP tables in
> hwmap.c. Is there a reason you did this that we may not have noticed?

Because of me not being aware of point 3 above? Having a shared kernel
map structures will clean up my patch significantly. In particular I
no longer will have to contemplate updating cpu_ncpu maps when we grow
the kernel heap.

> 5. We definitely care about SMP on non-PAE machines (for efficiency
> reasons).

Ok.

> 6. I have not explored your code enough to understand this, but we
> definitely want a way to force SMP machines to operating with only one
> CPU. Does your patch preserve a way to do this?

If you force cpu_ncpu to 1 everything will work just fine without
starting any APs.

-JCV