[PATCH v3 0/3] linux-user/ppc: Deliver SIGTRAP on tw[i]/td[i]

[PATCH v3 0/3] linux-user/ppc: Deliver SIGTRAP on tw[i]/td[i]

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

In the review of 66c6b40aba1, Richard Henderson suggested[1] using
"trap" instead of ".long 0x0" to generate the signal to test XER
save/restore behavior. However, linux-user aborts when a trap
exception is raised, so we kept the patch with SIGILL.

This patch series is a follow-up to remove the cpu_abort call, deliver
SIGTRAP instead (using TRAP_BRKPT as si_code), and apply the suggestion
to the signal_save_restore_xer test.

The first patch removes the "qemu: fatal: Tried to call a TRAP" reported
in issue #588[2]. The third patch is an RFC to address the other logged
messages of "Unknown privilege violation (03)".

[1] https://lists.gnu.org/archive/html/qemu-ppc/2021-10/msg00143.html
[2] https://gitlab.com/qemu-project/qemu/-/issues/588

v3:
 - RFC to address the "Unknown privilege violation (03)" in #588.

v2:
 - Based-on rth's patch to use force_sig_fault and avoid merge conflicts

Matheus Ferst (3):
  linux-user/ppc: deliver SIGTRAP on POWERPC_EXCP_TRAP
  tests/tcg/ppc64le: change signal_save_restore_xer to use SIGTRAP
  target/ppc: Fix gen_priv_exception error value in mfspr/mtspr

 linux-user/ppc/cpu_loop.c                   | 3 ++-
 target/ppc/translate.c                      | 8 ++++----
 tests/tcg/ppc64le/signal_save_restore_xer.c | 8 ++++----
 3 files changed, 10 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] linux-user/ppc: deliver SIGTRAP on POWERPC_EXCP_TRAP

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Handle POWERPC_EXCP_TRAP in cpu_loop to deliver SIGTRAP on tw[i]/td[i].
The si_code comes from do_program_check in the kernel source file
arch/powerpc/kernel/traps.c

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 linux-user/ppc/cpu_loop.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
index 46e6ffd6d3..6c99feb19b 100644
--- a/linux-user/ppc/cpu_loop.c
+++ b/linux-user/ppc/cpu_loop.c
@@ -188,7 +188,8 @@ void cpu_loop(CPUPPCState *env)
                 }
                 break;
             case POWERPC_EXCP_TRAP:
-                cpu_abort(cs, "Tried to call a TRAP\n");
+                si_signo = TARGET_SIGTRAP;
+                si_code = TARGET_TRAP_BRKPT;
                 break;
             default:
                 /* Should not happen ! */
-- 
2.25.1

[PATCH v3 2/3] tests/tcg/ppc64le: change signal_save_restore_xer to use SIGTRAP

[matheus.ferst]

From: Matheus Ferst <matheus.ferst@eldorado.org.br>

Now that linux-user delivers the signal on tw, we can change
signal_save_restore_xer to use SIGTRAP instead of SIGILL.

Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
---
 tests/tcg/ppc64le/signal_save_restore_xer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tests/tcg/ppc64le/signal_save_restore_xer.c b/tests/tcg/ppc64le/signal_save_restore_xer.c
index e4f8a07dd7..9227f4f455 100644
--- a/tests/tcg/ppc64le/signal_save_restore_xer.c
+++ b/tests/tcg/ppc64le/signal_save_restore_xer.c
@@ -11,7 +11,7 @@
 
 uint64_t saved;
 
-void sigill_handler(int sig, siginfo_t *si, void *ucontext)
+void sigtrap_handler(int sig, siginfo_t *si, void *ucontext)
 {
     ucontext_t *uc = ucontext;
     uc->uc_mcontext.regs->nip += 4;
@@ -23,14 +23,14 @@ int main(void)
 {
     uint64_t initial = XER_CA | XER_CA32, restored;
     struct sigaction sa = {
-        .sa_sigaction = sigill_handler,
+        .sa_sigaction = sigtrap_handler,
         .sa_flags = SA_SIGINFO
     };
 
-    sigaction(SIGILL, &sa, NULL);
+    sigaction(SIGTRAP, &sa, NULL);
 
     asm("mtspr 1, %1\n\t"
-        ".long 0x0\n\t"
+        "trap\n\t"
         "mfspr %0, 1\n\t"
         : "=r" (restored)
         : "r" (initial));
-- 
2.25.1

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

[matheus.ferst]

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

Re: [PATCH v3 0/3] linux-user/ppc: Deliver SIGTRAP on tw[i]/td[i]

[Matheus K. Ferst]

Ping.

The based-on series is already on master, only patch 3 is missing review.

On 13/01/2022 14:04, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> In the review of 66c6b40aba1, Richard Henderson suggested[1] using
> "trap" instead of ".long 0x0" to generate the signal to test XER
> save/restore behavior. However, linux-user aborts when a trap
> exception is raised, so we kept the patch with SIGILL.
> 
> This patch series is a follow-up to remove the cpu_abort call, deliver
> SIGTRAP instead (using TRAP_BRKPT as si_code), and apply the suggestion
> to the signal_save_restore_xer test.
> 
> The first patch removes the "qemu: fatal: Tried to call a TRAP" reported
> in issue #588[2]. The third patch is an RFC to address the other logged
> messages of "Unknown privilege violation (03)".
> 
> [1] https://lists.gnu.org/archive/html/qemu-ppc/2021-10/msg00143.html
> [2] https://gitlab.com/qemu-project/qemu/-/issues/588
> 
> v3:
>   - RFC to address the "Unknown privilege violation (03)" in #588.
> 
> v2:
>   - Based-on rth's patch to use force_sig_fault and avoid merge conflicts
> 
> Matheus Ferst (3):
>    linux-user/ppc: deliver SIGTRAP on POWERPC_EXCP_TRAP
>    tests/tcg/ppc64le: change signal_save_restore_xer to use SIGTRAP
>    target/ppc: Fix gen_priv_exception error value in mfspr/mtspr
> 
>   linux-user/ppc/cpu_loop.c                   | 3 ++-
>   target/ppc/translate.c                      | 8 ++++----
>   tests/tcg/ppc64le/signal_save_restore_xer.c | 8 ++++----
>   3 files changed, 10 insertions(+), 9 deletions(-)
> 

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

[Fabiano Rosas]

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

Re: [PATCH v3 0/3] linux-user/ppc: Deliver SIGTRAP on tw[i]/td[i]

[Matheus K. Ferst]

Ping.

All patches reviewed and the series still applies to master with no 
conflicts.

On 13/01/2022 14:04, matheus.ferst@eldorado.org.br wrote:
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> In the review of 66c6b40aba1, Richard Henderson suggested[1] using
> "trap" instead of ".long 0x0" to generate the signal to test XER
> save/restore behavior. However, linux-user aborts when a trap
> exception is raised, so we kept the patch with SIGILL.
> 
> This patch series is a follow-up to remove the cpu_abort call, deliver
> SIGTRAP instead (using TRAP_BRKPT as si_code), and apply the suggestion
> to the signal_save_restore_xer test.
> 
> The first patch removes the "qemu: fatal: Tried to call a TRAP" reported
> in issue #588[2]. The third patch is an RFC to address the other logged
> messages of "Unknown privilege violation (03)".
> 
> [1] https://lists.gnu.org/archive/html/qemu-ppc/2021-10/msg00143.html
> [2] https://gitlab.com/qemu-project/qemu/-/issues/588
> 
> v3:
>   - RFC to address the "Unknown privilege violation (03)" in #588.
> 
> v2:
>   - Based-on rth's patch to use force_sig_fault and avoid merge conflicts
> 
> Matheus Ferst (3):
>    linux-user/ppc: deliver SIGTRAP on POWERPC_EXCP_TRAP
>    tests/tcg/ppc64le: change signal_save_restore_xer to use SIGTRAP
>    target/ppc: Fix gen_priv_exception error value in mfspr/mtspr
> 
>   linux-user/ppc/cpu_loop.c                   | 3 ++-
>   target/ppc/translate.c                      | 8 ++++----
>   tests/tcg/ppc64le/signal_save_restore_xer.c | 8 ++++----
>   3 files changed, 10 insertions(+), 9 deletions(-)
>

Re: [PATCH v3 1/3] linux-user/ppc: deliver SIGTRAP on POWERPC_EXCP_TRAP

[Laurent Vivier]

Le 13/01/2022 à 18:04, matheus.ferst@eldorado.org.br a écrit :
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Handle POWERPC_EXCP_TRAP in cpu_loop to deliver SIGTRAP on tw[i]/td[i].
> The si_code comes from do_program_check in the kernel source file
> arch/powerpc/kernel/traps.c
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>   linux-user/ppc/cpu_loop.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/ppc/cpu_loop.c b/linux-user/ppc/cpu_loop.c
> index 46e6ffd6d3..6c99feb19b 100644
> --- a/linux-user/ppc/cpu_loop.c
> +++ b/linux-user/ppc/cpu_loop.c
> @@ -188,7 +188,8 @@ void cpu_loop(CPUPPCState *env)
>                   }
>                   break;
>               case POWERPC_EXCP_TRAP:
> -                cpu_abort(cs, "Tried to call a TRAP\n");
> +                si_signo = TARGET_SIGTRAP;
> +                si_code = TARGET_TRAP_BRKPT;
>                   break;
>               default:
>                   /* Should not happen ! */

Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent

Re: [PATCH v3 2/3] tests/tcg/ppc64le: change signal_save_restore_xer to use SIGTRAP

[Laurent Vivier]

Le 13/01/2022 à 18:04, matheus.ferst@eldorado.org.br a écrit :
> From: Matheus Ferst <matheus.ferst@eldorado.org.br>
> 
> Now that linux-user delivers the signal on tw, we can change
> signal_save_restore_xer to use SIGTRAP instead of SIGILL.
> 
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
>   tests/tcg/ppc64le/signal_save_restore_xer.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/tests/tcg/ppc64le/signal_save_restore_xer.c b/tests/tcg/ppc64le/signal_save_restore_xer.c
> index e4f8a07dd7..9227f4f455 100644
> --- a/tests/tcg/ppc64le/signal_save_restore_xer.c
> +++ b/tests/tcg/ppc64le/signal_save_restore_xer.c
> @@ -11,7 +11,7 @@
>   
>   uint64_t saved;
>   
> -void sigill_handler(int sig, siginfo_t *si, void *ucontext)
> +void sigtrap_handler(int sig, siginfo_t *si, void *ucontext)
>   {
>       ucontext_t *uc = ucontext;
>       uc->uc_mcontext.regs->nip += 4;
> @@ -23,14 +23,14 @@ int main(void)
>   {
>       uint64_t initial = XER_CA | XER_CA32, restored;
>       struct sigaction sa = {
> -        .sa_sigaction = sigill_handler,
> +        .sa_sigaction = sigtrap_handler,
>           .sa_flags = SA_SIGINFO
>       };
>   
> -    sigaction(SIGILL, &sa, NULL);
> +    sigaction(SIGTRAP, &sa, NULL);
>   
>       asm("mtspr 1, %1\n\t"
> -        ".long 0x0\n\t"
> +        "trap\n\t"
>           "mfspr %0, 1\n\t"
>           : "=r" (restored)
>           : "r" (initial));

Applied to my linux-user-for-7.0 branch.

Thanks,
Laurent

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

[Laurent Vivier]

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

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>