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

[Fabiano Rosas <farosas@linux.ibm.com>]

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);
>              }
>          }
>      }