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

[Daniel Axtens]

Ravi Bangoria <ravi.bangoria at 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