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

[Alistair Francis]

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
>>
>>