LinuxPPC-Dev Archive on lore.kernel.org
 help / color / Atom feed
From: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
To: Daniel Axtens <dja@axtens.net>
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 16:37:56 +0530
Message-ID: <41dda593-0fc6-569e-2243-189d84653b4a@linux.ibm.com> (raw)
In-Reply-To: <87h7r0w6s0.fsf@dja-thinkpad.axtens.net>

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

  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 [this message]
2020-10-12 12:55       ` Daniel Axtens
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=41dda593-0fc6-569e-2243-189d84653b4a@linux.ibm.com \
    --to=ravi.bangoria@linux.ibm.com \
    --cc=bala24@linux.ibm.com \
    --cc=dja@axtens.net \
    --cc=jniethe5@gmail.com \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=naveen.n.rao@linux.vnet.ibm.com \
    --cc=paulus@samba.org \
    --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