Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
Hi Daniel, On 10/12/20 7:14 PM, Daniel Axtens wrote: Hi, To review this, I looked through the supported instructions to see if there were any that I thought might have been missed. I didn't find any other v3.1 ones, although I don't have a v3.1 ISA to hand so I was basically looking for instructions I didn't recognise. I did, however, find a number of instructions that are new in ISA 3.0 that aren't guarded: - addpcis - lxvl/stxvl - lxvll/stxvll - lxvwsx - stxvx - lxsipzx - lxvh8x - lxsihzx - lxvb16x/stxvb16x - stxsibx - stxsihx - lxvb16x - lxsd/stxsd - lxssp/stxssp - lxv/stxv Also, I don't know how bothered we are about P7, but if I'm reading the ISA correctly, lqarx/stqcx. are not supported before ISA 2.07. Likewise a number of the vector instructions like lxsiwzx and lxsiwax (and the companion stores). I realise it's not really the point of this particular patch, so I don't think this should block acceptance. What I would like to know - and maybe this is something where we need mpe to weigh in - is whether we need consistent guards for 2.07 and 3.0. We have some 3.0 guards already but clearly not everywhere. Yes, those needs to be handled properly. Otherwise they can be harmful when emulated on a non-supporting platform. Will work on it when I get some time. Thanks for reporting it. With all that said - the patch does what it says it does, and looks good to me: Reviewed-by: Daniel Axtens Thanks! Ravi
Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
Hi,
To review this, I looked through the supported instructions to see if
there were any that I thought might have been missed.
I didn't find any other v3.1 ones, although I don't have a v3.1 ISA to
hand so I was basically looking for instructions I didn't recognise.
I did, however, find a number of instructions that are new in ISA 3.0
that aren't guarded:
- addpcis
- lxvl/stxvl
- lxvll/stxvll
- lxvwsx
- stxvx
- lxsipzx
- lxvh8x
- lxsihzx
- lxvb16x/stxvb16x
- stxsibx
- stxsihx
- lxvb16x
- lxsd/stxsd
- lxssp/stxssp
- lxv/stxv
Also, I don't know how bothered we are about P7, but if I'm reading the
ISA correctly, lqarx/stqcx. are not supported before ISA 2.07. Likewise
a number of the vector instructions like lxsiwzx and lxsiwax (and the
companion stores).
I realise it's not really the point of this particular patch, so I don't
think this should block acceptance. What I would like to know - and
maybe this is something where we need mpe to weigh in - is whether we
need consistent guards for 2.07 and 3.0. We have some 3.0 guards already
but clearly not everywhere.
With all that said - the patch does what it says it does, and looks good
to me:
Reviewed-by: Daniel Axtens
Kind regards,
Daniel
> From: Balamuruhan S
>
> Unconditional emulation of prefixed instructions will allow
> emulation of them on Power10 predecessors which might cause
> issues. Restrict that.
>
> Fixes: 3920742b92f5 ("powerpc sstep: Add support for prefixed fixed-point
> arithmetic")
> Fixes: 50b80a12e4cc ("powerpc sstep: Add support for prefixed load/stores")
> Signed-off-by: Balamuruhan S
> Signed-off-by: Ravi Bangoria
> ---
> arch/powerpc/lib/sstep.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index e9dcaba9a4f8..e6242744c71b 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1346,6 +1346,9 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> switch (opcode) {
> #ifdef __powerpc64__
> case 1:
> + if (!cpu_has_feature(CPU_FTR_ARCH_31))
> + return -1;
> +
> prefix_r = GET_PREFIX_R(word);
> ra = GET_PREFIX_RA(suffix);
> rd = (suffix >> 21) & 0x1f;
> @@ -2733,6 +2736,9 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> }
> break;
> case 1: /* Prefixed instructions */
> + if (!cpu_has_feature(CPU_FTR_ARCH_31))
> + return -1;
> +
> prefix_r = GET_PREFIX_R(word);
> ra = GET_PREFIX_RA(suffix);
> op->update_reg = ra;
> --
> 2.26.2
Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
Ravi Bangoria 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
Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
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
Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
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?
Kind regards,
Daniel
Re: [PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
On 11/10/20 10:39 am, Ravi Bangoria wrote:
> From: Balamuruhan S
>
> Unconditional emulation of prefixed instructions will allow
> emulation of them on Power10 predecessors which might cause
> issues. Restrict that.
>
> Fixes: 3920742b92f5 ("powerpc sstep: Add support for prefixed fixed-point
> arithmetic")
> Fixes: 50b80a12e4cc ("powerpc sstep: Add support for prefixed load/stores")
> Signed-off-by: Balamuruhan S
> Signed-off-by: Ravi Bangoria
> ---
> arch/powerpc/lib/sstep.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
> index e9dcaba9a4f8..e6242744c71b 100644
> --- a/arch/powerpc/lib/sstep.c
> +++ b/arch/powerpc/lib/sstep.c
> @@ -1346,6 +1346,9 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> switch (opcode) {
> #ifdef __powerpc64__
> case 1:
> + if (!cpu_has_feature(CPU_FTR_ARCH_31))
> + return -1;
> +
> prefix_r = GET_PREFIX_R(word);
> ra = GET_PREFIX_RA(suffix);
> rd = (suffix >> 21) & 0x1f;
> @@ -2733,6 +2736,9 @@ int analyse_instr(struct instruction_op *op, const
> struct pt_regs *regs,
> }
> break;
> case 1: /* Prefixed instructions */
> + if (!cpu_has_feature(CPU_FTR_ARCH_31))
> + return -1;
> +
> prefix_r = GET_PREFIX_R(word);
> ra = GET_PREFIX_RA(suffix);
> op->update_reg = ra;
>
LGTM
Reviewed-by: Sandipan Das
[PATCH v5 1/5] powerpc/sstep: Emulate prefixed instructions only when CPU_FTR_ARCH_31 is set
From: Balamuruhan S
Unconditional emulation of prefixed instructions will allow
emulation of them on Power10 predecessors which might cause
issues. Restrict that.
Fixes: 3920742b92f5 ("powerpc sstep: Add support for prefixed fixed-point
arithmetic")
Fixes: 50b80a12e4cc ("powerpc sstep: Add support for prefixed load/stores")
Signed-off-by: Balamuruhan S
Signed-off-by: Ravi Bangoria
---
arch/powerpc/lib/sstep.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c
index e9dcaba9a4f8..e6242744c71b 100644
--- a/arch/powerpc/lib/sstep.c
+++ b/arch/powerpc/lib/sstep.c
@@ -1346,6 +1346,9 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
switch (opcode) {
#ifdef __powerpc64__
case 1:
+ if (!cpu_has_feature(CPU_FTR_ARCH_31))
+ return -1;
+
prefix_r = GET_PREFIX_R(word);
ra = GET_PREFIX_RA(suffix);
rd = (suffix >> 21) & 0x1f;
@@ -2733,6 +2736,9 @@ int analyse_instr(struct instruction_op *op, const struct
pt_regs *regs,
}
break;
case 1: /* Prefixed instructions */
+ if (!cpu_has_feature(CPU_FTR_ARCH_31))
+ return -1;
+
prefix_r = GET_PREFIX_R(word);
ra = GET_PREFIX_RA(suffix);
op->update_reg = ra;
--
2.26.2
