On Tue, Nov 9, 2010 at 10:41 AM, Mike Belopuhov <[email protected]> wrote:
> On Sun, Oct 31, 2010 at 17:10 -0700, Philip Guenther wrote:
...
>> The TSS also contains the pointer to the stack used for double faults.
>> Previously, this was placed one page above each process's PCB. Rather
>> than change that on each context switch, I've just left it set to one page
>> above each CPU's idle process's PCB.
>
> i failed to find the code that does this. could you please point me to it.
>
> is it supposed to be referenced by the tss_ist[1]?
No, the double-fault stack is tss_ist[0]. That's set in
x86_64_proc0_tss_ldt_init() and x86_64_init_pcb_tss_ldt (primary vs
secondary processor) to point into the processor's idle process
uspace. It's the same place it points now, it's just that the patch
changes it to only get set once, when each processor's idle process is
created, instead of having it be a per-process value by virtue of the
tss being per-process.
> you might also want to remove disabled amd64_{get,set}_ioperm code.
Good point, though I'm inclined to do that as a separate commit.
>> Index: amd64/locore.S
>> ===================================================================
>> RCS file: /cvs/src/sys/arch/amd64/amd64/locore.S,v
>> retrieving revision 1.42
>> diff -u -p -r1.42 locore.S
>> --- amd64/locore.S 26 Oct 2010 05:49:10 -0000 1.42
>> +++ amd64/locore.S 31 Oct 2010 23:04:12 -0000
>> @@ -727,6 +727,7 @@ ENTRY(cpu_switchto)
>> pushq %r15
>>
>> movq %rdi, %r13
>> + movq %rdi, %r15
>
> why do you need to load 'r15' with 'rdi'?
> it looks like a dead store to me
Hmm, yeah, that was a debugging leftover.
>> + /* clear the old pmap's bit for the cpu */
>> + movq PCB_PMAP(%r13),%rcx
>> + lock
>> + btcl %edi,PM_CPUS(%rcx)
>> +
>
> 'btr' looks a bit more explicit to me. but that's just nitpicking.
Yeah, that's clearer.
>> + /* set the new pmap's bit for the cpu */
>> + movl CPUVAR(CPUID),%edi
>> + movq PCB_PMAP(%r13),%rcx
>> + movl PM_CPUS(%rcx),%eax
>> + movq %rax,%r14
>
> the previous line looks like a dead store to me.
Ah, that was debugging code, capturing rax before the diagnostic check
which overwrites it. It can go, yes.
(Oooh, two 'movq's that can go! Time to rerun the perf test!)
(Yes, that's a joke)
>> + lock
>> + btsl %edi,PM_CPUS(%rcx)
>> +#ifdef DIAGNOSTIC
>> + jc _C_LABEL(switch_pmcpu_set)
>
> that's a nice 'free' check, indeed.
It's kinda unfortunate that the "no longer using this pmap" btcl above
can't do the same thing, but since this code is skipped for kernel
threads, the kernel pmap isn't normally marked as used. I suppose the
DIAGNOSTIC code could have a check for whether the process being
switched _from_ was a P_SYSTEM process, but it didn't feel worth it at
that point...
>> /* exceptions */
>> for (x = 0; x < 32; x++) {
>> ist = (x == 8) ? 1 : 0;
>> + // ist = (x == 8) ? 1 : (x == 2) ? 2 : 0;
>
> why do you need this comment? or is it a reminder to have
> a separate stack for the NMI in the future?
Yep. I was thinking of doing it as part of this diff, realized I
didn't know how to test it, and so left it as a comment.
Philip Guenther
