* [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode
2020-10-28 14:42 [PATCH v2 0/5] Fix the Hypervisor access functions Alistair Francis
@ 2020-10-28 14:42 ` Alistair Francis
2020-10-28 15:13 ` Richard Henderson
2020-10-28 14:42 ` [PATCH v2 2/5] target/riscv: Set the virtualised MMU mode when doing hyp accesses Alistair Francis
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2020-10-28 14:42 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair.francis, richard.henderson, bmeng.cn, palmer, alistair23
Add a new MMU mode that includes the current virt mode.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/cpu-param.h | 10 +++++++++-
target/riscv/cpu.h | 4 +++-
target/riscv/cpu_helper.c | 6 +++++-
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
index 664fc1d371..0db6e23140 100644
--- a/target/riscv/cpu-param.h
+++ b/target/riscv/cpu-param.h
@@ -18,6 +18,14 @@
# define TARGET_VIRT_ADDR_SPACE_BITS 32 /* sv32 */
#endif
#define TARGET_PAGE_BITS 12 /* 4 KiB Pages */
-#define NB_MMU_MODES 4
+/*
+ * The current MMU Modes are:
+ * - U mode 0b000
+ * - S mode 0b001
+ * - M mode 0b011
+ * - HU mode 0b100
+ * - HS mode 0b101
+ */
+#define NB_MMU_MODES 6
#endif
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 87b68affa8..5d8e54c426 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -363,7 +363,9 @@ void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
-#define TB_FLAGS_MMU_MASK 3
+#define TB_FLAGS_MMU_MASK 7
+#define TB_FLAGS_PRIV_MMU_MASK 3
+#define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2)
#define TB_FLAGS_MSTATUS_FS MSTATUS_FS
typedef CPURISCVState CPUArchState;
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 3eb3a034db..453e4c6d8a 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -30,6 +30,10 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
#ifdef CONFIG_USER_ONLY
return 0;
#else
+ if (riscv_cpu_virt_enabled(env)) {
+ return env->priv | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+ }
+
return env->priv;
#endif
}
@@ -323,7 +327,7 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
* (riscv_cpu_do_interrupt) is correct */
MemTxResult res;
MemTxAttrs attrs = MEMTXATTRS_UNSPECIFIED;
- int mode = mmu_idx;
+ int mode = mmu_idx & TB_FLAGS_PRIV_MMU_MASK;
bool use_background = false;
/*
--
2.28.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode
2020-10-28 14:42 ` [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode Alistair Francis
@ 2020-10-28 15:13 ` Richard Henderson
2020-10-28 20:51 ` Alistair Francis
0 siblings, 1 reply; 13+ messages in thread
From: Richard Henderson @ 2020-10-28 15:13 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, bmeng.cn, palmer
On 10/28/20 7:42 AM, Alistair Francis wrote:
> Add a new MMU mode that includes the current virt mode.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> target/riscv/cpu-param.h | 10 +++++++++-
> target/riscv/cpu.h | 4 +++-
> target/riscv/cpu_helper.c | 6 +++++-
> 3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
> index 664fc1d371..0db6e23140 100644
> --- a/target/riscv/cpu-param.h
> +++ b/target/riscv/cpu-param.h
> @@ -18,6 +18,14 @@
> # define TARGET_VIRT_ADDR_SPACE_BITS 32 /* sv32 */
> #endif
> #define TARGET_PAGE_BITS 12 /* 4 KiB Pages */
> -#define NB_MMU_MODES 4
> +/*
> + * The current MMU Modes are:
> + * - U mode 0b000
> + * - S mode 0b001
> + * - M mode 0b011
> + * - HU mode 0b100
> + * - HS mode 0b101
> + */
> +#define NB_MMU_MODES 6
>
> #endif
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 87b68affa8..5d8e54c426 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -363,7 +363,9 @@ void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
> target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
> void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
>
> -#define TB_FLAGS_MMU_MASK 3
> +#define TB_FLAGS_MMU_MASK 7
> +#define TB_FLAGS_PRIV_MMU_MASK 3
> +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2)
> #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
>
> typedef CPURISCVState CPUArchState;
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 3eb3a034db..453e4c6d8a 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -30,6 +30,10 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> #ifdef CONFIG_USER_ONLY
> return 0;
> #else
> + if (riscv_cpu_virt_enabled(env)) {
> + return env->priv | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
> + }
This is wrong. You only want to set this flag in response to one of the
hypervisor special instructions. This is setting it any time virt is enabled.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode
2020-10-28 15:13 ` Richard Henderson
@ 2020-10-28 20:51 ` Alistair Francis
2020-10-28 21:33 ` Richard Henderson
0 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2020-10-28 20:51 UTC (permalink / raw)
To: Richard Henderson
Cc: open list:RISC-V, Palmer Dabbelt, Bin Meng, Alistair Francis,
qemu-devel@nongnu.org Developers
On Wed, Oct 28, 2020 at 8:13 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 10/28/20 7:42 AM, Alistair Francis wrote:
> > Add a new MMU mode that includes the current virt mode.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> > target/riscv/cpu-param.h | 10 +++++++++-
> > target/riscv/cpu.h | 4 +++-
> > target/riscv/cpu_helper.c | 6 +++++-
> > 3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/riscv/cpu-param.h b/target/riscv/cpu-param.h
> > index 664fc1d371..0db6e23140 100644
> > --- a/target/riscv/cpu-param.h
> > +++ b/target/riscv/cpu-param.h
> > @@ -18,6 +18,14 @@
> > # define TARGET_VIRT_ADDR_SPACE_BITS 32 /* sv32 */
> > #endif
> > #define TARGET_PAGE_BITS 12 /* 4 KiB Pages */
> > -#define NB_MMU_MODES 4
> > +/*
> > + * The current MMU Modes are:
> > + * - U mode 0b000
> > + * - S mode 0b001
> > + * - M mode 0b011
> > + * - HU mode 0b100
> > + * - HS mode 0b101
> > + */
> > +#define NB_MMU_MODES 6
> >
> > #endif
> > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> > index 87b68affa8..5d8e54c426 100644
> > --- a/target/riscv/cpu.h
> > +++ b/target/riscv/cpu.h
> > @@ -363,7 +363,9 @@ void QEMU_NORETURN riscv_raise_exception(CPURISCVState *env,
> > target_ulong riscv_cpu_get_fflags(CPURISCVState *env);
> > void riscv_cpu_set_fflags(CPURISCVState *env, target_ulong);
> >
> > -#define TB_FLAGS_MMU_MASK 3
> > +#define TB_FLAGS_MMU_MASK 7
> > +#define TB_FLAGS_PRIV_MMU_MASK 3
> > +#define TB_FLAGS_PRIV_HYP_ACCESS_MASK (1 << 2)
> > #define TB_FLAGS_MSTATUS_FS MSTATUS_FS
> >
> > typedef CPURISCVState CPUArchState;
> > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> > index 3eb3a034db..453e4c6d8a 100644
> > --- a/target/riscv/cpu_helper.c
> > +++ b/target/riscv/cpu_helper.c
> > @@ -30,6 +30,10 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> > #ifdef CONFIG_USER_ONLY
> > return 0;
> > #else
> > + if (riscv_cpu_virt_enabled(env)) {
> > + return env->priv | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
> > + }
>
> This is wrong. You only want to set this flag in response to one of the
> hypervisor special instructions. This is setting it any time virt is enabled.
Isn't that ok though? I thought this was the correct approach.
Now we have a MMU context for Virtual guests (VS) which have different
contexts to the hypervisor (S). It also then means that when doing the
hypervisor access load/stores we can re-use the existing MMU context
from when the Hypervisor guest was running.
Alistair
>
>
> r~
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode
2020-10-28 20:51 ` Alistair Francis
@ 2020-10-28 21:33 ` Richard Henderson
0 siblings, 0 replies; 13+ messages in thread
From: Richard Henderson @ 2020-10-28 21:33 UTC (permalink / raw)
To: Alistair Francis
Cc: open list:RISC-V, Palmer Dabbelt, Bin Meng, Alistair Francis,
qemu-devel@nongnu.org Developers
On 10/28/20 1:51 PM, Alistair Francis wrote:
>>> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
>>> index 3eb3a034db..453e4c6d8a 100644
>>> --- a/target/riscv/cpu_helper.c
>>> +++ b/target/riscv/cpu_helper.c
>>> @@ -30,6 +30,10 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
>>> #ifdef CONFIG_USER_ONLY
>>> return 0;
>>> #else
>>> + if (riscv_cpu_virt_enabled(env)) {
>>> + return env->priv | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
>>> + }
>>
>> This is wrong. You only want to set this flag in response to one of the
>> hypervisor special instructions. This is setting it any time virt is enabled.
>
> Isn't that ok though? I thought this was the correct approach.
No.
Consider: The *presence* of this bit means a change of behaviour in
get_physical_address.
Things are mostly working for you because you then mask this bit off when
installing it to TBFLAGS. Which you then use during translate without adding
it back on, except for the one place you need it.
The things that won't work are generic bits of code which use e.g.
cpu_ldub_data(), which calls cpu_mmu_index(), change behaviour. Which you
don't want.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/5] target/riscv: Set the virtualised MMU mode when doing hyp accesses
2020-10-28 14:42 [PATCH v2 0/5] Fix the Hypervisor access functions Alistair Francis
2020-10-28 14:42 ` [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode Alistair Francis
@ 2020-10-28 14:42 ` Alistair Francis
2020-10-28 15:08 ` Richard Henderson
2020-10-28 14:42 ` [PATCH v2 3/5] target/riscv: Remove the HS_TWO_STAGE flag Alistair Francis
` (2 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2020-10-28 14:42 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair.francis, richard.henderson, bmeng.cn, palmer, alistair23
When performing the hypervisor load/store operations set the MMU mode to
indicate that we are virtualised.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/op_helper.c | 30 +++++++++++++++++-------------
1 file changed, 17 insertions(+), 13 deletions(-)
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index e20d56dcb8..548c5851ec 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -235,30 +235,31 @@ target_ulong helper_hyp_load(CPURISCVState *env, target_ulong address,
(env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
get_field(env->hstatus, HSTATUS_HU))) {
target_ulong pte;
+ int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
riscv_cpu_set_two_stage_lookup(env, true);
switch (memop) {
case MO_SB:
- pte = cpu_ldsb_data_ra(env, address, GETPC());
+ pte = cpu_ldsb_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_UB:
- pte = cpu_ldub_data_ra(env, address, GETPC());
+ pte = cpu_ldub_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_TESW:
- pte = cpu_ldsw_data_ra(env, address, GETPC());
+ pte = cpu_ldsw_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_TEUW:
- pte = cpu_lduw_data_ra(env, address, GETPC());
+ pte = cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_TESL:
- pte = cpu_ldl_data_ra(env, address, GETPC());
+ pte = cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_TEUL:
- pte = cpu_ldl_data_ra(env, address, GETPC());
+ pte = cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_TEQ:
- pte = cpu_ldq_data_ra(env, address, GETPC());
+ pte = cpu_ldq_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
default:
g_assert_not_reached();
@@ -284,23 +285,25 @@ void helper_hyp_store(CPURISCVState *env, target_ulong address,
(env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
(env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
get_field(env->hstatus, HSTATUS_HU))) {
+ int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+
riscv_cpu_set_two_stage_lookup(env, true);
switch (memop) {
case MO_SB:
case MO_UB:
- cpu_stb_data_ra(env, address, val, GETPC());
+ cpu_stb_mmuidx_ra(env, address, val, mmu_idx, GETPC());
break;
case MO_TESW:
case MO_TEUW:
- cpu_stw_data_ra(env, address, val, GETPC());
+ cpu_stw_mmuidx_ra(env, address, val, mmu_idx, GETPC());
break;
case MO_TESL:
case MO_TEUL:
- cpu_stl_data_ra(env, address, val, GETPC());
+ cpu_stl_mmuidx_ra(env, address, val, mmu_idx, GETPC());
break;
case MO_TEQ:
- cpu_stq_data_ra(env, address, val, GETPC());
+ cpu_stq_mmuidx_ra(env, address, val, mmu_idx, GETPC());
break;
default:
g_assert_not_reached();
@@ -326,15 +329,16 @@ target_ulong helper_hyp_x_load(CPURISCVState *env, target_ulong address,
(env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
get_field(env->hstatus, HSTATUS_HU))) {
target_ulong pte;
+ int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
riscv_cpu_set_two_stage_lookup(env, true);
switch (memop) {
case MO_TEUW:
- pte = cpu_lduw_data_ra(env, address, GETPC());
+ pte = cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
case MO_TEUL:
- pte = cpu_ldl_data_ra(env, address, GETPC());
+ pte = cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
break;
default:
g_assert_not_reached();
--
2.28.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* [PATCH v2 3/5] target/riscv: Remove the HS_TWO_STAGE flag
2020-10-28 14:42 [PATCH v2 0/5] Fix the Hypervisor access functions Alistair Francis
2020-10-28 14:42 ` [PATCH v2 1/5] target/riscv: Add a virtualised MMU Mode Alistair Francis
2020-10-28 14:42 ` [PATCH v2 2/5] target/riscv: Set the virtualised MMU mode when doing hyp accesses Alistair Francis
@ 2020-10-28 14:42 ` Alistair Francis
2020-10-28 15:11 ` Richard Henderson
2020-10-28 14:42 ` [PATCH v2 4/5] target/riscv: Remove the hyp load and store functions Alistair Francis
2020-10-28 14:42 ` [PATCH v2 5/5] target/riscv: Split the Hypervisor execute load helpers Alistair Francis
4 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2020-10-28 14:42 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair.francis, richard.henderson, bmeng.cn, palmer, alistair23
The HS_TWO_STAGE flag is no longer required as the MMU index contains
the information if we are performing a two stage access.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/cpu.h | 3 +-
target/riscv/cpu_bits.h | 1 -
target/riscv/cpu_helper.c | 61 ++++++++++++++++-----------------------
target/riscv/op_helper.c | 12 --------
4 files changed, 26 insertions(+), 51 deletions(-)
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 5d8e54c426..0cf48a1521 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -323,8 +323,7 @@ bool riscv_cpu_virt_enabled(CPURISCVState *env);
void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable);
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_two_stage_lookup(int mmu_idx);
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 daedad8691..24b24c69c5 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -469,7 +469,6 @@
* page table fault.
*/
#define FORCE_HS_EXCEP 2
-#define HS_TWO_STAGE 4
/* RV32 satp CSR field masks */
#define SATP32_MODE 0x80000000
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 453e4c6d8a..9edf7282d9 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -211,22 +211,9 @@ void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable)
env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable);
}
-bool riscv_cpu_two_stage_lookup(CPURISCVState *env)
+bool riscv_cpu_two_stage_lookup(int mmu_idx)
{
- if (!riscv_has_ext(env, RVH)) {
- return false;
- }
-
- return get_field(env->virt, HS_TWO_STAGE);
-}
-
-void riscv_cpu_set_two_stage_lookup(CPURISCVState *env, bool enable)
-{
- if (!riscv_has_ext(env, RVH)) {
- return;
- }
-
- env->virt = set_field(env->virt, HS_TWO_STAGE, enable);
+ return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK;
}
int riscv_cpu_claim_interrupts(RISCVCPU *cpu, uint32_t interrupts)
@@ -337,7 +324,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
* was called. Background registers will be used if the guest has
* forced a two stage translation to be on (in HS or M mode).
*/
- if (riscv_cpu_two_stage_lookup(env) && access_type != MMU_INST_FETCH) {
+ if ((!riscv_cpu_virt_enabled(env) && riscv_cpu_two_stage_lookup(mmu_idx))
+ && access_type != MMU_INST_FETCH) {
use_background = true;
}
@@ -576,7 +564,7 @@ restart:
static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
MMUAccessType access_type, bool pmp_violation,
- bool first_stage)
+ bool first_stage, bool two_stage)
{
CPUState *cs = env_cpu(env);
int page_fault_exceptions;
@@ -599,8 +587,7 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
}
break;
case MMU_DATA_LOAD:
- if ((riscv_cpu_virt_enabled(env) || riscv_cpu_two_stage_lookup(env)) &&
- !first_stage) {
+ if (two_stage && !first_stage) {
cs->exception_index = RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT;
} else {
cs->exception_index = page_fault_exceptions ?
@@ -608,8 +595,7 @@ static void raise_mmu_exception(CPURISCVState *env, target_ulong address,
}
break;
case MMU_DATA_STORE:
- if ((riscv_cpu_virt_enabled(env) || riscv_cpu_two_stage_lookup(env)) &&
- !first_stage) {
+ if (two_stage && !first_stage) {
cs->exception_index = RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT;
} else {
cs->exception_index = page_fault_exceptions ?
@@ -700,6 +686,7 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
int prot, prot2;
bool pmp_violation = false;
bool first_stage_error = true;
+ bool two_stage_lookup = false;
int ret = TRANSLATE_FAIL;
int mode = mmu_idx;
target_ulong tlb_size = 0;
@@ -719,11 +706,12 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
access_type != MMU_INST_FETCH &&
get_field(env->mstatus, MSTATUS_MPRV) &&
get_field(env->mstatus, MSTATUS_MPV)) {
- riscv_cpu_set_two_stage_lookup(env, true);
+ two_stage_lookup = true;
}
if (riscv_cpu_virt_enabled(env) ||
- (riscv_cpu_two_stage_lookup(env) && access_type != MMU_INST_FETCH)) {
+ ((riscv_cpu_two_stage_lookup(mmu_idx) || two_stage_lookup) &&
+ access_type != MMU_INST_FETCH)) {
/* Two stage lookup */
ret = get_physical_address(env, &pa, &prot, address,
&env->guest_phys_fault_addr, access_type,
@@ -786,14 +774,6 @@ 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) &&
- get_field(env->mstatus, MSTATUS_MPV)) {
- riscv_cpu_set_two_stage_lookup(env, false);
- }
-
if (riscv_feature(env, RISCV_FEATURE_PMP) &&
(ret == TRANSLATE_SUCCESS) &&
!pmp_hart_has_privs(env, pa, size, 1 << access_type, mode)) {
@@ -815,7 +795,10 @@ bool riscv_cpu_tlb_fill(CPUState *cs, vaddr address, int size,
} else if (probe) {
return false;
} else {
- raise_mmu_exception(env, address, access_type, pmp_violation, first_stage_error);
+ raise_mmu_exception(env, address, access_type, pmp_violation,
+ first_stage_error,
+ riscv_cpu_virt_enabled(env) ||
+ riscv_cpu_two_stage_lookup(mmu_idx));
riscv_raise_exception(env, cs->exception_index, retaddr);
}
@@ -919,9 +902,16 @@ void riscv_cpu_do_interrupt(CPUState *cs)
/* handle the trap in S-mode */
if (riscv_has_ext(env, RVH)) {
target_ulong hdeleg = async ? env->hideleg : env->hedeleg;
+ bool two_stage_lookup = false;
+
+ if (env->priv == PRV_M ||
+ (env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
+ (env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
+ get_field(env->hstatus, HSTATUS_HU))) {
+ two_stage_lookup = true;
+ }
- if ((riscv_cpu_virt_enabled(env) ||
- riscv_cpu_two_stage_lookup(env)) && write_tval) {
+ if ((riscv_cpu_virt_enabled(env) || two_stage_lookup) && write_tval) {
/*
* If we are writing a guest virtual address to stval, set
* this to 1. If we are trapping to VS we will set this to 0
@@ -959,11 +949,10 @@ void riscv_cpu_do_interrupt(CPUState *cs)
riscv_cpu_set_force_hs_excep(env, 0);
} else {
/* Trap into HS mode */
- if (!riscv_cpu_two_stage_lookup(env)) {
+ if (!two_stage_lookup) {
env->hstatus = set_field(env->hstatus, HSTATUS_SPV,
riscv_cpu_virt_enabled(env));
}
- riscv_cpu_set_two_stage_lookup(env, false);
htval = env->guest_phys_fault_addr;
}
}
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 548c5851ec..5759850e69 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -237,8 +237,6 @@ target_ulong helper_hyp_load(CPURISCVState *env, target_ulong address,
target_ulong pte;
int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
- riscv_cpu_set_two_stage_lookup(env, true);
-
switch (memop) {
case MO_SB:
pte = cpu_ldsb_mmuidx_ra(env, address, mmu_idx, GETPC());
@@ -265,8 +263,6 @@ target_ulong helper_hyp_load(CPURISCVState *env, target_ulong address,
g_assert_not_reached();
}
- riscv_cpu_set_two_stage_lookup(env, false);
-
return pte;
}
@@ -287,8 +283,6 @@ void helper_hyp_store(CPURISCVState *env, target_ulong address,
get_field(env->hstatus, HSTATUS_HU))) {
int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
- riscv_cpu_set_two_stage_lookup(env, true);
-
switch (memop) {
case MO_SB:
case MO_UB:
@@ -309,8 +303,6 @@ void helper_hyp_store(CPURISCVState *env, target_ulong address,
g_assert_not_reached();
}
- riscv_cpu_set_two_stage_lookup(env, false);
-
return;
}
@@ -331,8 +323,6 @@ target_ulong helper_hyp_x_load(CPURISCVState *env, target_ulong address,
target_ulong pte;
int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
- riscv_cpu_set_two_stage_lookup(env, true);
-
switch (memop) {
case MO_TEUW:
pte = cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
@@ -344,8 +334,6 @@ target_ulong helper_hyp_x_load(CPURISCVState *env, target_ulong address,
g_assert_not_reached();
}
- riscv_cpu_set_two_stage_lookup(env, false);
-
return pte;
}
--
2.28.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/5] target/riscv: Remove the HS_TWO_STAGE flag
2020-10-28 14:42 ` [PATCH v2 3/5] target/riscv: Remove the HS_TWO_STAGE flag Alistair Francis
@ 2020-10-28 15:11 ` Richard Henderson
0 siblings, 0 replies; 13+ messages in thread
From: Richard Henderson @ 2020-10-28 15:11 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, bmeng.cn, palmer
On 10/28/20 7:42 AM, Alistair Francis wrote:
> @@ -337,7 +324,8 @@ static int get_physical_address(CPURISCVState *env, hwaddr *physical,
> * was called. Background registers will be used if the guest has
> * forced a two stage translation to be on (in HS or M mode).
> */
> - if (riscv_cpu_two_stage_lookup(env) && access_type != MMU_INST_FETCH) {
> + if ((!riscv_cpu_virt_enabled(env) && riscv_cpu_two_stage_lookup(mmu_idx))
> + && access_type != MMU_INST_FETCH) {
This access_type check looks like a bug. And possibly why you said that you
can't use code access for the execute load helpers.
Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/5] target/riscv: Remove the hyp load and store functions
2020-10-28 14:42 [PATCH v2 0/5] Fix the Hypervisor access functions Alistair Francis
` (2 preceding siblings ...)
2020-10-28 14:42 ` [PATCH v2 3/5] target/riscv: Remove the HS_TWO_STAGE flag Alistair Francis
@ 2020-10-28 14:42 ` Alistair Francis
2020-10-28 15:18 ` Richard Henderson
2020-10-28 14:42 ` [PATCH v2 5/5] target/riscv: Split the Hypervisor execute load helpers Alistair Francis
4 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2020-10-28 14:42 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair.francis, richard.henderson, bmeng.cn, palmer, alistair23
Remove the special Virtulisation load and store functions and just use
the standard tcg tcg_gen_qemu_ld_tl() and tcg_gen_qemu_st_tl() functions
instead.
As part of this change we ensure we still run an access check to make
sure we can perform the operations.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/helper.h | 3 +-
target/riscv/op_helper.c | 72 +--------------
target/riscv/insn_trans/trans_rvh.c.inc | 111 +++++++-----------------
3 files changed, 35 insertions(+), 151 deletions(-)
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index 4b690147fb..7dbdd117d2 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -81,8 +81,7 @@ DEF_HELPER_1(tlb_flush, void, env)
#ifndef CONFIG_USER_ONLY
DEF_HELPER_1(hyp_tlb_flush, void, env)
DEF_HELPER_1(hyp_gvma_tlb_flush, void, env)
-DEF_HELPER_4(hyp_load, tl, env, tl, tl, tl)
-DEF_HELPER_5(hyp_store, void, env, tl, tl, tl, tl)
+DEF_HELPER_1(hyp_access_check, void, env)
DEF_HELPER_4(hyp_x_load, tl, env, tl, tl, tl)
#endif
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index 5759850e69..d81d8282cc 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -227,82 +227,12 @@ void helper_hyp_gvma_tlb_flush(CPURISCVState *env)
helper_hyp_tlb_flush(env);
}
-target_ulong helper_hyp_load(CPURISCVState *env, target_ulong address,
- target_ulong attrs, target_ulong memop)
+void helper_hyp_access_check(CPURISCVState *env)
{
if (env->priv == PRV_M ||
(env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
(env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
get_field(env->hstatus, HSTATUS_HU))) {
- target_ulong pte;
- int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
-
- switch (memop) {
- case MO_SB:
- pte = cpu_ldsb_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_UB:
- pte = cpu_ldub_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_TESW:
- pte = cpu_ldsw_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_TEUW:
- pte = cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_TESL:
- pte = cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_TEUL:
- pte = cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_TEQ:
- pte = cpu_ldq_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- default:
- g_assert_not_reached();
- }
-
- return pte;
- }
-
- if (riscv_cpu_virt_enabled(env)) {
- riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
- } else {
- riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
- }
- return 0;
-}
-
-void helper_hyp_store(CPURISCVState *env, target_ulong address,
- target_ulong val, target_ulong attrs, target_ulong memop)
-{
- if (env->priv == PRV_M ||
- (env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
- (env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
- get_field(env->hstatus, HSTATUS_HU))) {
- int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
-
- switch (memop) {
- case MO_SB:
- case MO_UB:
- cpu_stb_mmuidx_ra(env, address, val, mmu_idx, GETPC());
- break;
- case MO_TESW:
- case MO_TEUW:
- cpu_stw_mmuidx_ra(env, address, val, mmu_idx, GETPC());
- break;
- case MO_TESL:
- case MO_TEUL:
- cpu_stl_mmuidx_ra(env, address, val, mmu_idx, GETPC());
- break;
- case MO_TEQ:
- cpu_stq_mmuidx_ra(env, address, val, mmu_idx, GETPC());
- break;
- default:
- g_assert_not_reached();
- }
-
return;
}
diff --git a/target/riscv/insn_trans/trans_rvh.c.inc b/target/riscv/insn_trans/trans_rvh.c.inc
index 881c9ef4d2..79968701e9 100644
--- a/target/riscv/insn_trans/trans_rvh.c.inc
+++ b/target/riscv/insn_trans/trans_rvh.c.inc
@@ -22,20 +22,16 @@ static bool trans_hlv_b(DisasContext *ctx, arg_hlv_b *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_SB);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_SB);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -48,20 +44,16 @@ static bool trans_hlv_h(DisasContext *ctx, arg_hlv_h *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TESW);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESW);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -74,20 +66,16 @@ static bool trans_hlv_w(DisasContext *ctx, arg_hlv_w *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TESL);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESL);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -100,20 +88,16 @@ static bool trans_hlv_bu(DisasContext *ctx, arg_hlv_bu *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_UB);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_UB);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -126,20 +110,15 @@ static bool trans_hlv_hu(DisasContext *ctx, arg_hlv_hu *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
- gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TEUW);
+ gen_helper_hyp_access_check(cpu_env);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ gen_get_gpr(t0, a->rs1);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEUW);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -152,20 +131,16 @@ static bool trans_hsv_b(DisasContext *ctx, arg_hsv_b *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv dat = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
gen_get_gpr(dat, a->rs2);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_SB);
- gen_helper_hyp_store(cpu_env, t0, dat, mem_idx, memop);
+ tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_SB);
tcg_temp_free(t0);
tcg_temp_free(dat);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -178,20 +153,16 @@ static bool trans_hsv_h(DisasContext *ctx, arg_hsv_h *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv dat = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
gen_get_gpr(dat, a->rs2);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TESW);
- gen_helper_hyp_store(cpu_env, t0, dat, mem_idx, memop);
+ tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESW);
tcg_temp_free(t0);
tcg_temp_free(dat);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -204,20 +175,16 @@ static bool trans_hsv_w(DisasContext *ctx, arg_hsv_w *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv dat = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
gen_get_gpr(dat, a->rs2);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TESL);
- gen_helper_hyp_store(cpu_env, t0, dat, mem_idx, memop);
+ tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TESL);
tcg_temp_free(t0);
tcg_temp_free(dat);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -231,20 +198,16 @@ static bool trans_hlv_wu(DisasContext *ctx, arg_hlv_wu *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TEUL);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEUL);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -257,20 +220,16 @@ static bool trans_hlv_d(DisasContext *ctx, arg_hlv_d *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TEQ);
- gen_helper_hyp_load(t1, cpu_env, t0, mem_idx, memop);
+ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEQ);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -283,20 +242,16 @@ static bool trans_hsv_d(DisasContext *ctx, arg_hsv_d *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv dat = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
+
+ gen_helper_hyp_access_check(cpu_env);
gen_get_gpr(t0, a->rs1);
gen_get_gpr(dat, a->rs2);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TEQ);
- gen_helper_hyp_store(cpu_env, t0, dat, mem_idx, memop);
+ tcg_gen_qemu_st_tl(dat, t0, ctx->mem_idx | TB_FLAGS_PRIV_HYP_ACCESS_MASK, MO_TEQ);
tcg_temp_free(t0);
tcg_temp_free(dat);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
--
2.28.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 4/5] target/riscv: Remove the hyp load and store functions
2020-10-28 14:42 ` [PATCH v2 4/5] target/riscv: Remove the hyp load and store functions Alistair Francis
@ 2020-10-28 15:18 ` Richard Henderson
0 siblings, 0 replies; 13+ messages in thread
From: Richard Henderson @ 2020-10-28 15:18 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, bmeng.cn, palmer
On 10/28/20 7:42 AM, Alistair Francis wrote:
> +void helper_hyp_access_check(CPURISCVState *env)
> {
> if (env->priv == PRV_M ||
> (env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
> (env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
> get_field(env->hstatus, HSTATUS_HU))) {
> return;
> }
While this works, I think it would be better to compute this into one bit of
TBFLAGS. Then you can test it during translate and do not need an external
helper at all for the data accesses.
It also means that patch 5 can be simplified...
r~
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 5/5] target/riscv: Split the Hypervisor execute load helpers
2020-10-28 14:42 [PATCH v2 0/5] Fix the Hypervisor access functions Alistair Francis
` (3 preceding siblings ...)
2020-10-28 14:42 ` [PATCH v2 4/5] target/riscv: Remove the hyp load and store functions Alistair Francis
@ 2020-10-28 14:42 ` Alistair Francis
2020-10-28 15:22 ` Richard Henderson
4 siblings, 1 reply; 13+ messages in thread
From: Alistair Francis @ 2020-10-28 14:42 UTC (permalink / raw)
To: qemu-devel, qemu-riscv
Cc: alistair.francis, richard.henderson, bmeng.cn, palmer, alistair23
Split the hypervisor execute load functions into two seperate functions.
This avoids us having to pass the memop to the C helper functions.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
target/riscv/helper.h | 3 +-
target/riscv/op_helper.c | 38 ++++++++++++++-----------
target/riscv/insn_trans/trans_rvh.c.inc | 16 ++---------
3 files changed, 26 insertions(+), 31 deletions(-)
diff --git a/target/riscv/helper.h b/target/riscv/helper.h
index 7dbdd117d2..ae50730273 100644
--- a/target/riscv/helper.h
+++ b/target/riscv/helper.h
@@ -82,7 +82,8 @@ DEF_HELPER_1(tlb_flush, void, env)
DEF_HELPER_1(hyp_tlb_flush, void, env)
DEF_HELPER_1(hyp_gvma_tlb_flush, void, env)
DEF_HELPER_1(hyp_access_check, void, env)
-DEF_HELPER_4(hyp_x_load, tl, env, tl, tl, tl)
+DEF_HELPER_2(hyp_hlvx_hu, tl, env, tl)
+DEF_HELPER_2(hyp_hlvx_wu, tl, env, tl)
#endif
/* Vector functions */
diff --git a/target/riscv/op_helper.c b/target/riscv/op_helper.c
index d81d8282cc..ff4426cf4d 100644
--- a/target/riscv/op_helper.c
+++ b/target/riscv/op_helper.c
@@ -243,28 +243,34 @@ void helper_hyp_access_check(CPURISCVState *env)
}
}
-target_ulong helper_hyp_x_load(CPURISCVState *env, target_ulong address,
- target_ulong attrs, target_ulong memop)
+target_ulong helper_hyp_hlvx_hu(CPURISCVState *env, target_ulong address)
{
if (env->priv == PRV_M ||
(env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
(env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
get_field(env->hstatus, HSTATUS_HU))) {
- target_ulong pte;
- int mmu_idx = cpu_mmu_index(env, false) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
-
- switch (memop) {
- case MO_TEUW:
- pte = cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- case MO_TEUL:
- pte = cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
- break;
- default:
- g_assert_not_reached();
- }
+ int mmu_idx = cpu_mmu_index(env, true) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
+
+ return cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
+ }
+
+ if (riscv_cpu_virt_enabled(env)) {
+ riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
+ } else {
+ riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
+ }
+ return 0;
+}
+
+target_ulong helper_hyp_hlvx_wu(CPURISCVState *env, target_ulong address)
+{
+ if (env->priv == PRV_M ||
+ (env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
+ (env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
+ get_field(env->hstatus, HSTATUS_HU))) {
+ int mmu_idx = cpu_mmu_index(env, true) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
- return pte;
+ return cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
}
if (riscv_cpu_virt_enabled(env)) {
diff --git a/target/riscv/insn_trans/trans_rvh.c.inc b/target/riscv/insn_trans/trans_rvh.c.inc
index 79968701e9..f3ffd742d3 100644
--- a/target/riscv/insn_trans/trans_rvh.c.inc
+++ b/target/riscv/insn_trans/trans_rvh.c.inc
@@ -265,20 +265,14 @@ static bool trans_hlvx_hu(DisasContext *ctx, arg_hlvx_hu *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TEUW);
- gen_helper_hyp_x_load(t1, cpu_env, t0, mem_idx, memop);
+ gen_helper_hyp_hlvx_hu(t1, cpu_env, t0);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
@@ -291,20 +285,14 @@ static bool trans_hlvx_wu(DisasContext *ctx, arg_hlvx_wu *a)
#ifndef CONFIG_USER_ONLY
TCGv t0 = tcg_temp_new();
TCGv t1 = tcg_temp_new();
- TCGv mem_idx = tcg_temp_new();
- TCGv memop = tcg_temp_new();
gen_get_gpr(t0, a->rs1);
- tcg_gen_movi_tl(mem_idx, ctx->mem_idx);
- tcg_gen_movi_tl(memop, MO_TEUL);
- gen_helper_hyp_x_load(t1, cpu_env, t0, mem_idx, memop);
+ gen_helper_hyp_hlvx_wu(t1, cpu_env, t0);
gen_set_gpr(a->rd, t1);
tcg_temp_free(t0);
tcg_temp_free(t1);
- tcg_temp_free(mem_idx);
- tcg_temp_free(memop);
return true;
#else
return false;
--
2.28.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 5/5] target/riscv: Split the Hypervisor execute load helpers
2020-10-28 14:42 ` [PATCH v2 5/5] target/riscv: Split the Hypervisor execute load helpers Alistair Francis
@ 2020-10-28 15:22 ` Richard Henderson
0 siblings, 0 replies; 13+ messages in thread
From: Richard Henderson @ 2020-10-28 15:22 UTC (permalink / raw)
To: Alistair Francis, qemu-devel, qemu-riscv; +Cc: alistair23, bmeng.cn, palmer
On 10/28/20 7:42 AM, Alistair Francis wrote:
> +target_ulong helper_hyp_hlvx_hu(CPURISCVState *env, target_ulong address)
> {
> if (env->priv == PRV_M ||
> (env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
> (env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
> get_field(env->hstatus, HSTATUS_HU))) {
> + int mmu_idx = cpu_mmu_index(env, true) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
> +
> + return cpu_lduw_mmuidx_ra(env, address, mmu_idx, GETPC());
> + }
> +
> + if (riscv_cpu_virt_enabled(env)) {
> + riscv_raise_exception(env, RISCV_EXCP_VIRT_INSTRUCTION_FAULT, GETPC());
> + } else {
> + riscv_raise_exception(env, RISCV_EXCP_ILLEGAL_INST, GETPC());
> + }
> + return 0;
> +}
> +
> +target_ulong helper_hyp_hlvx_wu(CPURISCVState *env, target_ulong address)
> +{
> + if (env->priv == PRV_M ||
> + (env->priv == PRV_S && !riscv_cpu_virt_enabled(env)) ||
> + (env->priv == PRV_U && !riscv_cpu_virt_enabled(env) &&
> + get_field(env->hstatus, HSTATUS_HU))) {
> + int mmu_idx = cpu_mmu_index(env, true) | TB_FLAGS_PRIV_HYP_ACCESS_MASK;
>
> + return cpu_ldl_mmuidx_ra(env, address, mmu_idx, GETPC());
> }
Do not replicate the PRV tests.
My first suggestion is to compute this into TBFLAGS and test it at translate
time, so that these functions just become the one cpu_ld* call.
But failing that, at least split out the test + exception into a common helper
function.
r~
^ permalink raw reply [flat|nested] 13+ messages in thread