Re: [PATCH v3 09/14] target/ppc: Move patching nip from exception handler to helper_scv

[Nicholas Piggin]

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