Re: [PATCH for 5.0 v1 1/2] riscv: Don't use stage-2 PTE lookup protection flags

[Alistair Francis]

On Thu, Mar 26, 2020 at 4:50 PM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/26/20 3:44 PM, Alistair Francis wrote:
> > When doing the fist of a two stage lookup (Hypervisor extensions) don't
> > set the current protection flags from the second stage lookup of the
> > base address PTE.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  target/riscv/cpu_helper.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index d3ba9efb02..f36d184b7b 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -452,10 +452,11 @@ restart:
> >          hwaddr pte_addr;
> >
> >          if (two_stage && first_stage) {
> > +            int vbase_prot;
> >              hwaddr vbase;
> >
> >              /* Do the second stage translation on the base PTE address. */
> > -            get_physical_address(env, &vbase, prot, base, access_type,
> > +            get_physical_address(env, &vbase, &vbase_prot, base, 
> > access_type,
> >                                   mmu_idx, false, true);
> >
> >              pte_addr = vbase + idx * ptesize;
> >
>
> Certainly stage2 pte lookup has nothing to do with the original lookup, so
> using a new variable for prot is correct.
>
> So as far as this minimal patch,
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>
> However, this bit of code doesn't look right:

Thanks for the comments here. Coming back to this after a while.

>
> (1) Similarly, what has the original access_type got to do with the PTE 
> lookup?
>  Seems like this should be MMU_DATA_LOAD always.

Fixed in master now

>
> (2) Why is the get_physical_address return value ignored?  On failure, surely
> this should be some sort of PTE lookup failure.

Also fixed in master now

>
> (3) Do we need to validate vbase_prot for write before updating the PTE for
> Access or Dirty?  That seems like a loop-hole to allow silent modification of
> hypervisor read-only memory.

That's a good point.

Updating the accessed bit seems correct to me as we did access it and
that doesn't then provide write permissions.

Updating the dirty bit would provide write permissions, but we would
only change the dirty bit on a store and vbase_prot is now always a
load.

If the PTE was already dirty then we might incorrectly provide write
access though.

>
> I do wonder if it might be easier to manage all of this by using additional
> TLBs to handle the stage2 and physical address spaces.  That's probably too
> invasive for this stage of development though.

Do you mean change riscv_cpu_mmu_index() to take into account
virtulisation and have more then the current 3 (M, S and U) MMU
indexes?

Alistair

>
>
> r~