Re: [RFC PATCH v3 3/3] target/ppc: Fix gen_priv_exception error value in mfspr/mtspr

[Matheus K. Ferst]

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>