From: Nicholas Piggin <npiggin@gmail.com>
Unlike sc, for scv a facility unavailable interrupt must be generated
if FSCR[SCV]=0 so we can't raise the exception with nip set to next
instruction but we can move advancing nip if the FSCR check passes to
helper_scv so the exception handler does not need to change it.
[balaton: added commit message]
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
This needs SoB from Nick
target/ppc/excp_helper.c | 2 +-
target/ppc/translate.c | 6 +++++-
2 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 903216c2a6..ef363b0285 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception */
lev = env->error_code;
dump_syscall(env);
- env->nip += 4;
new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
@@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
void helper_scv(CPUPPCState *env, uint32_t lev)
{
if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
+ env->nip += 4;
raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
} else {
raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
diff --git a/target/ppc/translate.c b/target/ppc/translate.c
index 4260d3d66f..0360a17fb3 100644
--- a/target/ppc/translate.c
+++ b/target/ppc/translate.c
@@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
{
uint32_t lev = (ctx->opcode >> 5) & 0x7F;
- /* Set the PC back to the faulting instruction. */
+ /*
+ * Set the PC back to the scv instruction (unlike sc), because a facility
+ * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
+ * advances nip if the FSCR check passes.
+ */
gen_update_nip(ctx, ctx->cia);
gen_helper_scv(cpu_env, tcg_constant_i32(lev));
--
2.30.9On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
>
> Unlike sc, for scv a facility unavailable interrupt must be generated
> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> instruction but we can move advancing nip if the FSCR check passes to
> helper_scv so the exception handler does not need to change it.
>
> [balaton: added commit message]
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Thanks, sorry for the delay :( Would you be able to resend the series?
You could drop the machine check one for now perhaps until we sort out
what to do with it.
Thanks,
Nick
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> This needs SoB from Nick
>
> target/ppc/excp_helper.c | 2 +-
> target/ppc/translate.c | 6 +++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 903216c2a6..ef363b0285 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception */
> lev = env->error_code;
> dump_syscall(env);
> - env->nip += 4;
> new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
> new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>
> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
> void helper_scv(CPUPPCState *env, uint32_t lev)
> {
> if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> + env->nip += 4;
> raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> } else {
> raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 4260d3d66f..0360a17fb3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
> {
> uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>
> - /* Set the PC back to the faulting instruction. */
> + /*
> + * Set the PC back to the scv instruction (unlike sc), because a facility
> + * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
> + * advances nip if the FSCR check passes.
> + */
> gen_update_nip(ctx, ctx->cia);
> gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>
On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
> From: Nicholas Piggin <npiggin@gmail.com>
>
> Unlike sc, for scv a facility unavailable interrupt must be generated
> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
> instruction but we can move advancing nip if the FSCR check passes to
> helper_scv so the exception handler does not need to change it.
>
> [balaton: added commit message]
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Ah you sent it, fine, thank you. But actually now I look again,
now we're off by one in the other direction for the dumps.
So... probably your way is still better because it matches the
interrupt semantics of the ISA when executing the instruction,
but it needs this patch:
For my patch you can add
Signed-off-by: Nicholas Piggin <npiggin@gmail.com
Thanks,
Nick
diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
index 0e21cb4451..d7f42639c8 100644
--- a/target/ppc/excp_helper.c
+++ b/target/ppc/excp_helper.c
@@ -117,7 +117,7 @@ static void dump_syscall(CPUPPCState *env)
ppc_dump_gpr(env, 0), ppc_dump_gpr(env, 3),
ppc_dump_gpr(env, 4), ppc_dump_gpr(env, 5),
ppc_dump_gpr(env, 6), ppc_dump_gpr(env, 7),
- ppc_dump_gpr(env, 8), env->nip);
+ ppc_dump_gpr(env, 8), env->nip - 4);
}
static void dump_hcall(CPUPPCState *env)
@@ -132,7 +132,7 @@ static void dump_hcall(CPUPPCState *env)
ppc_dump_gpr(env, 7), ppc_dump_gpr(env, 8),
ppc_dump_gpr(env, 9), ppc_dump_gpr(env, 10),
ppc_dump_gpr(env, 11), ppc_dump_gpr(env, 12),
- env->nip);
+ env->nip - 4);
}
#ifdef CONFIG_TCG
> ---
> This needs SoB from Nick
>
> target/ppc/excp_helper.c | 2 +-
> target/ppc/translate.c | 6 +++++-
> 2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 903216c2a6..ef363b0285 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
> case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception */
> lev = env->error_code;
> dump_syscall(env);
> - env->nip += 4;
> new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
> new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>
> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
> void helper_scv(CPUPPCState *env, uint32_t lev)
> {
> if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
> + env->nip += 4;
> raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
> } else {
> raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index 4260d3d66f..0360a17fb3 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
> {
> uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>
> - /* Set the PC back to the faulting instruction. */
> + /*
> + * Set the PC back to the scv instruction (unlike sc), because a facility
> + * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
> + * advances nip if the FSCR check passes.
> + */
> gen_update_nip(ctx, ctx->cia);
> gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>
> --
> 2.30.9
On Tue, 20 Jun 2023, Nicholas Piggin wrote:
> On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote:
>> From: Nicholas Piggin <npiggin@gmail.com>
>>
>> Unlike sc, for scv a facility unavailable interrupt must be generated
>> if FSCR[SCV]=0 so we can't raise the exception with nip set to next
>> instruction but we can move advancing nip if the FSCR check passes to
>> helper_scv so the exception handler does not need to change it.
>>
>> [balaton: added commit message]
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>
> Ah you sent it, fine, thank you. But actually now I look again,
> now we're off by one in the other direction for the dumps.
This is mentioned in the commit message for the patch changing sc. I think
we should not patch nip in the dump so we actually dump what the CPU
should have and match the ISA docs.
> So... probably your way is still better because it matches the
> interrupt semantics of the ISA when executing the instruction,
> but it needs this patch:
OK so then I'm confused why we need nip - 4 in dumps?
> For my patch you can add
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com
Please reply to that patch with this if you're OK with that or I'll just
drop it in v4 and let you send a follow up to avoid confusion.
Regards,
BALATON Zoltan
> Thanks,
> Nick
>
>
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 0e21cb4451..d7f42639c8 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -117,7 +117,7 @@ static void dump_syscall(CPUPPCState *env)
> ppc_dump_gpr(env, 0), ppc_dump_gpr(env, 3),
> ppc_dump_gpr(env, 4), ppc_dump_gpr(env, 5),
> ppc_dump_gpr(env, 6), ppc_dump_gpr(env, 7),
> - ppc_dump_gpr(env, 8), env->nip);
> + ppc_dump_gpr(env, 8), env->nip - 4);
> }
>
> static void dump_hcall(CPUPPCState *env)
> @@ -132,7 +132,7 @@ static void dump_hcall(CPUPPCState *env)
> ppc_dump_gpr(env, 7), ppc_dump_gpr(env, 8),
> ppc_dump_gpr(env, 9), ppc_dump_gpr(env, 10),
> ppc_dump_gpr(env, 11), ppc_dump_gpr(env, 12),
> - env->nip);
> + env->nip - 4);
> }
>
> #ifdef CONFIG_TCG
>
>
>
>> ---
>> This needs SoB from Nick
>>
>> target/ppc/excp_helper.c | 2 +-
>> target/ppc/translate.c | 6 +++++-
>> 2 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
>> index 903216c2a6..ef363b0285 100644
>> --- a/target/ppc/excp_helper.c
>> +++ b/target/ppc/excp_helper.c
>> @@ -1304,7 +1304,6 @@ static void powerpc_excp_books(PowerPCCPU *cpu, int excp)
>> case POWERPC_EXCP_SYSCALL_VECTORED: /* scv exception */
>> lev = env->error_code;
>> dump_syscall(env);
>> - env->nip += 4;
>> new_msr |= env->msr & ((target_ulong)1 << MSR_EE);
>> new_msr |= env->msr & ((target_ulong)1 << MSR_RI);
>>
>> @@ -2410,6 +2409,7 @@ void helper_ppc_maybe_interrupt(CPUPPCState *env)
>> void helper_scv(CPUPPCState *env, uint32_t lev)
>> {
>> if (env->spr[SPR_FSCR] & (1ull << FSCR_SCV)) {
>> + env->nip += 4;
>> raise_exception_err(env, POWERPC_EXCP_SYSCALL_VECTORED, lev);
>> } else {
>> raise_exception_err(env, POWERPC_EXCP_FU, FSCR_IC_SCV);
>> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
>> index 4260d3d66f..0360a17fb3 100644
>> --- a/target/ppc/translate.c
>> +++ b/target/ppc/translate.c
>> @@ -4433,7 +4433,11 @@ static void gen_scv(DisasContext *ctx)
>> {
>> uint32_t lev = (ctx->opcode >> 5) & 0x7F;
>>
>> - /* Set the PC back to the faulting instruction. */
>> + /*
>> + * Set the PC back to the scv instruction (unlike sc), because a facility
>> + * unavailable interrupt must be generated if FSCR[SCV]=0. The helper
>> + * advances nip if the FSCR check passes.
>> + */
>> gen_update_nip(ctx, ctx->cia);
>> gen_helper_scv(cpu_env, tcg_constant_i32(lev));
>>
>> --
>> 2.30.9
>
>
>
On Tue Jun 20, 2023 at 8:47 PM AEST, BALATON Zoltan wrote: > On Tue, 20 Jun 2023, Nicholas Piggin wrote: > > On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote: > >> From: Nicholas Piggin <npiggin@gmail.com> > >> > >> Unlike sc, for scv a facility unavailable interrupt must be generated > >> if FSCR[SCV]=0 so we can't raise the exception with nip set to next > >> instruction but we can move advancing nip if the FSCR check passes to > >> helper_scv so the exception handler does not need to change it. > >> > >> [balaton: added commit message] > >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > > > Ah you sent it, fine, thank you. But actually now I look again, > > now we're off by one in the other direction for the dumps. > > This is mentioned in the commit message for the patch changing sc. I think > we should not patch nip in the dump so we actually dump what the CPU > should have and match the ISA docs. > > > So... probably your way is still better because it matches the > > interrupt semantics of the ISA when executing the instruction, > > but it needs this patch: > > OK so then I'm confused why we need nip - 4 in dumps? Sorry I missed your reply here. We want nip - 4 in dumps so the address of the syscall is the sc instruction itself, not the random one after it. Thanks, Nick
On Mon, 26 Jun 2023, Nicholas Piggin wrote: > On Tue Jun 20, 2023 at 8:47 PM AEST, BALATON Zoltan wrote: >> On Tue, 20 Jun 2023, Nicholas Piggin wrote: >>> On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote: >>>> From: Nicholas Piggin <npiggin@gmail.com> >>>> >>>> Unlike sc, for scv a facility unavailable interrupt must be generated >>>> if FSCR[SCV]=0 so we can't raise the exception with nip set to next >>>> instruction but we can move advancing nip if the FSCR check passes to >>>> helper_scv so the exception handler does not need to change it. >>>> >>>> [balaton: added commit message] >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> >>> Ah you sent it, fine, thank you. But actually now I look again, >>> now we're off by one in the other direction for the dumps. >> >> This is mentioned in the commit message for the patch changing sc. I think >> we should not patch nip in the dump so we actually dump what the CPU >> should have and match the ISA docs. >> >>> So... probably your way is still better because it matches the >>> interrupt semantics of the ISA when executing the instruction, >>> but it needs this patch: >> >> OK so then I'm confused why we need nip - 4 in dumps? > > Sorry I missed your reply here. We want nip - 4 in dumps so the > address of the syscall is the sc instruction itself, not the > random one after it. Although that's how it was in QEMU before that's not how it is on real hardware so I don't think we should keep this and just log what a real CPU would have and people should know how to interpret that after consulting the ISA docs. Regards, BALATON Zoltan
On Wed Jun 28, 2023 at 3:40 AM AEST, BALATON Zoltan wrote: > On Mon, 26 Jun 2023, Nicholas Piggin wrote: > > On Tue Jun 20, 2023 at 8:47 PM AEST, BALATON Zoltan wrote: > >> On Tue, 20 Jun 2023, Nicholas Piggin wrote: > >>> On Fri Jun 16, 2023 at 9:03 AM AEST, BALATON Zoltan wrote: > >>>> From: Nicholas Piggin <npiggin@gmail.com> > >>>> > >>>> Unlike sc, for scv a facility unavailable interrupt must be generated > >>>> if FSCR[SCV]=0 so we can't raise the exception with nip set to next > >>>> instruction but we can move advancing nip if the FSCR check passes to > >>>> helper_scv so the exception handler does not need to change it. > >>>> > >>>> [balaton: added commit message] > >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > >>> > >>> Ah you sent it, fine, thank you. But actually now I look again, > >>> now we're off by one in the other direction for the dumps. > >> > >> This is mentioned in the commit message for the patch changing sc. I think > >> we should not patch nip in the dump so we actually dump what the CPU > >> should have and match the ISA docs. > >> > >>> So... probably your way is still better because it matches the > >>> interrupt semantics of the ISA when executing the instruction, > >>> but it needs this patch: > >> > >> OK so then I'm confused why we need nip - 4 in dumps? > > > > Sorry I missed your reply here. We want nip - 4 in dumps so the > > address of the syscall is the sc instruction itself, not the > > random one after it. > > Although that's how it was in QEMU before Current upstream QEMU dumps syscall address of sc instruction. After patch 8 and 9, it will dump the address of the instruction after it. > that's not how it is on real > hardware so I don't think we should keep this and just log what a real CPU > would have and people should know how to interpret that after consulting > the ISA docs. I did get the feeling it was nicer your way, OTOH there really is not anything in the ISA that requires a particular implementation. QEMU is a real implementation of the ISA anyway. You could argue it's more consistent for QEMU to keep env->nip as the address of instruction that caused the interrupt, and then the sc fixup is restricted to setting SRR0. I'm on the fence about it now. Thanks, Nick
© 2016 - 2026 Red Hat, Inc.