Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2021-03-18 Thread Alistair Francis 
On Thu, Mar 11, 2021 at 5:32 AM Georg Kotheimer
 wrote:
>
> According to the specification the "field SPVP of hstatus controls the
> privilege level of the access" for the hypervisor virtual-machine load
> and store instructions HLV, HLVX and HSV.
>
> Signed-off-by: Georg Kotheimer 

Thanks!

Applied to riscv-to-apply.next

Alistair

> ---
>  target/riscv/cpu_helper.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 2f43939fb6..d0577b1e08 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -325,7 +325,11 @@ static int get_physical_address(CPURISCVState *env, 
> hwaddr *physical,
>  use_background = true;
>  }
>
> -if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> +/* MPRV does not affect the virtual-machine load/store
> +   instructions, HLV, HLVX, and HSV. */
> +if (riscv_cpu_two_stage_lookup(mmu_idx)) {
> +mode = get_field(env->hstatus, HSTATUS_SPVP);
> +} else if (mode == PRV_M && access_type != MMU_INST_FETCH) {
>  if (get_field(env->mstatus, MSTATUS_MPRV)) {
>  mode = get_field(env->mstatus, MSTATUS_MPP);
>  }
> @@ -695,19 +699,18 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, 
> int size,
>  qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>__func__, address, access_type, mmu_idx);
>
> -if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> -if (get_field(env->mstatus, MSTATUS_MPRV)) {
> -mode = get_field(env->mstatus, MSTATUS_MPP);
> +/* MPRV does not affect the virtual-machine load/store
> +   instructions, HLV, HLVX, and HSV. */
> +if (riscv_cpu_two_stage_lookup(mmu_idx)) {
> +mode = get_field(env->hstatus, HSTATUS_SPVP);
> +} else if (mode == PRV_M && access_type != MMU_INST_FETCH &&
> +   get_field(env->mstatus, MSTATUS_MPRV)) {
> +mode = get_field(env->mstatus, MSTATUS_MPP);
> +if (riscv_has_ext(env, RVH) && get_field(env->mstatus, MSTATUS_MPV)) 
> {
> +two_stage_lookup = true;
>  }
>  }
>
> -if (riscv_has_ext(env, RVH) && env->priv == PRV_M &&
> -access_type != MMU_INST_FETCH &&
> -get_field(env->mstatus, MSTATUS_MPRV) &&
> -get_field(env->mstatus, MSTATUS_MPV)) {
> -two_stage_lookup = true;
> -}
> -
>  if (riscv_cpu_virt_enabled(env) ||
>  ((riscv_cpu_two_stage_lookup(mmu_idx) || two_stage_lookup) &&
>   access_type != MMU_INST_FETCH)) {
> --
> 2.30.1
>
>

Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2021-03-16 Thread Alistair Francis 
On Thu, Mar 11, 2021 at 5:32 AM Georg Kotheimer
 wrote:
>
> According to the specification the "field SPVP of hstatus controls the
> privilege level of the access" for the hypervisor virtual-machine load
> and store instructions HLV, HLVX and HSV.
>
> Signed-off-by: Georg Kotheimer 

Reviewed-by: Alistair Francis 

Alistair

> ---
>  target/riscv/cpu_helper.c | 25 ++---
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 2f43939fb6..d0577b1e08 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -325,7 +325,11 @@ static int get_physical_address(CPURISCVState *env, 
> hwaddr *physical,
>  use_background = true;
>  }
>
> -if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> +/* MPRV does not affect the virtual-machine load/store
> +   instructions, HLV, HLVX, and HSV. */
> +if (riscv_cpu_two_stage_lookup(mmu_idx)) {
> +mode = get_field(env->hstatus, HSTATUS_SPVP);
> +} else if (mode == PRV_M && access_type != MMU_INST_FETCH) {
>  if (get_field(env->mstatus, MSTATUS_MPRV)) {
>  mode = get_field(env->mstatus, MSTATUS_MPP);
>  }
> @@ -695,19 +699,18 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, 
> int size,
>  qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
>__func__, address, access_type, mmu_idx);
>
> -if (mode == PRV_M && access_type != MMU_INST_FETCH) {
> -if (get_field(env->mstatus, MSTATUS_MPRV)) {
> -mode = get_field(env->mstatus, MSTATUS_MPP);
> +/* MPRV does not affect the virtual-machine load/store
> +   instructions, HLV, HLVX, and HSV. */
> +if (riscv_cpu_two_stage_lookup(mmu_idx)) {
> +mode = get_field(env->hstatus, HSTATUS_SPVP);
> +} else if (mode == PRV_M && access_type != MMU_INST_FETCH &&
> +   get_field(env->mstatus, MSTATUS_MPRV)) {
> +mode = get_field(env->mstatus, MSTATUS_MPP);
> +if (riscv_has_ext(env, RVH) && get_field(env->mstatus, MSTATUS_MPV)) 
> {
> +two_stage_lookup = true;
>  }
>  }
>
> -if (riscv_has_ext(env, RVH) && env->priv == PRV_M &&
> -access_type != MMU_INST_FETCH &&
> -get_field(env->mstatus, MSTATUS_MPRV) &&
> -get_field(env->mstatus, MSTATUS_MPV)) {
> -two_stage_lookup = true;
> -}
> -
>  if (riscv_cpu_virt_enabled(env) ||
>  ((riscv_cpu_two_stage_lookup(mmu_idx) || two_stage_lookup) &&
>   access_type != MMU_INST_FETCH)) {
> --
> 2.30.1
>
>

[PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2021-03-11 Thread Georg Kotheimer 
According to the specification the "field SPVP of hstatus controls the
privilege level of the access" for the hypervisor virtual-machine load
and store instructions HLV, HLVX and HSV.

Signed-off-by: Georg Kotheimer 
---
 target/riscv/cpu_helper.c | 25 ++---
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 2f43939fb6..d0577b1e08 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -325,7 +325,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr 
*physical,
 use_background = true;
 }
 
-if (mode == PRV_M && access_type != MMU_INST_FETCH) {
+/* MPRV does not affect the virtual-machine load/store
+   instructions, HLV, HLVX, and HSV. */
+if (riscv_cpu_two_stage_lookup(mmu_idx)) {
+mode = get_field(env->hstatus, HSTATUS_SPVP);
+} else if (mode == PRV_M && access_type != MMU_INST_FETCH) {
 if (get_field(env->mstatus, MSTATUS_MPRV)) {
 mode = get_field(env->mstatus, MSTATUS_MPP);
 }
@@ -695,19 +699,18 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
   __func__, address, access_type, mmu_idx);
 
-if (mode == PRV_M && access_type != MMU_INST_FETCH) {
-if (get_field(env->mstatus, MSTATUS_MPRV)) {
-mode = get_field(env->mstatus, MSTATUS_MPP);
+/* MPRV does not affect the virtual-machine load/store
+   instructions, HLV, HLVX, and HSV. */
+if (riscv_cpu_two_stage_lookup(mmu_idx)) {
+mode = get_field(env->hstatus, HSTATUS_SPVP);
+} else if (mode == PRV_M && access_type != MMU_INST_FETCH &&
+   get_field(env->mstatus, MSTATUS_MPRV)) {
+mode = get_field(env->mstatus, MSTATUS_MPP);
+if (riscv_has_ext(env, RVH) && get_field(env->mstatus, MSTATUS_MPV)) {
+two_stage_lookup = true;
 }
 }
 
-if (riscv_has_ext(env, RVH) && env->priv == PRV_M &&
-access_type != MMU_INST_FETCH &&
-get_field(env->mstatus, MSTATUS_MPRV) &&
-get_field(env->mstatus, MSTATUS_MPV)) {
-two_stage_lookup = true;
-}
-
 if (riscv_cpu_virt_enabled(env) ||
 ((riscv_cpu_two_stage_lookup(mmu_idx) || two_stage_lookup) &&
  access_type != MMU_INST_FETCH)) {
-- 
2.30.1

Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2020-10-22 Thread Richard Henderson 
On 10/21/20 12:21 PM, Alistair Francis wrote:
>> mmuidx, you would use cpu_lduw_mmuidx_ra.  I would also split the function 
>> into
>> two, so that one performs HLVX.HU and the other HLVX.WU, so that you don't 
>> have
>> to pass the size as a parameter.
> 
> I'm not clear what you mean here.

target_ulong helper_hyp_x_load(CPURISCVState *env,
  target_ulong address, target_ulong attrs, target_ulong memop)

should be split to

target_ulong helper_hlvx_hu(CPURISCVState *env,
target_ulong address)
target_ulong helper_hlvx_hw(CPURISCVState *env,
target_ulong address)

(attrs is already unused?) so that memop is not required.


r~

Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2020-10-21 Thread Alistair Francis 
On Sun, Oct 18, 2020 at 8:58 AM Richard Henderson
 wrote:
>
> On 10/18/20 5:03 AM, Georg Kotheimer wrote:
> > According to the specification the "field SPVP of hstatus controls the
> > privilege level of the access" for the hypervisor virtual-machine load
> > and store instructions HLV, HLVX and HSV.
> >
> > We introduce the new virtualization register field HS_HYP_LD_ST,
> > similar to HS_TWO_STAGE, which tracks whether we are currently
> > executing a hypervisor virtual-macine load or store instruction.
> >
> > Signed-off-by: Georg Kotheimer 
>
> Well, let me start by mentioning the existing implementation of hyp_load et al
> is broken.  I guess I wasn't paying attention when Alistair implemented them.
>
> Here's the problem: When you change how riscv_cpu_tlb_fill works, as you are 
> by
> modifying get_physical_address, you have to remember that those results are
> *saved* in the qemu tlb.
>
> So by setting some flags, performing one memory operation, and resetting the
> flags, we have in fact corrupted the tlb for the next operation without those
> flags.
>
> You need to either:
>
> (1) perform the memory operation *without* using the qemu tlb machinery (i.e.
> use get_physical_address directly, then use address_space_ld/st), or
>
> (2) add a new mmu index for the HLV/HSV operation, with all of the differences
> implied.
>
> The second is imo preferable, as it means that helper_hyp_load et al can go
> away and become normal qemu_ld/st opcodes with the new mmu indexes.

Ok, I have some patches to fix this. I'll send them soon.


>
> Annoyingly, for the moment you wouldn't be able to remove helper_hyp_x_load,
> because there's no qemu_ld variant that uses execute permission.  But it can 
> be
> done with helpers.  I'll note that the current implementation is broken,
> because it uses cpu_lduw_data_ra, and not cpu_lduw_code_ra, which is exactly
> the difference that uses execute permissions.  After the conversion to the new

I have tried to fix this as well, but it seems to break a virtual
Linux guest booting.

> mmuidx, you would use cpu_lduw_mmuidx_ra.  I would also split the function 
> into
> two, so that one performs HLVX.HU and the other HLVX.WU, so that you don't 
> have
> to pass the size as a parameter.

I'm not clear what you mean here.

Alistair

>
> Finally, this patch, changing behaviour when hstatus.SPVP changes... depends 
> on
> how often this bit is expected to be toggled.
>
> If the bit toggles frequently, e.g. around some small section of kernel code,
> then one might copy the SPVP bit into tlb_flags and use two different mmu
> indexes to imply the state of the SPVP bit.
>
> If the bit does not toggle frequently, e.g. whatever bit of kernel code runs
> infrequently, or it only happens around priv level changes, then simply
> flushing the qemu tlb when the SPVP bit changes is sufficient.  You then get 
> to
> look at SPVP directly within tlb_fill.
>
>
> r~
>

Re: [PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2020-10-18 Thread Richard Henderson 
On 10/18/20 5:03 AM, Georg Kotheimer wrote:
> According to the specification the "field SPVP of hstatus controls the
> privilege level of the access" for the hypervisor virtual-machine load
> and store instructions HLV, HLVX and HSV.
> 
> We introduce the new virtualization register field HS_HYP_LD_ST,
> similar to HS_TWO_STAGE, which tracks whether we are currently
> executing a hypervisor virtual-macine load or store instruction.
> 
> Signed-off-by: Georg Kotheimer 

Well, let me start by mentioning the existing implementation of hyp_load et al
is broken.  I guess I wasn't paying attention when Alistair implemented them.

Here's the problem: When you change how riscv_cpu_tlb_fill works, as you are by
modifying get_physical_address, you have to remember that those results are
*saved* in the qemu tlb.

So by setting some flags, performing one memory operation, and resetting the
flags, we have in fact corrupted the tlb for the next operation without those
flags.

You need to either:

(1) perform the memory operation *without* using the qemu tlb machinery (i.e.
use get_physical_address directly, then use address_space_ld/st), or

(2) add a new mmu index for the HLV/HSV operation, with all of the differences
implied.

The second is imo preferable, as it means that helper_hyp_load et al can go
away and become normal qemu_ld/st opcodes with the new mmu indexes.

Annoyingly, for the moment you wouldn't be able to remove helper_hyp_x_load,
because there's no qemu_ld variant that uses execute permission.  But it can be
done with helpers.  I'll note that the current implementation is broken,
because it uses cpu_lduw_data_ra, and not cpu_lduw_code_ra, which is exactly
the difference that uses execute permissions.  After the conversion to the new
mmuidx, you would use cpu_lduw_mmuidx_ra.  I would also split the function into
two, so that one performs HLVX.HU and the other HLVX.WU, so that you don't have
to pass the size as a parameter.

Finally, this patch, changing behaviour when hstatus.SPVP changes... depends on
how often this bit is expected to be toggled.

If the bit toggles frequently, e.g. around some small section of kernel code,
then one might copy the SPVP bit into tlb_flags and use two different mmu
indexes to imply the state of the SPVP bit.

If the bit does not toggle frequently, e.g. whatever bit of kernel code runs
infrequently, or it only happens around priv level changes, then simply
flushing the qemu tlb when the SPVP bit changes is sufficient.  You then get to
look at SPVP directly within tlb_fill.


r~

[PATCH] target/riscv: Adjust privilege level for HLV(X)/HSV instructions

2020-10-18 Thread Georg Kotheimer 
According to the specification the "field SPVP of hstatus controls the
privilege level of the access" for the hypervisor virtual-machine load
and store instructions HLV, HLVX and HSV.

We introduce the new virtualization register field HS_HYP_LD_ST,
similar to HS_TWO_STAGE, which tracks whether we are currently
executing a hypervisor virtual-macine load or store instruction.

Signed-off-by: Georg Kotheimer 
---
 target/riscv/cpu.h|  2 ++
 target/riscv/cpu_bits.h   |  1 +
 target/riscv/cpu_helper.c | 60 ++-
 target/riscv/op_helper.c  |  7 +
 4 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index de275782e6..a39674a84d 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -323,6 +323,8 @@ bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env);
 void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable);
 bool riscv_cpu_two_stage_lookup(CPURISCVState *env);
 void riscv_cpu_set_two_stage_lookup(CPURISCVState *env, bool enable);
+bool riscv_cpu_hyp_load_store_inst(CPURISCVState *env);
+void riscv_cpu_set_hyp_load_store_inst(CPURISCVState *env, bool enable);
 int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch);
 hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
 void  riscv_cpu_do_unaligned_access(CPUState *cs, vaddr addr,
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index bd36062877..0983ec6471 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -481,6 +481,7 @@
  */
 #define FORCE_HS_EXCEP  2
 #define HS_TWO_STAGE4
+#define HS_HYP_LD_ST8
 
 /* RV32 satp CSR field masks */
 #define SATP32_MODE 0x8000
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 904899054d..58d1fa2675 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -238,6 +238,24 @@ void riscv_cpu_set_two_stage_lookup(CPURISCVState *env, 
bool enable)
 env->virt = set_field(env->virt, HS_TWO_STAGE, enable);
 }
 
+bool riscv_cpu_hyp_load_store_inst(CPURISCVState *env)
+{
+if (!riscv_has_ext(env, RVH)) {
+return false;
+}
+
+return get_field(env->virt, HS_HYP_LD_ST);
+}
+
+void riscv_cpu_set_hyp_load_store_inst(CPURISCVState *env, bool enable)
+{
+if (!riscv_has_ext(env, RVH)) {
+return;
+}
+
+env->virt = set_field(env->virt, HS_HYP_LD_ST, enable);
+}
+
 int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts)
 {
 CPURISCVState *env = >env;
@@ -346,7 +364,11 @@ static int get_physical_address(CPURISCVState *env, hwaddr 
*physical,
 use_background = true;
 }
 
-if (mode == PRV_M && access_type != MMU_INST_FETCH) {
+/* MPRV does not affect the virtual-machine load/store
+   instructions, HLV, HLVX, and HSV. */
+if (riscv_cpu_hyp_load_store_inst(env)) {
+mode = get_field(env->hstatus, HSTATUS_SPVP);
+} else if (mode == PRV_M && access_type != MMU_INST_FETCH) {
 if (get_field(env->mstatus, MSTATUS_MPRV)) {
 mode = get_field(env->mstatus, MSTATUS_MPP);
 }
@@ -711,17 +733,21 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
 qemu_log_mask(CPU_LOG_MMU, "%s ad %" VADDR_PRIx " rw %d mmu_idx %d\n",
   __func__, address, access_type, mmu_idx);
 
-if (mode == PRV_M && access_type != MMU_INST_FETCH) {
+/* MPRV does not affect the virtual-machine load/store
+   instructions, HLV, HLVX, and HSV. */
+if (riscv_cpu_hyp_load_store_inst(env)) {
+mode = get_field(env->hstatus, HSTATUS_SPVP);
+} else if (mode == PRV_M && access_type != MMU_INST_FETCH) {
 if (get_field(env->mstatus, MSTATUS_MPRV)) {
 mode = get_field(env->mstatus, MSTATUS_MPP);
 }
-}
 
-if (riscv_has_ext(env, RVH) && env->priv == PRV_M &&
-access_type != MMU_INST_FETCH &&
-get_field(env->mstatus, MSTATUS_MPRV) &&
-MSTATUS_MPV_ISSET(env)) {
-riscv_cpu_set_two_stage_lookup(env, true);
+if (riscv_has_ext(env, RVH) && env->priv == PRV_M &&
+access_type != MMU_INST_FETCH &&
+get_field(env->mstatus, MSTATUS_MPRV) &&
+MSTATUS_MPV_ISSET(env)) {
+riscv_cpu_set_two_stage_lookup(env, true);
+}
 }
 
 if (riscv_cpu_virt_enabled(env) ||
@@ -777,12 +803,16 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int 
size,
   __func__, address, ret, pa, prot);
 }
 
-/* We did the two stage lookup based on MPRV, unset the lookup */
-if (riscv_has_ext(env, RVH) && env->priv == PRV_M &&
-access_type != MMU_INST_FETCH &&
-get_field(env->mstatus, MSTATUS_MPRV) &&
-MSTATUS_MPV_ISSET(env)) {
-riscv_cpu_set_two_stage_lookup(env, false);
+/* MPRV does not affect the virtual-machine load/store
+   instructions, HLV, HLVX, and HSV. */
+if (!riscv_cpu_hyp_load_store_inst(env)) {
+