[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] target/riscv: fix VS interrupts forwarding to HS
From: |
Alistair Francis |
Subject: |
Re: [PATCH 1/1] target/riscv: fix VS interrupts forwarding to HS |
Date: |
Wed, 29 Apr 2020 11:43:18 -0700 |
On Wed, Apr 29, 2020 at 9:07 AM Jose Martins <address@hidden> wrote:
>
> > If the Hypervisor sets the V* interrupts why does it then want to
> > receive the interrupt itself?
>
> I don't think this is a question of whether there is a use case for it
> or not (I agree with you, of the top of my head I don't see why would
> you forward v* interrupts to the hypervisor). However, from what I
> can understand, the spec allows for it. If you don't set the
> corresponding bits in hideleg, v* interrupts should be forwarded to HS
> (as I said, they are guaranteed not to be forwarded to m mode because
> these bits must be hardwired in mideleg). Otherwise, there would be no
> purpose for the hideleg register, as v* interrupts bits are the only
> ones that can be written in it (am I missing something?).
I think you are right.
"Among bits 15:0 of hideleg, only bits 10, 6, and 2 (corresponding to
the standard VS-level interrupts) shall be writable, and the others
shall be hardwired to zero."
Which means that it only controls the V* interrupts.
>
> > Isn't hs_sie only ever accessed if riscv_cpu_virt_enabled(env)?
> > Doesn't this just set hs_sie to always be 1?
>
> I don't understand if you don't agree that hs_sie should be always set
> when riscv_cpu_virt_enabled(env), or if you agree with it and don't
> see the need for the hs_sie variable at all. If it is the latter, I
> agree with you. So the patch would become:
Currently hs_sie is set:
- When we are in U-Mode
- If we are in S-Mode and hs_mstatus_sie is true
Then hs_sie is only accessed if virtulisation is enabled.
Your change just made it true for whenever virtulisation is enabled
(in which case we don't need it).
>
> Signed-off-by: Jose Martins <address@hidden>
> ---
> target/riscv/cpu_helper.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/target/riscv/cpu_helper.c b/target/riscv/cpu_helper.c
> index d3ba9efb02..a85eadb4fb 100644
> --- a/target/riscv/cpu_helper.c
> +++ b/target/riscv/cpu_helper.c
> @@ -41,10 +41,8 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *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 pending = env->mip & env->mie;
This looks good
> target_ulong vspending = (env->mip & env->mie &
> (MIP_VSSIP | MIP_VSTIP | MIP_VSEIP));
>
> @@ -52,11 +50,9 @@ static int riscv_cpu_local_irq_pending(CPURISCVState *env)
> (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;
> + target_ulong pending_hs_irq = pending & ~env->hideleg;
I don't see why we don't need to check the HS version of MSTATUS_SIE.
I think hs_sie should stay shouldn't it?
Alistair
>
> if (pending_hs_irq) {
> riscv_cpu_set_force_hs_excep(env, FORCE_HS_EXCEP);
> --
> 2.17.1
>
> Jose