On Fri, Jul 26, 2019 at 2:00 PM Jonathan Behrens <finte...@gmail.com> 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 <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
>> >>
>> >>
