Ravi Bangoria <ravi.bango...@linux.ibm.com> writes: > Hi Daniel, > > On 10/12/20 7:21 AM, Daniel Axtens wrote: >> Hi, >> >> Apologies if this has come up in a previous revision. >> >> >>> case 1: >>> + if (!cpu_has_feature(CPU_FTR_ARCH_31)) >>> + return -1; >>> + >>> prefix_r = GET_PREFIX_R(word); >>> ra = GET_PREFIX_RA(suffix); >> >> The comment above analyse_instr reads in part: >> >> * Return value is 1 if the instruction can be emulated just by >> * updating *regs with the information in *op, -1 if we need the >> * GPRs but *regs doesn't contain the full register set, or 0 >> * otherwise. >> >> I was wondering why returning -1 if the instruction isn't supported the >> right thing to do - it seemed to me that it should return 0? >> >> I did look and see that there are other cases where the code returns -1 >> for an unsupported operation, e.g.: >> >> #ifdef __powerpc64__ >> case 4: >> if (!cpu_has_feature(CPU_FTR_ARCH_300)) >> return -1; >> >> switch (word & 0x3f) { >> case 48: /* maddhd */ >> >> That's from commit 930d6288a267 ("powerpc: sstep: Add support for >> maddhd, maddhdu, maddld instructions"), but it's not explained there either >> >> I see the same pattern in a number of commits: commit 6324320de609 >> ("powerpc sstep: Add support for modsd, modud instructions"), commit >> 6c180071509a ("powerpc sstep: Add support for modsw, moduw >> instructions"), commit a23987ef267a ("powerpc: sstep: Add support for >> darn instruction") and a few others, all of which seem to have come >> through Sandipan in February 2019. I haven't spotted any explanation for >> why -1 was chosen, but I haven't checked the mailing list archives. >> >> If -1 is OK, would it be possible to update the comment to explain why? > > Agreed. As you rightly pointed out, there are many more such cases and, yes, > we are aware of this issue and it's being worked upon as a separate patch. > (Sandipan did send a fix patch internally some time back).
That sounds like a perfectly reasonable approach. I'd love to review the patch when it's sent - if you or Sandipan could please cc: me so I don't miss it I'd really appreciate that. I will proceed to review the rest of the patch and series. Kind regards, Daniel > > Thanks for pointing it out! > Ravi