| Message ID | 20210522154429.361235-1-josemartins90@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] target/riscv: fix VS interrupts forwarding to HS | expand |
Hi Jose,
For one patch, the commit message is too long and complex. I recommend
to split
this patch to a patch set with 4 patches. The tremohread topic is
'target/riscv: Remove force hs exception'
1) Define the right hsie to select pending_hs_irqs.
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 21c54ef561..babe3d844b 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
#ifndef CONFIG_USER_ONLY
static int riscv_cpu_local_irq_pending(CPURISCVState *env)
{
target_ulong irqs;
+ target_ulong virt_enabled = riscv_cpu_virt_enabled(env);
target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
- target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE);
target_ulong pending = env->mip & env->mie &
~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);
target_ulong vspending = (env->mip & env->mie &
(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
target_ulong mie = env->priv < PRV_M ||
(env->priv == PRV_M && mstatus_mie);
target_ulong sie = env->priv < PRV_S ||
(env->priv == PRV_S && mstatus_sie);
+ target_ulong hsie = virt_enabled || sie;
if (riscv_cpu_virt_enabled(env)) {
target_ulong pending_hs_irq = pending & -hs_sie;
if (pending_hs_irq) {
riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
return ctz64(pending_hs_irq);
}
pending = vspending;
}
irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie);
2) Merge the vspending to pending, and calculate the right irq.
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 21c54ef561..babe3d844b 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
@@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
#ifndef CONFIG_USER_ONLY
static int riscv_cpu_local_irq_pending(CPURISCVState *env)
{
target_ulong irqs;
target_ulong virt_enabled = riscv_cpu_virt_enabled(env);
target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
- target_ulong pending = env->mip & env->mie &
- ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);
- target_ulong vspending = (env->mip & env->mie &
- (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
+ target_ulong pending = env->mip & env->mie;
target_ulong mie = env->priv < PRV_M ||
(env->priv == PRV_M && mstatus_mie);
target_ulong sie = env->priv < PRV_S ||
(env->priv == PRV_S && mstatus_sie);
target_ulong hsie = virt_enabled || sie;
+ target_ulong vsie = virt_enabled && sie;
- if (riscv_cpu_virt_enabled(env)) {
- target_ulong pending_hs_irq = pending & -hs_sie;
-
- if (pending_hs_irq) {
- riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
- return ctz64(pending_hs_irq);
- }
-
- pending = vspending;
- }
-
- irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie);
+ irqs = (pending & ~env->mideleg & -mie) |
+ (pending & env->mideleg & ~env->hideleg & -hsie) |
+ (pending & env->mideleg & env->hideleg & -vsie);
if (irqs) {
return ctz64(irqs); /* since non-zero */
3) Clear the use force_hs_excp of the synchronous guest exceptions.
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 21c54ef561..babe3d844b 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
bool riscv_cpu_two_stage_lookup(int mmu_idx)
{
return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK;
@@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
RISCVCPU *cpu = RISCV_CPU(cs);
CPURISCVState *env = &cpu->env;
- bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env);
uint64_t s;
/* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide
@@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
- force_hs_execp = true;
- /* fallthrough */
case RISCV_EXCP_INST_ADDR_MIS:
case RISCV_EXCP_INST_ACCESS_FAULT:
case RISCV_EXCP_LOAD_ADDR_MIS:
@@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
}
- if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) &&
- !force_hs_execp) {
+ if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
/* Trap to VS mode */
/*
* See if we need to adjust cause. Yes if its VS mode interrupt
@@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
htval = env->guest_phys_fault_addr;
riscv_cpu_set_virt_enabled(env, 0);
riscv_cpu_set_force_hs_excep(env, 0);
} else {
/* Trap into HS mode */
env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
@@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
/* Trapping to M mode, virt is disabled */
riscv_cpu_set_virt_enabled(env, 0);
riscv_cpu_set_force_hs_excep(env, 0);
}
s = env->mstatus;
4) Remove the definition and references of riscv_cpu_set_force_hs_excep and
riscv_cpu_force_hs_excep_enabled.
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
index 0a33d387ba..a30a64241a 100644
--- a/target/riscv/cpu.h
+++ b/target/riscv/cpu.h
@@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
bool riscv_cpu_fp_enabled(CPURISCVState *env);
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(int mmu_idx);
int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch);
hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
index caf4599207..7322f54157 100644
--- a/target/riscv/cpu_bits.h
+++ b/target/riscv/cpu_bits.h
@@ -462,12 +462,6 @@
/* Virtulisation Register Fields */
#define VIRT_ONOFF 1
-/* This is used to save state for when we take an exception. If this is set
- * that means that we want to force a HS level exception (no matter what the
- * delegation is set to). This will occur for things such as a second level
- * page table fault.
- */
-#define FORCE_HS_EXCEP 2
/* RV32 satp CSR field masks */
#define SATP32_MODE 0x80000000
diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
index 21c54ef561..babe3d844b 100644
--- a/target/riscv/cpu_helper.c
+++ b/target/riscv/cpu_helper.c
-bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env)
-{
- if (!riscv_has_ext(env, RVH)) {
- return false;
- }
-
- return get_field(env->virt, FORCE_HS_EXCEP);
-}
-
-void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable)
-{
- if (!riscv_has_ext(env, RVH)) {
- return;
- }
-
- env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable);
-}
-
bool riscv_cpu_two_stage_lookup(int mmu_idx)
{
return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK;
@@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
RISCVCPU *cpu = RISCV_CPU(cs);
CPURISCVState *env = &cpu->env;
uint64_t s;
/* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide
@@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
case RISCV_EXCP_INST_ADDR_MIS:
case RISCV_EXCP_INST_ACCESS_FAULT:
case RISCV_EXCP_LOAD_ADDR_MIS:
@@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
}
if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
/* Trap to VS mode */
/*
* See if we need to adjust cause. Yes if its VS mode interrupt
@@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
htval = env->guest_phys_fault_addr;
riscv_cpu_set_virt_enabled(env, 0);
- riscv_cpu_set_force_hs_excep(env, 0);
} else {
/* Trap into HS mode */
env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
@@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
/* Trapping to M mode, virt is disabled */
riscv_cpu_set_virt_enabled(env, 0);
- riscv_cpu_set_force_hs_excep(env, 0);
}
s = env->mstatus;
On 5/22/21 11:44 PM, Jose Martins wrote:
> VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when
> not delegated in hideleg (which was not being taken into account).
I think there are two errors in riscv_cpu_local_irq_pending.
1) VS interrupts can't be forwarded to hs-mode rightly . It has
nothing to do with delegate or not in hideleg. The reason is that
VS interrupts are always discarded when V=0 in
riscv_cpu_local_irq_pending.
2) Use MSTATUS_SIE in mstatus_hs to select pending_hs_irqs.
The reason is as you described below.
> This
> was mainly because hs level sie was not always considered enabled when
> it should. The spec states that "Interrupts for higher-privilege modes,
> y>x, are always globally enabled regardless of the setting of the global
> yIE bit for the higher-privilege mode." and also "For purposes of
> interrupt global enables, HS-mode is considered more privileged than
> VS-mode, and VS-mode is considered more privileged than VU-mode".
This can be used as commit message for patch 1/4.
>
> These interrupts should be treated the same as any other kind of
> exception. Therefore, there is no need to "force an hs exception" as the
> current privilege level, the state of the global ie and of the
> delegation registers should be enough to route the interrupt to the
> appropriate privilege level in riscv_cpu_do_interrupt.
This can be used as commit message for patch 2/4.
> Also, these
> interrupts never target m-mode, which is guaranteed by the hardwiring
> of the corresponding bits in mideleg.
> The same is true for synchronous
> exceptions, specifically, guest page faults which must be hardwired in
> to zero hedeleg. As such the hs_force_except mechanism can be removed.
This can be used as commit message for patch 3/4.
Best Regards,
Zhiwei
>
> Signed-off-by: Jose Martins <josemartins90@gmail.com>
> ---
> This version of the patch also removes the uneeded hs_force_except
> functions, variables and macro.
>
> target/riscv/cpu.h | 2 --
> target/riscv/cpu_bits.h | 6 -----
> target/riscv/cpu_helper.c | 54 +++++++--------------------------------
> 3 files changed, 9 insertions(+), 53 deletions(-)
>
> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h
> index 0a33d387ba..a30a64241a 100644
> --- a/target/riscv/cpu.h
> +++ b/target/riscv/cpu.h
> @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request);
> bool riscv_cpu_fp_enabled(CPURISCVState *env);
> 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(int mmu_idx);
> int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch);
> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr);
> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h
> index caf4599207..7322f54157 100644
> --- a/target/riscv/cpu_bits.h
> +++ b/target/riscv/cpu_bits.h
> @@ -462,12 +462,6 @@
>
> /* Virtulisation Register Fields */
> #define VIRT_ONOFF 1
> -/* This is used to save state for when we take an exception. If this is set
> - * that means that we want to force a HS level exception (no matter what the
> - * delegation is set to). This will occur for things such as a second level
> - * page table fault.
> - */
> -#define FORCE_HS_EXCEP 2
>
> /* RV32 satp CSR field masks */
> #define SATP32_MODE 0x80000000
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index 21c54ef561..babe3d844b 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch)
> #ifndef CONFIG_USER_ONLY
> static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> {
> - target_ulong irqs;
> + target_ulong virt_enabled = riscv_cpu_virt_enabled(env);
>
> target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE);
> target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE);
> - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE);
>
> - target_ulong pending = env->mip & env->mie &
> - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP);
> - target_ulong vspending = (env->mip & env->mie &
> - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
> + target_ulong pending = env->mip & env->mie;
>
> target_ulong mie = env->priv < PRV_M ||
> (env->priv == PRV_M && mstatus_mie);
> target_ulong sie = env->priv < PRV_S ||
> (env->priv == PRV_S && mstatus_sie);
> - target_ulong hs_sie = env->priv < PRV_S ||
> - (env->priv == PRV_S && hs_mstatus_sie);
> + target_ulong hsie = virt_enabled || sie;
> + target_ulong vsie = virt_enabled && sie;
>
> - if (riscv_cpu_virt_enabled(env)) {
> - target_ulong pending_hs_irq = pending & -hs_sie;
> -
> - if (pending_hs_irq) {
> - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
> - return ctz64(pending_hs_irq);
> - }
> -
> - pending = vspending;
> - }
> -
> - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie);
> + target_ulong irqs =
> + (pending & ~env->mideleg & -mie) |
> + (pending & env->mideleg & ~env->hideleg & -hsie) |
> + (pending & env->mideleg & env->hideleg & -vsie);
>
> if (irqs) {
> return ctz64(irqs); /* since non-zero */
> @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable)
> env->virt = set_field(env->virt, VIRT_ONOFF, enable);
> }
>
> -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env)
> -{
> - if (!riscv_has_ext(env, RVH)) {
> - return false;
> - }
> -
> - return get_field(env->virt, FORCE_HS_EXCEP);
> -}
> -
> -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable)
> -{
> - if (!riscv_has_ext(env, RVH)) {
> - return;
> - }
> -
> - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable);
> -}
> -
> bool riscv_cpu_two_stage_lookup(int mmu_idx)
> {
> return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK;
> @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>
> RISCVCPU *cpu = RISCV_CPU(cs);
> CPURISCVState *env = &cpu->env;
> - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env);
> uint64_t s;
>
> /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide
> @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> case RISCV_EXCP_INST_GUEST_PAGE_FAULT:
> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT:
> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT:
> - force_hs_execp = true;
> - /* fallthrough */
> case RISCV_EXCP_INST_ADDR_MIS:
> case RISCV_EXCP_INST_ACCESS_FAULT:
> case RISCV_EXCP_LOAD_ADDR_MIS:
> @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0);
> }
>
> - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) &&
> - !force_hs_execp) {
> + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) {
> /* Trap to VS mode */
> /*
> * See if we need to adjust cause. Yes if its VS mode interrupt
> @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
> htval = env->guest_phys_fault_addr;
>
> riscv_cpu_set_virt_enabled(env, 0);
> - riscv_cpu_set_force_hs_excep(env, 0);
> } else {
> /* Trap into HS mode */
> env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false);
> @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs)
>
> /* Trapping to M mode, virt is disabled */
> riscv_cpu_set_virt_enabled(env, 0);
> - riscv_cpu_set_force_hs_excep(env, 0);
> }
>
> s = env->mstatus;
Hello Zhiwei, thank you for reviewing the patch. I'll split the patch in a series as you suggest. But first can you help me understand what the problems are with riscv_cpu_local_irq_pending? > I think there are two errors in riscv_cpu_local_irq_pending. > > 1) VS interrupts can't be forwarded to hs-mode rightly . It has > nothing to do with delegate or not in hideleg. The reason is that > VS interrupts are always discarded when V=0 in > riscv_cpu_local_irq_pending. I don't see why this is the case. The way I see it, VS interrupts are only discarded for V=0 *iff* they are delegated in mideleg/hideleg. I actually tested it and I see the correct forwarding of vs-mode interrupts to hs-mode. I tested it by running in hs-mode with all the needed interrupt enables set, the interrupts not delegated in hideleg, and forcing the trigger of the interrupt by writing hvip. But maybe there are some corner cases I'm not taking into account. Can you explain this further? Maybe walk me through an example of when this issue might occur. > 2) Use MSTATUS_SIE in mstatus_hs to select pending_hs_irqs. I don't think you need to go through mstatus_hs to get the correct sie state. My logic behind this is: env->mstatus will have the vs-level sie if V=1 and hs-level sie if V=0. Due to the short-circuiting property of the logic operators the sie variable will only have an effect on hsie if V=0 and on vsie if V=1. So the value of sie is only used in the correct context. Again, please correct me if I'm wrong. I might be missing something. Best, José
On 5/26/21 7:50 PM, Jose Martins wrote: > Hello Zhiwei, thank you for reviewing the patch. > > I'll split the patch in a series as you suggest. But first can you > help me understand what the problems are with > riscv_cpu_local_irq_pending? > >> I think there are two errors in riscv_cpu_local_irq_pending. >> >> 1) VS interrupts can't be forwarded to hs-mode rightly . It has >> nothing to do with delegate or not in hideleg. The reason is that >> VS interrupts are always discarded when V=0 in >> riscv_cpu_local_irq_pending. > I don't see why this is the case. The way I see it, VS interrupts are > only discarded for V=0 *iff* they are delegated in mideleg/hideleg. First I paste the code to ensure we are talking about the same version. static int riscv_cpu_local_irq_pending(CPURISCVState *env) { target_ulong irqs; target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); target_ulong pending = env->mip & env->mie & ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); target_ulong vspending = (env->mip & env->mie & (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M && mstatus_mie); target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S && mstatus_sie); target_ulong hs_sie = env->priv < PRV_S || (env->priv == PRV_S && hs_mstatus_sie); if (riscv_cpu_virt_enabled(env)) { target_ulong pending_hs_irq = pending & -hs_sie; if (pending_hs_irq) { riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); return ctz64(pending_hs_irq); } pending = vspending; } irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); if (irqs) { return ctz64(irqs); /* since non-zero */ } else { return RISCV_EXCP_NONE; /* indicates no pending interrupt */ } } Only when VS = 0, the variable vspending can transfer to variable pending. Any interrupt not included in variable pending is discarded. That's why I say VS interrupts are always discarded when V=0 in riscv_cpu_local_irq_pending. > I > actually tested it and I see the correct forwarding of vs-mode > interrupts to hs-mode. I tested it by running in hs-mode with all the > needed interrupt enables set, the interrupts not delegated in hideleg, > and forcing the trigger of the interrupt by writing hvip. But maybe > there are some corner cases I'm not taking into account. Can you > explain this further? Maybe walk me through an example of when this > issue might occur. > >> 2) Use MSTATUS_SIE in mstatus_hs to select pending_hs_irqs. I mean the second error is to misuse MSATUS_SIE in mstatus_hs to select pending_hs_irqs. > I don't think you need to go through mstatus_hs to get the correct sie > state. Agree. > My logic behind this is: env->mstatus will have the vs-level > sie if V=1 and hs-level sie if V=0. Due to the short-circuiting > property of the logic operators the sie variable will only have an > effect on hsie if V=0 and on vsie if V=1. So the value of sie is only > used in the correct context. The swap regs funciton has done the right thing. I think V mode and hideleg/mideleg make it possible to process VS interrupt or HS interrupt like other interrupts. Zhiwei > > Again, please correct me if I'm wrong. I might be missing something. > > Best, > José
On Sun, May 23, 2021 at 1:45 AM Jose Martins <josemartins90@gmail.com> wrote: > > VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when > not delegated in hideleg (which was not being taken into account). This > was mainly because hs level sie was not always considered enabled when > it should. The spec states that "Interrupts for higher-privilege modes, > y>x, are always globally enabled regardless of the setting of the global > yIE bit for the higher-privilege mode." and also "For purposes of > interrupt global enables, HS-mode is considered more privileged than > VS-mode, and VS-mode is considered more privileged than VU-mode". > > These interrupts should be treated the same as any other kind of > exception. Therefore, there is no need to "force an hs exception" as the > current privilege level, the state of the global ie and of the > delegation registers should be enough to route the interrupt to the > appropriate privilege level in riscv_cpu_do_interrupt. Also, these > interrupts never target m-mode, which is guaranteed by the hardwiring > of the corresponding bits in mideleg. The same is true for synchronous > exceptions, specifically, guest page faults which must be hardwired in > to zero hedeleg. As such the hs_force_except mechanism can be removed. > > Signed-off-by: Jose Martins <josemartins90@gmail.com> This looks right to me, but this was one of the most confusing parts of the implementation. I also don't think the patch is too long as it's mostly just deleting stuff. I don't fully understand your concerns Zhiwei, do you mind stating them again? Alistair > --- > This version of the patch also removes the uneeded hs_force_except > functions, variables and macro. > > target/riscv/cpu.h | 2 -- > target/riscv/cpu_bits.h | 6 ----- > target/riscv/cpu_helper.c | 54 +++++++-------------------------------- > 3 files changed, 9 insertions(+), 53 deletions(-) > > diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > index 0a33d387ba..a30a64241a 100644 > --- a/target/riscv/cpu.h > +++ b/target/riscv/cpu.h > @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); > bool riscv_cpu_fp_enabled(CPURISCVState *env); > 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(int mmu_idx); > int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); > hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > index caf4599207..7322f54157 100644 > --- a/target/riscv/cpu_bits.h > +++ b/target/riscv/cpu_bits.h > @@ -462,12 +462,6 @@ > > /* Virtulisation Register Fields */ > #define VIRT_ONOFF 1 > -/* This is used to save state for when we take an exception. If this is set > - * that means that we want to force a HS level exception (no matter what the > - * delegation is set to). This will occur for things such as a second level > - * page table fault. > - */ > -#define FORCE_HS_EXCEP 2 > > /* RV32 satp CSR field masks */ > #define SATP32_MODE 0x80000000 > diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > index 21c54ef561..babe3d844b 100644 > --- a/target/riscv/cpu_helper.c > +++ b/target/riscv/cpu_helper.c > @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > #ifndef CONFIG_USER_ONLY > static int riscv_cpu_local_irq_pending(CPURISCVState *env) > { > - target_ulong irqs; > + target_ulong virt_enabled = riscv_cpu_virt_enabled(env); > > target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); > target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); > - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); > > - target_ulong pending = env->mip & env->mie & > - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); > - target_ulong vspending = (env->mip & env->mie & > - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); > + target_ulong pending = env->mip & env->mie; > > target_ulong mie = env->priv < PRV_M || > (env->priv == PRV_M && mstatus_mie); > target_ulong sie = env->priv < PRV_S || > (env->priv == PRV_S && mstatus_sie); > - target_ulong hs_sie = env->priv < PRV_S || > - (env->priv == PRV_S && hs_mstatus_sie); > + target_ulong hsie = virt_enabled || sie; > + target_ulong vsie = virt_enabled && sie; > > - if (riscv_cpu_virt_enabled(env)) { > - target_ulong pending_hs_irq = pending & -hs_sie; > - > - if (pending_hs_irq) { > - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); > - return ctz64(pending_hs_irq); > - } > - > - pending = vspending; > - } > - > - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); > + target_ulong irqs = > + (pending & ~env->mideleg & -mie) | > + (pending & env->mideleg & ~env->hideleg & -hsie) | > + (pending & env->mideleg & env->hideleg & -vsie); > > if (irqs) { > return ctz64(irqs); /* since non-zero */ > @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable) > env->virt = set_field(env->virt, VIRT_ONOFF, enable); > } > > -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env) > -{ > - if (!riscv_has_ext(env, RVH)) { > - return false; > - } > - > - return get_field(env->virt, FORCE_HS_EXCEP); > -} > - > -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable) > -{ > - if (!riscv_has_ext(env, RVH)) { > - return; > - } > - > - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable); > -} > - > bool riscv_cpu_two_stage_lookup(int mmu_idx) > { > return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK; > @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > RISCVCPU *cpu = RISCV_CPU(cs); > CPURISCVState *env = &cpu->env; > - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env); > uint64_t s; > > /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > case RISCV_EXCP_INST_GUEST_PAGE_FAULT: > case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: > case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: > - force_hs_execp = true; > - /* fallthrough */ > case RISCV_EXCP_INST_ADDR_MIS: > case RISCV_EXCP_INST_ACCESS_FAULT: > case RISCV_EXCP_LOAD_ADDR_MIS: > @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > } > > - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) && > - !force_hs_execp) { > + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > /* Trap to VS mode */ > /* > * See if we need to adjust cause. Yes if its VS mode interrupt > @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > htval = env->guest_phys_fault_addr; > > riscv_cpu_set_virt_enabled(env, 0); > - riscv_cpu_set_force_hs_excep(env, 0); > } else { > /* Trap into HS mode */ > env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > /* Trapping to M mode, virt is disabled */ > riscv_cpu_set_virt_enabled(env, 0); > - riscv_cpu_set_force_hs_excep(env, 0); > } > > s = env->mstatus; > -- > 2.30.2 > >
On 5/28/21 6:34 AM, Alistair Francis wrote: > On Sun, May 23, 2021 at 1:45 AM Jose Martins <josemartins90@gmail.com> wrote: >> VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when >> not delegated in hideleg (which was not being taken into account). This >> was mainly because hs level sie was not always considered enabled when >> it should. The spec states that "Interrupts for higher-privilege modes, >> y>x, are always globally enabled regardless of the setting of the global >> yIE bit for the higher-privilege mode." and also "For purposes of >> interrupt global enables, HS-mode is considered more privileged than >> VS-mode, and VS-mode is considered more privileged than VU-mode". >> >> These interrupts should be treated the same as any other kind of >> exception. Therefore, there is no need to "force an hs exception" as the >> current privilege level, the state of the global ie and of the >> delegation registers should be enough to route the interrupt to the >> appropriate privilege level in riscv_cpu_do_interrupt. Also, these >> interrupts never target m-mode, which is guaranteed by the hardwiring >> of the corresponding bits in mideleg. The same is true for synchronous >> exceptions, specifically, guest page faults which must be hardwired in >> to zero hedeleg. As such the hs_force_except mechanism can be removed. >> >> Signed-off-by: Jose Martins <josemartins90@gmail.com> > This looks right to me, but this was one of the most confusing parts > of the implementation. I also don't think the patch is too long as > it's mostly just deleting stuff. > > I don't fully understand your concerns Zhiwei, do you mind stating them again? Hi Alistair, My main concern is the commit message is to complex. I also have a question why force hs exception in current code. Then we can give a brief commit message. Best Regards, Zhiwei > > Alistair > >> --- >> This version of the patch also removes the uneeded hs_force_except >> functions, variables and macro. >> >> target/riscv/cpu.h | 2 -- >> target/riscv/cpu_bits.h | 6 ----- >> target/riscv/cpu_helper.c | 54 +++++++-------------------------------- >> 3 files changed, 9 insertions(+), 53 deletions(-) >> >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h >> index 0a33d387ba..a30a64241a 100644 >> --- a/target/riscv/cpu.h >> +++ b/target/riscv/cpu.h >> @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); >> bool riscv_cpu_fp_enabled(CPURISCVState *env); >> 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(int mmu_idx); >> int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); >> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h >> index caf4599207..7322f54157 100644 >> --- a/target/riscv/cpu_bits.h >> +++ b/target/riscv/cpu_bits.h >> @@ -462,12 +462,6 @@ >> >> /* Virtulisation Register Fields */ >> #define VIRT_ONOFF 1 >> -/* This is used to save state for when we take an exception. If this is set >> - * that means that we want to force a HS level exception (no matter what the >> - * delegation is set to). This will occur for things such as a second level >> - * page table fault. >> - */ >> -#define FORCE_HS_EXCEP 2 >> >> /* RV32 satp CSR field masks */ >> #define SATP32_MODE 0x80000000 >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c >> index 21c54ef561..babe3d844b 100644 >> --- a/target/riscv/cpu_helper.c >> +++ b/target/riscv/cpu_helper.c >> @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) >> #ifndef CONFIG_USER_ONLY >> static int riscv_cpu_local_irq_pending(CPURISCVState *env) >> { >> - target_ulong irqs; >> + target_ulong virt_enabled = riscv_cpu_virt_enabled(env); >> >> target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); >> target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); >> - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); >> >> - target_ulong pending = env->mip & env->mie & >> - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); >> - target_ulong vspending = (env->mip & env->mie & >> - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); >> + target_ulong pending = env->mip & env->mie; >> >> target_ulong mie = env->priv < PRV_M || >> (env->priv == PRV_M && mstatus_mie); >> target_ulong sie = env->priv < PRV_S || >> (env->priv == PRV_S && mstatus_sie); >> - target_ulong hs_sie = env->priv < PRV_S || >> - (env->priv == PRV_S && hs_mstatus_sie); >> + target_ulong hsie = virt_enabled || sie; >> + target_ulong vsie = virt_enabled && sie; >> >> - if (riscv_cpu_virt_enabled(env)) { >> - target_ulong pending_hs_irq = pending & -hs_sie; >> - >> - if (pending_hs_irq) { >> - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); >> - return ctz64(pending_hs_irq); >> - } >> - >> - pending = vspending; >> - } >> - >> - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); >> + target_ulong irqs = >> + (pending & ~env->mideleg & -mie) | >> + (pending & env->mideleg & ~env->hideleg & -hsie) | >> + (pending & env->mideleg & env->hideleg & -vsie); >> >> if (irqs) { >> return ctz64(irqs); /* since non-zero */ >> @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable) >> env->virt = set_field(env->virt, VIRT_ONOFF, enable); >> } >> >> -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env) >> -{ >> - if (!riscv_has_ext(env, RVH)) { >> - return false; >> - } >> - >> - return get_field(env->virt, FORCE_HS_EXCEP); >> -} >> - >> -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable) >> -{ >> - if (!riscv_has_ext(env, RVH)) { >> - return; >> - } >> - >> - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable); >> -} >> - >> bool riscv_cpu_two_stage_lookup(int mmu_idx) >> { >> return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK; >> @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) >> >> RISCVCPU *cpu = RISCV_CPU(cs); >> CPURISCVState *env = &cpu->env; >> - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env); >> uint64_t s; >> >> /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide >> @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) >> case RISCV_EXCP_INST_GUEST_PAGE_FAULT: >> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: >> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: >> - force_hs_execp = true; >> - /* fallthrough */ >> case RISCV_EXCP_INST_ADDR_MIS: >> case RISCV_EXCP_INST_ACCESS_FAULT: >> case RISCV_EXCP_LOAD_ADDR_MIS: >> @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) >> env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); >> } >> >> - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) && >> - !force_hs_execp) { >> + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { >> /* Trap to VS mode */ >> /* >> * See if we need to adjust cause. Yes if its VS mode interrupt >> @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) >> htval = env->guest_phys_fault_addr; >> >> riscv_cpu_set_virt_enabled(env, 0); >> - riscv_cpu_set_force_hs_excep(env, 0); >> } else { >> /* Trap into HS mode */ >> env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); >> @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) >> >> /* Trapping to M mode, virt is disabled */ >> riscv_cpu_set_virt_enabled(env, 0); >> - riscv_cpu_set_force_hs_excep(env, 0); >> } >> >> s = env->mstatus; >> -- >> 2.30.2 >> >>
Hello Zhiwei and Alistair, I went for a middle-ground solution. I divided the patch into two: one fixes the vs interrupt forwarding to the hypervisor, the other removes the unnecessary force exception stuff. I just submitted the patch series. I hope it's ok with you. José On Fri, 28 May 2021 at 01:36, LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > > On 5/28/21 6:34 AM, Alistair Francis wrote: > > On Sun, May 23, 2021 at 1:45 AM Jose Martins <josemartins90@gmail.com> wrote: > >> VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when > >> not delegated in hideleg (which was not being taken into account). This > >> was mainly because hs level sie was not always considered enabled when > >> it should. The spec states that "Interrupts for higher-privilege modes, > >> y>x, are always globally enabled regardless of the setting of the global > >> yIE bit for the higher-privilege mode." and also "For purposes of > >> interrupt global enables, HS-mode is considered more privileged than > >> VS-mode, and VS-mode is considered more privileged than VU-mode". > >> > >> These interrupts should be treated the same as any other kind of > >> exception. Therefore, there is no need to "force an hs exception" as the > >> current privilege level, the state of the global ie and of the > >> delegation registers should be enough to route the interrupt to the > >> appropriate privilege level in riscv_cpu_do_interrupt. Also, these > >> interrupts never target m-mode, which is guaranteed by the hardwiring > >> of the corresponding bits in mideleg. The same is true for synchronous > >> exceptions, specifically, guest page faults which must be hardwired in > >> to zero hedeleg. As such the hs_force_except mechanism can be removed. > >> > >> Signed-off-by: Jose Martins <josemartins90@gmail.com> > > This looks right to me, but this was one of the most confusing parts > > of the implementation. I also don't think the patch is too long as > > it's mostly just deleting stuff. > > > > I don't fully understand your concerns Zhiwei, do you mind stating them again? > > Hi Alistair, > > My main concern is the commit message is to complex. > > I also have a question why force hs exception in current code. > Then we can give a brief commit message. > > Best Regards, > Zhiwei > > > > > Alistair > > > >> --- > >> This version of the patch also removes the uneeded hs_force_except > >> functions, variables and macro. > >> > >> target/riscv/cpu.h | 2 -- > >> target/riscv/cpu_bits.h | 6 ----- > >> target/riscv/cpu_helper.c | 54 +++++++-------------------------------- > >> 3 files changed, 9 insertions(+), 53 deletions(-) > >> > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > >> index 0a33d387ba..a30a64241a 100644 > >> --- a/target/riscv/cpu.h > >> +++ b/target/riscv/cpu.h > >> @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); > >> bool riscv_cpu_fp_enabled(CPURISCVState *env); > >> 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(int mmu_idx); > >> int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); > >> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > >> index caf4599207..7322f54157 100644 > >> --- a/target/riscv/cpu_bits.h > >> +++ b/target/riscv/cpu_bits.h > >> @@ -462,12 +462,6 @@ > >> > >> /* Virtulisation Register Fields */ > >> #define VIRT_ONOFF 1 > >> -/* This is used to save state for when we take an exception. If this is set > >> - * that means that we want to force a HS level exception (no matter what the > >> - * delegation is set to). This will occur for things such as a second level > >> - * page table fault. > >> - */ > >> -#define FORCE_HS_EXCEP 2 > >> > >> /* RV32 satp CSR field masks */ > >> #define SATP32_MODE 0x80000000 > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > >> index 21c54ef561..babe3d844b 100644 > >> --- a/target/riscv/cpu_helper.c > >> +++ b/target/riscv/cpu_helper.c > >> @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > >> #ifndef CONFIG_USER_ONLY > >> static int riscv_cpu_local_irq_pending(CPURISCVState *env) > >> { > >> - target_ulong irqs; > >> + target_ulong virt_enabled = riscv_cpu_virt_enabled(env); > >> > >> target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); > >> target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); > >> - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); > >> > >> - target_ulong pending = env->mip & env->mie & > >> - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); > >> - target_ulong vspending = (env->mip & env->mie & > >> - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); > >> + target_ulong pending = env->mip & env->mie; > >> > >> target_ulong mie = env->priv < PRV_M || > >> (env->priv == PRV_M && mstatus_mie); > >> target_ulong sie = env->priv < PRV_S || > >> (env->priv == PRV_S && mstatus_sie); > >> - target_ulong hs_sie = env->priv < PRV_S || > >> - (env->priv == PRV_S && hs_mstatus_sie); > >> + target_ulong hsie = virt_enabled || sie; > >> + target_ulong vsie = virt_enabled && sie; > >> > >> - if (riscv_cpu_virt_enabled(env)) { > >> - target_ulong pending_hs_irq = pending & -hs_sie; > >> - > >> - if (pending_hs_irq) { > >> - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); > >> - return ctz64(pending_hs_irq); > >> - } > >> - > >> - pending = vspending; > >> - } > >> - > >> - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); > >> + target_ulong irqs = > >> + (pending & ~env->mideleg & -mie) | > >> + (pending & env->mideleg & ~env->hideleg & -hsie) | > >> + (pending & env->mideleg & env->hideleg & -vsie); > >> > >> if (irqs) { > >> return ctz64(irqs); /* since non-zero */ > >> @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable) > >> env->virt = set_field(env->virt, VIRT_ONOFF, enable); > >> } > >> > >> -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env) > >> -{ > >> - if (!riscv_has_ext(env, RVH)) { > >> - return false; > >> - } > >> - > >> - return get_field(env->virt, FORCE_HS_EXCEP); > >> -} > >> - > >> -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable) > >> -{ > >> - if (!riscv_has_ext(env, RVH)) { > >> - return; > >> - } > >> - > >> - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable); > >> -} > >> - > >> bool riscv_cpu_two_stage_lookup(int mmu_idx) > >> { > >> return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK; > >> @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > >> > >> RISCVCPU *cpu = RISCV_CPU(cs); > >> CPURISCVState *env = &cpu->env; > >> - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env); > >> uint64_t s; > >> > >> /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > >> @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > >> case RISCV_EXCP_INST_GUEST_PAGE_FAULT: > >> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: > >> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: > >> - force_hs_execp = true; > >> - /* fallthrough */ > >> case RISCV_EXCP_INST_ADDR_MIS: > >> case RISCV_EXCP_INST_ACCESS_FAULT: > >> case RISCV_EXCP_LOAD_ADDR_MIS: > >> @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > >> env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > >> } > >> > >> - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) && > >> - !force_hs_execp) { > >> + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > >> /* Trap to VS mode */ > >> /* > >> * See if we need to adjust cause. Yes if its VS mode interrupt > >> @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > >> htval = env->guest_phys_fault_addr; > >> > >> riscv_cpu_set_virt_enabled(env, 0); > >> - riscv_cpu_set_force_hs_excep(env, 0); > >> } else { > >> /* Trap into HS mode */ > >> env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > >> @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > >> > >> /* Trapping to M mode, virt is disabled */ > >> riscv_cpu_set_virt_enabled(env, 0); > >> - riscv_cpu_set_force_hs_excep(env, 0); > >> } > >> > >> s = env->mstatus; > >> -- > >> 2.30.2 > >> > >>
Hello Zhiwei and Alistair, I noticed this patch did not make it upstream, contrarily to a couple other patches I submitted around the same time. Is there something else needed from my side to push this forward? Best, José On Wed, 2 Jun 2021 at 20:14, Jose Martins <josemartins90@gmail.com> wrote: > > Hello Zhiwei and Alistair, > > I went for a middle-ground solution. I divided the patch into two: one > fixes the vs interrupt forwarding to the hypervisor, the other removes > the unnecessary force exception stuff. I just submitted the patch > series. I hope it's ok with you. > > José > > On Fri, 28 May 2021 at 01:36, LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > > > > > On 5/28/21 6:34 AM, Alistair Francis wrote: > > > On Sun, May 23, 2021 at 1:45 AM Jose Martins <josemartins90@gmail.com> wrote: > > >> VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when > > >> not delegated in hideleg (which was not being taken into account). This > > >> was mainly because hs level sie was not always considered enabled when > > >> it should. The spec states that "Interrupts for higher-privilege modes, > > >> y>x, are always globally enabled regardless of the setting of the global > > >> yIE bit for the higher-privilege mode." and also "For purposes of > > >> interrupt global enables, HS-mode is considered more privileged than > > >> VS-mode, and VS-mode is considered more privileged than VU-mode". > > >> > > >> These interrupts should be treated the same as any other kind of > > >> exception. Therefore, there is no need to "force an hs exception" as the > > >> current privilege level, the state of the global ie and of the > > >> delegation registers should be enough to route the interrupt to the > > >> appropriate privilege level in riscv_cpu_do_interrupt. Also, these > > >> interrupts never target m-mode, which is guaranteed by the hardwiring > > >> of the corresponding bits in mideleg. The same is true for synchronous > > >> exceptions, specifically, guest page faults which must be hardwired in > > >> to zero hedeleg. As such the hs_force_except mechanism can be removed. > > >> > > >> Signed-off-by: Jose Martins <josemartins90@gmail.com> > > > This looks right to me, but this was one of the most confusing parts > > > of the implementation. I also don't think the patch is too long as > > > it's mostly just deleting stuff. > > > > > > I don't fully understand your concerns Zhiwei, do you mind stating them again? > > > > Hi Alistair, > > > > My main concern is the commit message is to complex. > > > > I also have a question why force hs exception in current code. > > Then we can give a brief commit message. > > > > Best Regards, > > Zhiwei > > > > > > > > Alistair > > > > > >> --- > > >> This version of the patch also removes the uneeded hs_force_except > > >> functions, variables and macro. > > >> > > >> target/riscv/cpu.h | 2 -- > > >> target/riscv/cpu_bits.h | 6 ----- > > >> target/riscv/cpu_helper.c | 54 +++++++-------------------------------- > > >> 3 files changed, 9 insertions(+), 53 deletions(-) > > >> > > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > >> index 0a33d387ba..a30a64241a 100644 > > >> --- a/target/riscv/cpu.h > > >> +++ b/target/riscv/cpu.h > > >> @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); > > >> bool riscv_cpu_fp_enabled(CPURISCVState *env); > > >> 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(int mmu_idx); > > >> int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); > > >> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > > >> index caf4599207..7322f54157 100644 > > >> --- a/target/riscv/cpu_bits.h > > >> +++ b/target/riscv/cpu_bits.h > > >> @@ -462,12 +462,6 @@ > > >> > > >> /* Virtulisation Register Fields */ > > >> #define VIRT_ONOFF 1 > > >> -/* This is used to save state for when we take an exception. If this is set > > >> - * that means that we want to force a HS level exception (no matter what the > > >> - * delegation is set to). This will occur for things such as a second level > > >> - * page table fault. > > >> - */ > > >> -#define FORCE_HS_EXCEP 2 > > >> > > >> /* RV32 satp CSR field masks */ > > >> #define SATP32_MODE 0x80000000 > > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > >> index 21c54ef561..babe3d844b 100644 > > >> --- a/target/riscv/cpu_helper.c > > >> +++ b/target/riscv/cpu_helper.c > > >> @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > >> #ifndef CONFIG_USER_ONLY > > >> static int riscv_cpu_local_irq_pending(CPURISCVState *env) > > >> { > > >> - target_ulong irqs; > > >> + target_ulong virt_enabled = riscv_cpu_virt_enabled(env); > > >> > > >> target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); > > >> target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); > > >> - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); > > >> > > >> - target_ulong pending = env->mip & env->mie & > > >> - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); > > >> - target_ulong vspending = (env->mip & env->mie & > > >> - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); > > >> + target_ulong pending = env->mip & env->mie; > > >> > > >> target_ulong mie = env->priv < PRV_M || > > >> (env->priv == PRV_M && mstatus_mie); > > >> target_ulong sie = env->priv < PRV_S || > > >> (env->priv == PRV_S && mstatus_sie); > > >> - target_ulong hs_sie = env->priv < PRV_S || > > >> - (env->priv == PRV_S && hs_mstatus_sie); > > >> + target_ulong hsie = virt_enabled || sie; > > >> + target_ulong vsie = virt_enabled && sie; > > >> > > >> - if (riscv_cpu_virt_enabled(env)) { > > >> - target_ulong pending_hs_irq = pending & -hs_sie; > > >> - > > >> - if (pending_hs_irq) { > > >> - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); > > >> - return ctz64(pending_hs_irq); > > >> - } > > >> - > > >> - pending = vspending; > > >> - } > > >> - > > >> - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); > > >> + target_ulong irqs = > > >> + (pending & ~env->mideleg & -mie) | > > >> + (pending & env->mideleg & ~env->hideleg & -hsie) | > > >> + (pending & env->mideleg & env->hideleg & -vsie); > > >> > > >> if (irqs) { > > >> return ctz64(irqs); /* since non-zero */ > > >> @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable) > > >> env->virt = set_field(env->virt, VIRT_ONOFF, enable); > > >> } > > >> > > >> -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env) > > >> -{ > > >> - if (!riscv_has_ext(env, RVH)) { > > >> - return false; > > >> - } > > >> - > > >> - return get_field(env->virt, FORCE_HS_EXCEP); > > >> -} > > >> - > > >> -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable) > > >> -{ > > >> - if (!riscv_has_ext(env, RVH)) { > > >> - return; > > >> - } > > >> - > > >> - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable); > > >> -} > > >> - > > >> bool riscv_cpu_two_stage_lookup(int mmu_idx) > > >> { > > >> return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK; > > >> @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > >> > > >> RISCVCPU *cpu = RISCV_CPU(cs); > > >> CPURISCVState *env = &cpu->env; > > >> - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env); > > >> uint64_t s; > > >> > > >> /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > > >> @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > >> case RISCV_EXCP_INST_GUEST_PAGE_FAULT: > > >> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: > > >> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: > > >> - force_hs_execp = true; > > >> - /* fallthrough */ > > >> case RISCV_EXCP_INST_ADDR_MIS: > > >> case RISCV_EXCP_INST_ACCESS_FAULT: > > >> case RISCV_EXCP_LOAD_ADDR_MIS: > > >> @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > >> env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > > >> } > > >> > > >> - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) && > > >> - !force_hs_execp) { > > >> + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > > >> /* Trap to VS mode */ > > >> /* > > >> * See if we need to adjust cause. Yes if its VS mode interrupt > > >> @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > >> htval = env->guest_phys_fault_addr; > > >> > > >> riscv_cpu_set_virt_enabled(env, 0); > > >> - riscv_cpu_set_force_hs_excep(env, 0); > > >> } else { > > >> /* Trap into HS mode */ > > >> env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > > >> @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > >> > > >> /* Trapping to M mode, virt is disabled */ > > >> riscv_cpu_set_virt_enabled(env, 0); > > >> - riscv_cpu_set_force_hs_excep(env, 0); > > >> } > > >> > > >> s = env->mstatus; > > >> -- > > >> 2.30.2 > > >> > > >>
On Mon, Oct 18, 2021 at 6:30 AM Jose Martins <josemartins90@gmail.com> wrote: > > Hello Zhiwei and Alistair, > > I noticed this patch did not make it upstream, contrarily to a couple > other patches I submitted around the same time. Is there something > else needed from my side to push this forward? From your last response I thought you sent a different series that replaces this patch. If that's not the case do you mind sending this patch again? Alistair > > Best, > José > > On Wed, 2 Jun 2021 at 20:14, Jose Martins <josemartins90@gmail.com> wrote: > > > > Hello Zhiwei and Alistair, > > > > I went for a middle-ground solution. I divided the patch into two: one > > fixes the vs interrupt forwarding to the hypervisor, the other removes > > the unnecessary force exception stuff. I just submitted the patch > > series. I hope it's ok with you. > > > > José > > > > On Fri, 28 May 2021 at 01:36, LIU Zhiwei <zhiwei_liu@c-sky.com> wrote: > > > > > > > > > On 5/28/21 6:34 AM, Alistair Francis wrote: > > > > On Sun, May 23, 2021 at 1:45 AM Jose Martins <josemartins90@gmail.com> wrote: > > > >> VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when > > > >> not delegated in hideleg (which was not being taken into account). This > > > >> was mainly because hs level sie was not always considered enabled when > > > >> it should. The spec states that "Interrupts for higher-privilege modes, > > > >> y>x, are always globally enabled regardless of the setting of the global > > > >> yIE bit for the higher-privilege mode." and also "For purposes of > > > >> interrupt global enables, HS-mode is considered more privileged than > > > >> VS-mode, and VS-mode is considered more privileged than VU-mode". > > > >> > > > >> These interrupts should be treated the same as any other kind of > > > >> exception. Therefore, there is no need to "force an hs exception" as the > > > >> current privilege level, the state of the global ie and of the > > > >> delegation registers should be enough to route the interrupt to the > > > >> appropriate privilege level in riscv_cpu_do_interrupt. Also, these > > > >> interrupts never target m-mode, which is guaranteed by the hardwiring > > > >> of the corresponding bits in mideleg. The same is true for synchronous > > > >> exceptions, specifically, guest page faults which must be hardwired in > > > >> to zero hedeleg. As such the hs_force_except mechanism can be removed. > > > >> > > > >> Signed-off-by: Jose Martins <josemartins90@gmail.com> > > > > This looks right to me, but this was one of the most confusing parts > > > > of the implementation. I also don't think the patch is too long as > > > > it's mostly just deleting stuff. > > > > > > > > I don't fully understand your concerns Zhiwei, do you mind stating them again? > > > > > > Hi Alistair, > > > > > > My main concern is the commit message is to complex. > > > > > > I also have a question why force hs exception in current code. > > > Then we can give a brief commit message. > > > > > > Best Regards, > > > Zhiwei > > > > > > > > > > > Alistair > > > > > > > >> --- > > > >> This version of the patch also removes the uneeded hs_force_except > > > >> functions, variables and macro. > > > >> > > > >> target/riscv/cpu.h | 2 -- > > > >> target/riscv/cpu_bits.h | 6 ----- > > > >> target/riscv/cpu_helper.c | 54 +++++++-------------------------------- > > > >> 3 files changed, 9 insertions(+), 53 deletions(-) > > > >> > > > >> diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h > > > >> index 0a33d387ba..a30a64241a 100644 > > > >> --- a/target/riscv/cpu.h > > > >> +++ b/target/riscv/cpu.h > > > >> @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); > > > >> bool riscv_cpu_fp_enabled(CPURISCVState *env); > > > >> 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(int mmu_idx); > > > >> int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); > > > >> hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); > > > >> diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h > > > >> index caf4599207..7322f54157 100644 > > > >> --- a/target/riscv/cpu_bits.h > > > >> +++ b/target/riscv/cpu_bits.h > > > >> @@ -462,12 +462,6 @@ > > > >> > > > >> /* Virtulisation Register Fields */ > > > >> #define VIRT_ONOFF 1 > > > >> -/* This is used to save state for when we take an exception. If this is set > > > >> - * that means that we want to force a HS level exception (no matter what the > > > >> - * delegation is set to). This will occur for things such as a second level > > > >> - * page table fault. > > > >> - */ > > > >> -#define FORCE_HS_EXCEP 2 > > > >> > > > >> /* RV32 satp CSR field masks */ > > > >> #define SATP32_MODE 0x80000000 > > > >> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c > > > >> index 21c54ef561..babe3d844b 100644 > > > >> --- a/target/riscv/cpu_helper.c > > > >> +++ b/target/riscv/cpu_helper.c > > > >> @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) > > > >> #ifndef CONFIG_USER_ONLY > > > >> static int riscv_cpu_local_irq_pending(CPURISCVState *env) > > > >> { > > > >> - target_ulong irqs; > > > >> + target_ulong virt_enabled = riscv_cpu_virt_enabled(env); > > > >> > > > >> target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); > > > >> target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); > > > >> - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); > > > >> > > > >> - target_ulong pending = env->mip & env->mie & > > > >> - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); > > > >> - target_ulong vspending = (env->mip & env->mie & > > > >> - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); > > > >> + target_ulong pending = env->mip & env->mie; > > > >> > > > >> target_ulong mie = env->priv < PRV_M || > > > >> (env->priv == PRV_M && mstatus_mie); > > > >> target_ulong sie = env->priv < PRV_S || > > > >> (env->priv == PRV_S && mstatus_sie); > > > >> - target_ulong hs_sie = env->priv < PRV_S || > > > >> - (env->priv == PRV_S && hs_mstatus_sie); > > > >> + target_ulong hsie = virt_enabled || sie; > > > >> + target_ulong vsie = virt_enabled && sie; > > > >> > > > >> - if (riscv_cpu_virt_enabled(env)) { > > > >> - target_ulong pending_hs_irq = pending & -hs_sie; > > > >> - > > > >> - if (pending_hs_irq) { > > > >> - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); > > > >> - return ctz64(pending_hs_irq); > > > >> - } > > > >> - > > > >> - pending = vspending; > > > >> - } > > > >> - > > > >> - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); > > > >> + target_ulong irqs = > > > >> + (pending & ~env->mideleg & -mie) | > > > >> + (pending & env->mideleg & ~env->hideleg & -hsie) | > > > >> + (pending & env->mideleg & env->hideleg & -vsie); > > > >> > > > >> if (irqs) { > > > >> return ctz64(irqs); /* since non-zero */ > > > >> @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable) > > > >> env->virt = set_field(env->virt, VIRT_ONOFF, enable); > > > >> } > > > >> > > > >> -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env) > > > >> -{ > > > >> - if (!riscv_has_ext(env, RVH)) { > > > >> - return false; > > > >> - } > > > >> - > > > >> - return get_field(env->virt, FORCE_HS_EXCEP); > > > >> -} > > > >> - > > > >> -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable) > > > >> -{ > > > >> - if (!riscv_has_ext(env, RVH)) { > > > >> - return; > > > >> - } > > > >> - > > > >> - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable); > > > >> -} > > > >> - > > > >> bool riscv_cpu_two_stage_lookup(int mmu_idx) > > > >> { > > > >> return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK; > > > >> @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > >> > > > >> RISCVCPU *cpu = RISCV_CPU(cs); > > > >> CPURISCVState *env = &cpu->env; > > > >> - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env); > > > >> uint64_t s; > > > >> > > > >> /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide > > > >> @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > >> case RISCV_EXCP_INST_GUEST_PAGE_FAULT: > > > >> case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: > > > >> case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: > > > >> - force_hs_execp = true; > > > >> - /* fallthrough */ > > > >> case RISCV_EXCP_INST_ADDR_MIS: > > > >> case RISCV_EXCP_INST_ACCESS_FAULT: > > > >> case RISCV_EXCP_LOAD_ADDR_MIS: > > > >> @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > >> env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); > > > >> } > > > >> > > > >> - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) && > > > >> - !force_hs_execp) { > > > >> + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { > > > >> /* Trap to VS mode */ > > > >> /* > > > >> * See if we need to adjust cause. Yes if its VS mode interrupt > > > >> @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > >> htval = env->guest_phys_fault_addr; > > > >> > > > >> riscv_cpu_set_virt_enabled(env, 0); > > > >> - riscv_cpu_set_force_hs_excep(env, 0); > > > >> } else { > > > >> /* Trap into HS mode */ > > > >> env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); > > > >> @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) > > > >> > > > >> /* Trapping to M mode, virt is disabled */ > > > >> riscv_cpu_set_virt_enabled(env, 0); > > > >> - riscv_cpu_set_force_hs_excep(env, 0); > > > >> } > > > >> > > > >> s = env->mstatus; > > > >> -- > > > >> 2.30.2 > > > >> > > > >>
> From your last response I thought you sent a different series that > replaces this patch. If that's not the case do you mind sending this > patch again? I already sent the patch series here: https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg00553.html. I got confused, I should have raised this issue in that thread. Let me know if you still want me to resend it. Jose
On Mon, Oct 25, 2021 at 7:48 PM Jose Martins <josemartins90@gmail.com> wrote: > > > From your last response I thought you sent a different series that > > replaces this patch. If that's not the case do you mind sending this > > patch again? > > I already sent the patch series here: > https://lists.gnu.org/archive/html/qemu-devel/2021-06/msg00553.html. I > got confused, I should have raised this issue in that thread. Let me > know if you still want me to resend it. Weird, I somehow lost them. Can you please re-send them and I'll apply them. Alistair > > Jose
diff --git a/target/riscv/cpu.h b/target/riscv/cpu.h index 0a33d387ba..a30a64241a 100644 --- a/target/riscv/cpu.h +++ b/target/riscv/cpu.h @@ -337,8 +337,6 @@ bool riscv_cpu_exec_interrupt(CPUState *cs, int interrupt_request); bool riscv_cpu_fp_enabled(CPURISCVState *env); 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(int mmu_idx); int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch); hwaddr riscv_cpu_get_phys_page_debug(CPUState *cpu, vaddr addr); diff --git a/target/riscv/cpu_bits.h b/target/riscv/cpu_bits.h index caf4599207..7322f54157 100644 --- a/target/riscv/cpu_bits.h +++ b/target/riscv/cpu_bits.h @@ -462,12 +462,6 @@ /* Virtulisation Register Fields */ #define VIRT_ONOFF 1 -/* This is used to save state for when we take an exception. If this is set - * that means that we want to force a HS level exception (no matter what the - * delegation is set to). This will occur for things such as a second level - * page table fault. - */ -#define FORCE_HS_EXCEP 2 /* RV32 satp CSR field masks */ #define SATP32_MODE 0x80000000 diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c index 21c54ef561..babe3d844b 100644 --- a/target/riscv/cpu_helper.c +++ b/target/riscv/cpu_helper.c @@ -38,36 +38,24 @@ int riscv_cpu_mmu_index(CPURISCVState *env, bool ifetch) #ifndef CONFIG_USER_ONLY static int riscv_cpu_local_irq_pending(CPURISCVState *env) { - target_ulong irqs; + target_ulong virt_enabled = riscv_cpu_virt_enabled(env); target_ulong mstatus_mie = get_field(env->mstatus, MSTATUS_MIE); target_ulong mstatus_sie = get_field(env->mstatus, MSTATUS_SIE); - target_ulong hs_mstatus_sie = get_field(env->mstatus_hs, MSTATUS_SIE); - target_ulong pending = env->mip & env->mie & - ~(MIP_VSSIP | MIP_VSTIP | MIP_VSEIP); - target_ulong vspending = (env->mip & env->mie & - (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP)); + target_ulong pending = env->mip & env->mie; target_ulong mie = env->priv < PRV_M || (env->priv == PRV_M && mstatus_mie); target_ulong sie = env->priv < PRV_S || (env->priv == PRV_S && mstatus_sie); - target_ulong hs_sie = env->priv < PRV_S || - (env->priv == PRV_S && hs_mstatus_sie); + target_ulong hsie = virt_enabled || sie; + target_ulong vsie = virt_enabled && sie; - if (riscv_cpu_virt_enabled(env)) { - target_ulong pending_hs_irq = pending & -hs_sie; - - if (pending_hs_irq) { - riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP); - return ctz64(pending_hs_irq); - } - - pending = vspending; - } - - irqs = (pending & ~env->mideleg & -mie) | (pending & env->mideleg & -sie); + target_ulong irqs = + (pending & ~env->mideleg & -mie) | + (pending & env->mideleg & ~env->hideleg & -hsie) | + (pending & env->mideleg & env->hideleg & -vsie); if (irqs) { return ctz64(irqs); /* since non-zero */ @@ -190,24 +178,6 @@ void riscv_cpu_set_virt_enabled(CPURISCVState *env, bool enable) env->virt = set_field(env->virt, VIRT_ONOFF, enable); } -bool riscv_cpu_force_hs_excep_enabled(CPURISCVState *env) -{ - if (!riscv_has_ext(env, RVH)) { - return false; - } - - return get_field(env->virt, FORCE_HS_EXCEP); -} - -void riscv_cpu_set_force_hs_excep(CPURISCVState *env, bool enable) -{ - if (!riscv_has_ext(env, RVH)) { - return; - } - - env->virt = set_field(env->virt, FORCE_HS_EXCEP, enable); -} - bool riscv_cpu_two_stage_lookup(int mmu_idx) { return mmu_idx & TB_FLAGS_PRIV_HYP_ACCESS_MASK; @@ -896,7 +866,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) RISCVCPU *cpu = RISCV_CPU(cs); CPURISCVState *env = &cpu->env; - bool force_hs_execp = riscv_cpu_force_hs_excep_enabled(env); uint64_t s; /* cs->exception is 32-bits wide unlike mcause which is XLEN-bits wide @@ -925,8 +894,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) case RISCV_EXCP_INST_GUEST_PAGE_FAULT: case RISCV_EXCP_LOAD_GUEST_ACCESS_FAULT: case RISCV_EXCP_STORE_GUEST_AMO_ACCESS_FAULT: - force_hs_execp = true; - /* fallthrough */ case RISCV_EXCP_INST_ADDR_MIS: case RISCV_EXCP_INST_ACCESS_FAULT: case RISCV_EXCP_LOAD_ADDR_MIS: @@ -985,8 +952,7 @@ void riscv_cpu_do_interrupt(CPUState *cs) env->hstatus = set_field(env->hstatus, HSTATUS_GVA, 0); } - if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1) && - !force_hs_execp) { + if (riscv_cpu_virt_enabled(env) && ((hdeleg >> cause) & 1)) { /* Trap to VS mode */ /* * See if we need to adjust cause. Yes if its VS mode interrupt @@ -1008,7 +974,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) htval = env->guest_phys_fault_addr; riscv_cpu_set_virt_enabled(env, 0); - riscv_cpu_set_force_hs_excep(env, 0); } else { /* Trap into HS mode */ env->hstatus = set_field(env->hstatus, HSTATUS_SPV, false); @@ -1044,7 +1009,6 @@ void riscv_cpu_do_interrupt(CPUState *cs) /* Trapping to M mode, virt is disabled */ riscv_cpu_set_virt_enabled(env, 0); - riscv_cpu_set_force_hs_excep(env, 0); } s = env->mstatus;
VS interrupts (2, 6, 10) were not correctly forwarded to hs-mode when not delegated in hideleg (which was not being taken into account). This was mainly because hs level sie was not always considered enabled when it should. The spec states that "Interrupts for higher-privilege modes, y>x, are always globally enabled regardless of the setting of the global yIE bit for the higher-privilege mode." and also "For purposes of interrupt global enables, HS-mode is considered more privileged than VS-mode, and VS-mode is considered more privileged than VU-mode". These interrupts should be treated the same as any other kind of exception. Therefore, there is no need to "force an hs exception" as the current privilege level, the state of the global ie and of the delegation registers should be enough to route the interrupt to the appropriate privilege level in riscv_cpu_do_interrupt. Also, these interrupts never target m-mode, which is guaranteed by the hardwiring of the corresponding bits in mideleg. The same is true for synchronous exceptions, specifically, guest page faults which must be hardwired in to zero hedeleg. As such the hs_force_except mechanism can be removed. Signed-off-by: Jose Martins <josemartins90@gmail.com> --- This version of the patch also removes the uneeded hs_force_except functions, variables and macro. target/riscv/cpu.h | 2 -- target/riscv/cpu_bits.h | 6 ----- target/riscv/cpu_helper.c | 54 +++++++-------------------------------- 3 files changed, 9 insertions(+), 53 deletions(-)