[PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set

[Ravi Bangoria]

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