[NSWI004] [Teachers at NSWI004] Bug report

[Vojtech Horky]

Hello.

Dne 16. 12. 20 v 0:50 Jura Pelc napsal(a):
> 
> Hello,
> 
> after nontrivial time spent scanning your assembly code, I figured out 
> that during a context switch the entryHi register is not loaded nor 
> stored in contexts.
> With your implementation of context.S, tests were failing, caused by 
> thread_kill call in global_exception_handler because memory was access 
> with an ASID unset.
> 
> I coincide it as a bug since context switch should preserve the entire 
> context_t structure but entryHi nor CONTEXT_CP0_ENTRYHI_OFFSET was not 
> used anywhere in SAVE_REGISTERS nor LOAD_REGISTERS.
> 
> I made the following changes, I want someone to concur with my findings.

Actually, for a normal (cooperative) context switch, you probably do not 
need to preserve (or touch at all) the CP0 registers. They are mostly 
related to exception handling and thus are not needed in cooperative 
scheduling.

And in preemptive scenario, their values could not be preserved as they 
were just overwritten by the exception.

Hence, for context.S we do not save these registers at all.

However, as you pointed out, you need to set ASIDs that are stored in 
EntryHi. We forgot to mention (sorry) that there is cp0_set_asid() that 
sets current ASID. We expect you to call this function just before 
thread_switch_to (consider that you would also need something like 
as_switch_to function for switching between address spaces).


The reason for this split is also that address spaces are not bound with 
a single thread, but rather with a process (subtree of threads). Thus it 
does not make that much sense to keep ASID together with thread context. 
Furthermore, the ASID may become invalid over time and needs to be 
checked (and perhaps recycled) whenever switching between address spaces.


Looking at the code again, we do not save EntryHi even into eh_context_t 
during an exception (but it is trivial to add it). We somehow assumed 
that you would use badvaddr instead of getting VPN2 from EntryHi so we 
saved only the most important registers.

Perhaps we should have made context_t and eh_context_t with only the 
really-required registers to emphasize this.

Hope this makes sense.

Cheers,
- VH



> ```
> diff --git a/kernel/src/proc/context.S b/kernel/src/proc/context.S
> index 3da3a0c..a43ddef 100644
> --- a/kernel/src/proc/context.S
> +++ b/kernel/src/proc/context.S
> @@ -42,6 +42,8 @@ cpu_switch_context:
>       mflo $t1
>       sw $t0, CONTEXT_HI_OFFSET($a0)
>       sw $t1, CONTEXT_LO_OFFSET($a0)
> +    mfc0 $t0, $10
> +    sw $t0, CONTEXT_CP0_ENTRYHI_OFFSET($a0)
> 
>       /*
>        * Save the CP0 Status register and disable interrupts (by
> @@ -71,6 +73,8 @@ cpu_switch_context:
>       lw $t1, CONTEXT_LO_OFFSET($a1)
>       mthi $t0
>       mtlo $t1
> +    lw $t0, CONTEXT_CP0_ENTRYHI_OFFSET($a1)
> +    mtc0 $t0, $10
> 
>       move $k0, $a1
>       LOAD_REGISTERS $k0
> ```
> 
> 
> -- 
> S pozdravem Jiří Pelc
> Matematicko-fyzikální fakulta
> Univerzita Karlova
>