[NSWI004] [Teachers at NSWI004] Bug report

[Lubomír Bulej]

Hello,

I see that Vojtech has already replied, so I'll just add a few more bits.

There are more things at play.


TLDR:

you need to set ASID explicitly before you call `cpu_switch_context()`, 
because ASID can (depending on your implementation) change between context 
switches.

The `cp0.h` header file contains the `cp0_set_asid()` function which should be 
used in an appropriate context to set the active ASID. We forgot to explicitly 
mention this function in the assignment text, even though I would assume that 
the section on TLB management would have you look in the `cpu0.h` header file.

My apologies for that omission.


Long answer:

Switching CPU context (i.e., switching between threads) does not require 
switching address spaces, which is why `cpu_switch_context()` does not do it.

Our goal was to have a relatively straightforward implementation of 
`cpu_switch_context()` so that it's clear what it is doing (even though there 
are a few extra hoops to go through to be able to use the k0 and k1 registers).

ASID is a property of an address space and an address space is a property of a 
process. Even though you do not have an explicit process context in your 
kernel, you should treat an address space as one, i.e., deal with it 
explicitly in your thread switch code.

I can imagine having a function `as_switch_to()` which would pick the right 
ASID (depending on your ASID allocation strategy) and use the cp0_set_asid() 
function to set it.
This function would be called by the `thread_switch_to()` function so that 
thread code would not have to worry about implementation details of switching 
address space (or having to work with ASIDs), as opposed to having 
`thread_switch_to()` store the value in the thread's `context_t` before 
switching and expecting `cpu_switch_context()` to load CP0 EntryHi.


I can understand how you arrived at the conclusion that the code in 
`context.S` is buggy, even though we consider it correct. One misleading bit 
is the content of `context_t`, which indeed contains an `entryhi` member which 
is not used by the assembly code in `context.S`.

Note, however, that it also does not work with `badvaddr`, `epc`, and `cause`.

The exception handler code works with `status`, `epc`, `badvaddr`, and 
`cause`. As mentioned in `exc.h`, we keep those values in `context_t` so that 
we can reuse the structure type (and the manually defined offsets) also in 
exception handlers. The trade-off we were making was in favor of simplicity.

Done "properly", we would have different structure types for `context_t` and 
`eh_context_t` and we would generate offsets using a tool to ensure that the 
two things remain consistent. However, this proved to be confusing to students 
in previous years.


Anyway, we are left with an `entryhi` which is not used anywhere. This is the 
result of `cpu_switch_context()` simplification we were doing for the earlier 
assignments, in which we did not think of removing the `entryhi` field.
In later assignments, we generally avoid changing files from earlier 
assignments unless necessary (i.e., if the change is required by the new 
assignment, or if we discover a bug).

However, the unused `entryhi` may have led you to the conclusion that it 
should be used by `cpu_switch_context()`. Technically, your modification 
works, but is not what we intended design-wise.

Rest assured that we spend non-trivial time on preparing the assignments, but 
sometimes trivial things combine in an unexpected fashion and become 
misleading. My apologies for the confusion.


Lubomir Bulej



On 16/12/2020 00:50, Jura Pelc wrote:
> 
> 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.
> ```
> 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
> ```
>