The remaining checks are not sufficient. If you look at the bottom of csr.c, you'll see that for most of the M-mode CSRs the predicate is set to "any" which unconditionally allows access regardless of privilege mode. The S-mode CSR predicates similarly only check that supervisor mode exists, but not that the processor is current running in that mode. I do agree with you that using the register address to determine the required privilege mode is a little strange, but RISC-V is designed so that bits 8 and 9 of the CSR address encode that information: 0=U-mode, 1=S-mode, 2=HS-mode, 3=M-mode.
I also tested by booting RVirt <https://github.com/mit-pdos/RVirt> with a Linux guest and found that this patch caused it to instantly crash because guest CSR accesses weren't intercepted. Jonathan On Fri, Jul 26, 2019 at 4:28 PM Alistair Francis <alistai...@gmail.com> wrote: > On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <finte...@gmail.com> > wrote: > > > > Unless I'm missing something, this is the only place that QEMU checks > the privilege level for read and writes to CSRs. The exact computation used > here won't work with the hypervisor extension, but we also can't just get > rid of privilege checking entirely... > > The csr_ops struct contains a checker function, so there are still > some checks occurring. I haven't done negative testing on this patch, > but the current check doesn't seem to make any sense so it should be > removed. We can separately discuss adding more checks but this current > way base don CSR address just seems strange. > > Alistair > > > > > Jonathan > > > > On Thu, Jul 25, 2019 at 2:56 PM Alistair Francis < > alistair.fran...@wdc.com> wrote: > >> > >> The privledge check based on the CSR address mask 0x300 doesn't work > >> when using Hypervisor extensions so remove the check > >> > >> Signed-off-by: Alistair Francis <alistair.fran...@wdc.com> > >> --- > >> target/riscv/csr.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/target/riscv/csr.c b/target/riscv/csr.c > >> index e0d4586760..af3b762c8b 100644 > >> --- a/target/riscv/csr.c > >> +++ b/target/riscv/csr.c > >> @@ -797,9 +797,8 @@ int riscv_csrrw(CPURISCVState *env, int csrno, > target_ulong *ret_value, > >> > >> /* check privileges and return -1 if check fails */ > >> #if !defined(CONFIG_USER_ONLY) > >> - int csr_priv = get_field(csrno, 0x300); > >> int read_only = get_field(csrno, 0xC00) == 3; > >> - if ((write_mask && read_only) || (env->priv < csr_priv)) { > >> + if (write_mask && read_only) { > >> return -1; > >> } > >> #endif > >> -- > >> 2.22.0 > >> > >> >
