Re: Move cmpi to decodetree causing regression

[BALATON Zoltan]

On Wed, 23 Jun 2021, BALATON Zoltan wrote:
> On Tue, 22 Jun 2021, Matheus K. Ferst wrote:
> > On 17/06/2021 21:46, BALATON Zoltan wrote:
> > > Hello,
> > >
> > > Commit 8f0a4b6a9 (target/ppc: Move cmp/cmpi/cmpl/cmpli to decodetree) 
> > > breaks AROS on sam460ex (see 
> > > http://zero.eik.bme.hu/~balaton/qemu/amiga/#aros ) as it seems to have a 
> > > cmpi instruction with the L bit set that real hardware apparently ignores. 
> > > Now I'm getting an exception (can be seen on serial when selecting Debug 
> > > option from AROS boot menu):
> > >
> > > [KRN] Exception 6 handler. Context @ ff7fb628, SysBase @ 00a60420, 
> > > KernelBase @ 0100017c
> > > [KRN] SRR0=0124b6c0, SRR1=000af000 DEAR=00000000 ESR=08000000
> > > [KRN] CTR=ff850a24 LR=0124c3dc XER=00000007 CCR=44022844
> > > [KRN] DAR=00000000 DSISR=00000000
> > > [KRN] GPR00=0124f9ec GPR01=012e0f40 GPR02=00a60420 GPR03=0123bd80
> > > [KRN] GPR04=00000001 GPR05=00000000 GPR06=012e0f33 GPR07=00000008
> > > [KRN] GPR08=0123beb0 GPR09=0123274c GPR10=00000004 GPR11=012aa7d8
> > > [KRN] GPR12=01230000 GPR13=00000000 GPR14=0000001e GPR15=01230000
> > > [KRN] GPR16=01232560 GPR17=0123272c GPR18=00000000 GPR19=80000a55
> > > [KRN] GPR20=0123df2c GPR21=0123df3c GPR22=0123df34 GPR23=01232778
> > > [KRN] GPR24=0123de6c GPR25=010f9998 GPR26=01230000 GPR27=00000000
> > > [KRN] GPR28=0105e0b0 GPR29=00000000 GPR30=00000001 GPR31=0123bc60
> > > [KRN] Instruction dump:
> > > [KRN] 0124b6c0: 2c240000 40820008 38800001 80230004
> > >
> > > This -----------^^^^^^^^ seems to be cmpi with L bit set.
> > >
> > > The commit message mentions this and we've found before that some guests 
> > > rely on invalid bits being ignored by real hardware, such as 27a3ea7e and 
> > > 0123d3cb. I think IBM cores generally ignore reserved bits while some 
> > > other implementations (e.g. from Motorola) may raise exceptions. It would 
> > > be better to do what hardware does so guest code running on real hardware 
> > > would also run on QEMU, then have an option for enforcing reserved bits if 
> > > this is useful for debugging (but just logging with -d guest_errors might 
> > > be enough for that). However since e500 for example seems to raise 
> > > exceptions for invalid bits then there should be a switch so that could be 
> > > connected to a debug option as well.
> > >
> > > Regards,
> > > BALATON Zoltan
> > I couldn't find 460 specific documentation, apart from the AMCC datasheet 
> > that doesn't help here, but the 440 manual is one of which explicitly says 
> > that L=1 is invalid. Rereading it now I found that:
> >
> > "If any bit in a reserved field does not contain 0, the instruction form is 
> > invalid and its result is architecturally undefined. Unless otherwise 
> > noted, the PPC440 will execute all invalid instruction forms without 
> > causing an Illegal Instruction exception."
> >
> > There is a similar text in the 405 manual, but the e500 and e500mc ones 
> > only say that invalid forms lead to "boundedly undefined results". A deeper 
> > reading, however, makes me think that later two consider L a defined bit, 
> > and L=1 a valid form that is unimplemented, in which case the illegal 
> > instruction should be invoked.
> >
> > If these two are the only ones that raise an exception, we can use 
> > PPC2_BOOKE206 to decide, as in 27a3ea7e:
> >
> > > if ((ctx->insns_flags & PPC_64B) == 0) {
> > >    if (a->l) {
> > >        if ((ctx->insns_flags2 & PPC2_BOOKE206) == 0) {
> > >            qemu_log_mask(LOG_GUEST_ERROR,
> > >                    "Invalid form of CMP%s at 0x" TARGET_FMT_lx ", L = 
> > > 1\n",
> > >                    s ? "" : "L", ctx->cia);
> > >        } else {
> > >            return false;
> > >        }
> > >    }
> > >    gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> > > } else {
> > >    if (a->l) {
> > >        gen_op_cmp(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> > >    } else {
> > >        gen_op_cmp32(cpu_gpr[a->ra], cpu_gpr[a->rb], s, a->bf);
> > >    }
> > > }
> > > return true;
> >
> > Is that a reasonable approach? Or should we keep invalid by default and 
> > only ignore bit L in know cases, such as 460ex and 7447A?
>
> These are the only ones I've seen so far with the guests I've tested but 
> there may be others out there that I haven't found yet. As it works on real 
> hardware these may go unnoticed.
>
> I have no preference on how it's fixed as long as it works with the guests I 
> run. I think it's more correct to do what real CPUs do and ignore invalid 
> bits (unless a run time debug option is enabled) for those CPUs that we know 
> do this (I think most IBM cores). But if it's easier to do it only for this 
> instruction that would work too, however that means we have special cases for 
> some instructions that could be avoded otherwise and handle everything 
> uniformly. But I'm OK with whatever others think would be best if that fixes 
> the problem too.

This is still a problem with master which prevents running AROS on 
sam460ex. As this is fixing a regression it can be addressed during the 
freeze but some solution should be proposed for this so it could be 
merged. Do you have some patches in the works? If a more complete solution 
is not feasible in this devel cycle then relaxing the mask as was before 
could be a solution for this release.

Regards,
BALATON Zoltan