On Sun, 27 Feb 2022 at 16:16, Amir Gonnen <amir.gon...@neuroblade.ai> wrote: > There's something I don't understand about gen_check_supervisor - > it looks like it checks CR_STATUS_U when generating code instead > of generating code that checks CR_STATUS_U.
This is OK because it is checking the value of CR_STATUS_U in the tbflags, not the one in the CPU state. Basically, when QEMU looks for a pre-existing TB it does so by looking up in a hash table where the key is (program counter, flags, cs_base). (cs_base here is named what it is for historical reasons, but it can be used for any data the target likes.) So the target code can put some parts of its CPU state into the TB flags word, and then at code-generation time it can generate code that assumes the value of that state. If we ever run the same bit of code both in supervisor and non-supervisor mode, we generate two separate TBs for it. (You can see what nios2 is putting in the flags if you look at cpu_get_tb_cpu_state() in cpu.h -- currently it just keeps the U and EH status bits there.) > > As an existing bug to be fixed by a separate patch, eret should also check > > for supervisor. > > Do you suggest I shouldn't fix this here? Why not fix this anyway? It should go in a separate patch (but you can put that patch in a v3 of this series), because it's a separate bug fix -- it is not related to support of shadow registers. We like to keep distinct changes in distinct patches (and thus git commits, because it makes things easier to code review and also means we have more information if we need to track down the reason for a change or diagnose a regression in future. thanks -- PMM
