Re: [Qemu-riscv] [PATCH-4.2 v1 2/6] target/riscv: Remove strict perm checking for CSR R/W

[Alistair Francis]

On Fri, Jul 26, 2019 at 2:00 PM Jonathan Behrens <address@hidden> wrote:
>
> 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.

Ah, I didn't realise that was guaranteed. I will drop this patch from
this series and update it in my Hypervisor series.

Alistair

>
> I also tested by booting 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 <address@hidden> wrote:
>>
>> On Thu, Jul 25, 2019 at 2:48 PM Jonathan Behrens <address@hidden> 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 <address@hidden> 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 <address@hidden>
>> >> ---
>> >>  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
>> >>
>> >>