| Message ID | 20220113170456.1796911-4-matheus.ferst@eldorado.org.br |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user/ppc: Deliver SIGTRAP on tw[i]/td[i] | expand |
matheus.ferst@eldorado.org.br writes: > From: Matheus Ferst <matheus.ferst@eldorado.org.br> > > The code in linux-user/ppc/cpu_loop.c expects POWERPC_EXCP_PRIV > exception with error POWERPC_EXCP_PRIV_OPC or POWERPC_EXCP_PRIV_REG, > while POWERPC_EXCP_INVAL_SPR is expected in POWERPC_EXCP_INVAL > exceptions. This mismatch caused an EXCP_DUMP with the message "Unknown > privilege violation (03)", as seen in [1]. > > Fixes: 9b2fadda3e01 ("ppc: Rework generation of priv and inval interrupts") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/588 > > [1] https://gitlab.com/qemu-project/qemu/-/issues/588 > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> This patch seems to do the right thing. So: Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com> Now, I'm not sure if the code around it does the right thing. =) Specifically the else blocks (read_cb == NULL) and (write_cb == NULL). From _spr_register I understand that cb == NULL means this is not a recognized SPR by this processor*. So in my mind 100% of them should be invalid SPR exceptions. The reserved SPRs should be registered in cpu_init and handled as "known, but privileged" or "known, but noop". Maybe using SPR_NOACCESS and/or a new SPR_NOOP. It might be a bit tricky because they have no names, but that is an implementation detail. * - there's some nuance here because of the system vs. linux-user build time configuration so I'm not entirely sure. Let's think a bit more about this. Everything seems to work just fine the way it is so there's no rush. But I think this code could perhaps be simplified and some of these assumptions handled at build time with spr_register. > --- > Is there any case where throwing a PRIV/INVAL exception with a > INVAL/PRIV error makes sense? It seems wrong, but maybe I'm missing > something... especially with the HV_EMU to program check conversion. > > Also, if this patch is correct, it seems that all invalid SPR access > would be nop or privilege exceptions. In this case, is > POWERPC_EXCP_INVAL_SPR still needed? I agree that as it stands this is not needed. But we might want to bring it back given the points I mentioned above. So let's keep it for now. > --- > target/ppc/translate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 40232201bb..abbc3a5bb9 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -4827,11 +4827,11 @@ static inline void gen_op_mfspr(DisasContext *ctx) > */ > if (sprn & 0x10) { > if (ctx->pr) { > - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } else { > if (ctx->pr || sprn == 0 || sprn == 4 || sprn == 5 || sprn == 6) { > - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } > } > @@ -5014,11 +5014,11 @@ static void gen_mtspr(DisasContext *ctx) > */ > if (sprn & 0x10) { > if (ctx->pr) { > - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } else { > if (ctx->pr || sprn == 0) { > - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } > }
Le 13/01/2022 à 18:04, matheus.ferst@eldorado.org.br a écrit : > From: Matheus Ferst <matheus.ferst@eldorado.org.br> > > The code in linux-user/ppc/cpu_loop.c expects POWERPC_EXCP_PRIV > exception with error POWERPC_EXCP_PRIV_OPC or POWERPC_EXCP_PRIV_REG, > while POWERPC_EXCP_INVAL_SPR is expected in POWERPC_EXCP_INVAL > exceptions. This mismatch caused an EXCP_DUMP with the message "Unknown > privilege violation (03)", as seen in [1]. > > Fixes: 9b2fadda3e01 ("ppc: Rework generation of priv and inval interrupts") > Resolves: https://gitlab.com/qemu-project/qemu/-/issues/588 > > [1] https://gitlab.com/qemu-project/qemu/-/issues/588 > > Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> > --- > Is there any case where throwing a PRIV/INVAL exception with a > INVAL/PRIV error makes sense? It seems wrong, but maybe I'm missing > something... especially with the HV_EMU to program check conversion. > > Also, if this patch is correct, it seems that all invalid SPR access > would be nop or privilege exceptions. In this case, is > POWERPC_EXCP_INVAL_SPR still needed? > --- > target/ppc/translate.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/target/ppc/translate.c b/target/ppc/translate.c > index 40232201bb..abbc3a5bb9 100644 > --- a/target/ppc/translate.c > +++ b/target/ppc/translate.c > @@ -4827,11 +4827,11 @@ static inline void gen_op_mfspr(DisasContext *ctx) > */ > if (sprn & 0x10) { > if (ctx->pr) { > - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } else { > if (ctx->pr || sprn == 0 || sprn == 4 || sprn == 5 || sprn == 6) { > - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } > } > @@ -5014,11 +5014,11 @@ static void gen_mtspr(DisasContext *ctx) > */ > if (sprn & 0x10) { > if (ctx->pr) { > - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } else { > if (ctx->pr || sprn == 0) { > - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); > + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); > } > } > } It seems logic to emit a POWERPC_EXCP_PRIV_XXX exception with gen_priv_exception() (POWERPC_EXCP_PRIV). Moreover in line above we have a gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG) if the register cannot be read (SPR_NOACCESS). But in helper_load_dcr() we have POWERPC_EXCP_PRIV_REG with POWERPC_EXCP_INVAL (whereas in the helper_store_dcr() function we have POWERPC_EXCP_INVAL with POWERPC_EXCP_INVAL_INVAL). It looks like another bug. and in gen_slbfee() we have also a POWERPC_EXCP_PRIV_REG with gen_inval_exception() (POWERPC_EXCP_INVAL). What is interesting is gen_inval_exception() uses POWERPC_EXCP_HV_EMU that could make thinking we could try to emulate the operation (for KVM PR, for instance). Thanks, Laurent
On 04/03/2022 11:42, Laurent Vivier wrote: > Le 13/01/2022 à 18:04, matheus.ferst@eldorado.org.br a écrit : >> From: Matheus Ferst <matheus.ferst@eldorado.org.br> >> >> The code in linux-user/ppc/cpu_loop.c expects POWERPC_EXCP_PRIV >> exception with error POWERPC_EXCP_PRIV_OPC or POWERPC_EXCP_PRIV_REG, >> while POWERPC_EXCP_INVAL_SPR is expected in POWERPC_EXCP_INVAL >> exceptions. This mismatch caused an EXCP_DUMP with the message "Unknown >> privilege violation (03)", as seen in [1]. >> >> Fixes: 9b2fadda3e01 ("ppc: Rework generation of priv and inval >> interrupts") >> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/588 >> >> [1] https://gitlab.com/qemu-project/qemu/-/issues/588 >> >> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br> >> --- >> Is there any case where throwing a PRIV/INVAL exception with a >> INVAL/PRIV error makes sense? It seems wrong, but maybe I'm missing >> something... especially with the HV_EMU to program check conversion. >> >> Also, if this patch is correct, it seems that all invalid SPR access >> would be nop or privilege exceptions. In this case, is >> POWERPC_EXCP_INVAL_SPR still needed? >> --- >> target/ppc/translate.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/target/ppc/translate.c b/target/ppc/translate.c >> index 40232201bb..abbc3a5bb9 100644 >> --- a/target/ppc/translate.c >> +++ b/target/ppc/translate.c >> @@ -4827,11 +4827,11 @@ static inline void gen_op_mfspr(DisasContext >> *ctx) >> */ >> if (sprn & 0x10) { >> if (ctx->pr) { >> - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); >> + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); >> } >> } else { >> if (ctx->pr || sprn == 0 || sprn == 4 || sprn == 5 || >> sprn == 6) { >> - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); >> + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); >> } >> } >> } >> @@ -5014,11 +5014,11 @@ static void gen_mtspr(DisasContext *ctx) >> */ >> if (sprn & 0x10) { >> if (ctx->pr) { >> - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); >> + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); >> } >> } else { >> if (ctx->pr || sprn == 0) { >> - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); >> + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); >> } >> } >> } > > It seems logic to emit a POWERPC_EXCP_PRIV_XXX exception with > gen_priv_exception() (POWERPC_EXCP_PRIV). > > Moreover in line above we have a gen_priv_exception(ctx, > POWERPC_EXCP_PRIV_REG) if the register > cannot be read (SPR_NOACCESS). > > But in helper_load_dcr() we have POWERPC_EXCP_PRIV_REG with > POWERPC_EXCP_INVAL (whereas in the > helper_store_dcr() function we have POWERPC_EXCP_INVAL with > POWERPC_EXCP_INVAL_INVAL). > It looks like another bug. The instructions that could invoke these helpers in user-mode (mfdcrux and mtdcrux) are behind PPC_DCRUX, and no CPU has this insns_flag, so I guess the code is wrong, but we cannot hit the bug in linux-user. Similarly, we have gen_hvpriv_exception with POWERPC_EXCP_INVAL_SPR in spr_groupA_write_allowed and gen_inval_exception with POWERPC_EXCP_PRIV_REG in spr_write_excp_vector, but both are behind !defined(CONFIG_USER_ONLY). > and in gen_slbfee() we have also a POWERPC_EXCP_PRIV_REG with > gen_inval_exception() > (POWERPC_EXCP_INVAL). This one is easier to test. Looking at si_code: void sigill_handle(int sig, siginfo_t *si, void *ucontext) { _exit(si->si_code); } int main(void) { struct sigaction sa = {.sa_sigaction = sigill_handle, .sa_flags = SA_SIGINFO}; uint64_t t = 0, b = 0; sigaction(SIGILL, &sa, NULL); asm("slbfee. %0, %1" : "=r" (t) : "r" (b)); return 0; } We have: $ ./slbee; echo $? 5 # ILL_PRVOPC $ ./qemu-ppc64 ./slbfee; echo $? 2 # ILL_ILLOPN So I think we should be using gen_priv_exception or gen_hvpriv_exception with POWERPC_EXCP_PRIV_OPC. > What is interesting is gen_inval_exception() uses POWERPC_EXCP_HV_EMU > that could make thinking we > could try to emulate the operation (for KVM PR, for instance). IIUC, we should use gen_hvpriv_exception in those cases, so we have POWERPC_EXCP_HV_EMU with POWERPC_EXCP_PRIV | something. Thanks, Matheus K. Ferst Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/> Analista de Software Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>
diff --git a/target/ppc/translate.c b/target/ppc/translate.c index 40232201bb..abbc3a5bb9 100644 --- a/target/ppc/translate.c +++ b/target/ppc/translate.c @@ -4827,11 +4827,11 @@ static inline void gen_op_mfspr(DisasContext *ctx) */ if (sprn & 0x10) { if (ctx->pr) { - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); } } else { if (ctx->pr || sprn == 0 || sprn == 4 || sprn == 5 || sprn == 6) { - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); } } } @@ -5014,11 +5014,11 @@ static void gen_mtspr(DisasContext *ctx) */ if (sprn & 0x10) { if (ctx->pr) { - gen_priv_exception(ctx, POWERPC_EXCP_INVAL_SPR); + gen_priv_exception(ctx, POWERPC_EXCP_PRIV_REG); } } else { if (ctx->pr || sprn == 0) { - gen_hvpriv_exception(ctx, POWERPC_EXCP_INVAL_SPR); + gen_hvpriv_exception(ctx, POWERPC_EXCP_PRIV_REG); } } }