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

[Richard Henderson <richard.henderson@linaro.org>]

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:

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

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

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

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.


r~