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).
Thanks for pointing it out!
Ravi
