On Wed, Apr 29, 2020 at 9:07 AM Jose Martins <josemartin...@gmail.com> 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 <josemartin...@gmail.com> > --- > 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