LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Daniel Axtens <dja@axtens.net>
To: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>,
	bala24@linux.ibm.com, paulus@samba.org, sandipan@linux.ibm.com,
	jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
Date: Mon, 12 Oct 2020 23:55:50 +1100
Message-ID: <87a6wrwqll.fsf@dja-thinkpad.axtens.net> (raw)
In-Reply-To: <41dda593-0fc6-569e-2243-189d84653b4a@linux.ibm.com>

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

  reply index

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-11  5:09 [PATCH v5 0/5] powerpc/sstep: VSX 32-byte vector paired load/store instructions Ravi Bangoria
2020-10-11  5:09 ` [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set Ravi Bangoria
2020-10-11 15:06   ` Sandipan Das
2020-10-12  1:51   ` Daniel Axtens
2020-10-12 11:07     ` Ravi Bangoria
2020-10-12 12:55       ` Daniel Axtens [this message]
2020-10-12 13:44   ` Daniel Axtens
2020-10-14  7:34     ` Ravi Bangoria
2020-10-11  5:09 ` [PATCH v5 2/5] powerpc/sstep: Cover new VSX instructions under CONFIG_VSX Ravi Bangoria
2020-10-11  5:09 ` [PATCH v5 3/5] powerpc/sstep: Support VSX vector paired storage access instructions Ravi Bangoria
2020-10-11  5:09 ` [PATCH v5 4/5] powerpc/ppc-opcode: Add encoding macros for VSX vector paired instructions Ravi Bangoria
2020-10-11  5:09 ` [PATCH v5 5/5] powerpc/sstep: Add testcases for VSX vector paired load/store instructions Ravi Bangoria
2020-12-15 10:49 ` [PATCH v5 0/5] powerpc/sstep: VSX 32-byte " Michael Ellerman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87a6wrwqll.fsf@dja-thinkpad.axtens.net \
    --to=dja@axtens.net \
    --cc=bala24@linux.ibm.com \
    --cc=jniethe5@gmail.com \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=naveen.n.rao@linux.vnet.ibm.com \
    --cc=paulus@samba.org \
    --cc=ravi.bangoria@linux.ibm.com \
    --cc=sandipan@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LinuxPPC-Dev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linuxppc-dev/0 linuxppc-dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linuxppc-dev linuxppc-dev/ https://lore.kernel.org/linuxppc-dev \
		linuxppc-dev@lists.ozlabs.org linuxppc-dev@ozlabs.org
	public-inbox-index linuxppc-dev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.ozlabs.lists.linuxppc-dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git