| Message ID | 20201011050908.72173-2-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ef6879f8c8053cc3b493f400a06d452d7fb13650 |
| Headers | show |
| Series | powerpc/sstep: VSX 32-byte vector paired load/store instructions | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (118be7377c97e35c33819bcb3bbbae5a42a4ac43) |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 18 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
On 11/10/20 10:39 am, Ravi Bangoria wrote: > From: Balamuruhan S <bala24@linux.ibm.com> > > 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 <bala24@linux.ibm.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > --- > 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 <sandipan@linux.ibm.com>
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
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
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
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 <dja@axtens.net> Kind regards, Daniel > From: Balamuruhan S <bala24@linux.ibm.com> > > 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 <bala24@linux.ibm.com> > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > --- > 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
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 <dja@axtens.net> Thanks! Ravi
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;