| Message ID | d0eeb9ea64462044a67f5b803d6cb62dd10e017a.1603896075.git.alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix the Hypervisor access functions | expand |
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~
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~
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~
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; /*
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(-)