| Message ID | 20220601054850.250287-1-rmclure@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/6] powerpc: Add ZERO_GPRS macros for register clears | expand |
Le 01/06/2022 à 07:48, Rohan McLure a écrit : > [You don't often get email from rmclure@linux.ibm.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > Macros for restoring saving registers to and from the stack exist. > Provide a macro for simply zeroing a range of gprs, or an individual > gpr. > > Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> > --- > arch/powerpc/include/asm/ppc_asm.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h > index 4dea2d963738..3fb37a9767f7 100644 > --- a/arch/powerpc/include/asm/ppc_asm.h > +++ b/arch/powerpc/include/asm/ppc_asm.h > @@ -33,6 +33,19 @@ > .endr > .endm > > +/* > + * Simplification of OP_REGS, for an arbitrary right hand operand. > + * > + * op reg, rhs You call it "BINOP" but it is rare to have a binary operation with only two operands. Another name could be more appropriate ? Or keep it as BINOP if you see another future use ? In that case have the 3 operands, then 'li r, 0' is same as 'addi r, 0, 0' > + */ > +.macro BINOP_REGS op, rhs, start, end > + .Lreg=\start > + .rept (\end - \start + 1) > + \op .Lreg, \rhs > + .Lreg=.Lreg+1 > + .endr > +.endm > + > /* > * Macros for storing registers into and loading registers from > * exception frames. > @@ -49,6 +62,10 @@ > #define REST_NVGPRS(base) REST_GPRS(13, 31, base) > #endif > > +#define ZERO_GPRS(start, end) BINOP_REGS li, 0, start, end > +#define ZERO_NVGPRS() ZERO_GPRS(14, 31) > +#define ZERO_GPR(n) ZERO_GPRS(n, n) Maybe it would be more explicit to use ZEROIZE_ > + > #define SAVE_GPR(n, base) SAVE_GPRS(n, n, base) > #define REST_GPR(n, base) REST_GPRS(n, n, base) > > -- > 2.34.1 >
Hi! On Wed, Jun 01, 2022 at 03:48:45PM +1000, Rohan McLure wrote: > +.macro BINOP_REGS op, rhs, start, end > + .Lreg=\start > + .rept (\end - \start + 1) > + \op .Lreg, \rhs > + .Lreg=.Lreg+1 > + .endr > +.endm This is for unary operations, not binary operations (there is only one item on the RHS). You can in principle put a string "a,b" in the rhs parameter, but in practice you need a or b to depend on the loop counter as well, so even such trickiness won't do. Make the naming less confusing, maybe? Or don't have an unused extra level of abstraction in the first place :-) Segher
> On 2 Jun 2022, at 2:00 am, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Wed, Jun 01, 2022 at 03:48:45PM +1000, Rohan McLure wrote: >> +.macro BINOP_REGS op, rhs, start, end >> + .Lreg=\start >> + .rept (\end - \start + 1) >> + \op .Lreg, \rhs >> + .Lreg=.Lreg+1 >> + .endr >> +.endm > > This is for unary operations, not binary operations (there is only one > item on the RHS). You can in principle put a string "a,b" in the rhs > parameter, but in practice you need a or b to depend on the loop counter > as well, so even such trickiness won't do. Make the naming less > confusing, maybe? Or don't have an unused extra level of abstraction in > the first place :-) > > > Segher Thanks Segher, Christophe for reviewing this. Yep I see how having a macro to perform rX = rX <> Y for arbitrary infix <> and operand is unlikely to find much use outside of ZERO_GPRS. As I resubmit this patch series I will rename it to ZERO_REGS or similar to be more explicitly coupled to ZERO_GPRS. Something like this I was thinking: .macro ZERO_REGS start, end .Lreg=\start .rept (\end - \start + 1) li .Lreg, 0 .Lreg=.Lreg+1 .endr .endm Thanks, Rohan
Hi! On Fri, Jun 10, 2022 at 01:32:58PM +1000, Rohan McLure wrote: > > On 2 Jun 2022, at 2:00 am, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > This is for unary operations, not binary operations (there is only one > > item on the RHS). You can in principle put a string "a,b" in the rhs > > parameter, but in practice you need a or b to depend on the loop counter > > as well, so even such trickiness won't do. Make the naming less > > confusing, maybe? Or don't have an unused extra level of abstraction in > > the first place :-) > Yep I see how having a macro to perform rX = rX <> Y for arbitrary infix <> and operand > is unlikely to find much use outside of ZERO_GPRS. Aha. On PowerPC (like on most RISC-like architectures) all the normal instructions are rD := rA OP rB, not rD := rD OP rA. It looks like that is our disconnect :-) Segher
Le 10/06/2022 à 05:32, Rohan McLure a écrit : >> On 2 Jun 2022, at 2:00 am, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> >> Hi! >> >> On Wed, Jun 01, 2022 at 03:48:45PM +1000, Rohan McLure wrote: >>> +.macro BINOP_REGS op, rhs, start, end >>> + .Lreg=\start >>> + .rept (\end - \start + 1) >>> + \op .Lreg, \rhs >>> + .Lreg=.Lreg+1 >>> + .endr >>> +.endm >> >> This is for unary operations, not binary operations (there is only one >> item on the RHS). You can in principle put a string "a,b" in the rhs >> parameter, but in practice you need a or b to depend on the loop counter >> as well, so even such trickiness won't do. Make the naming less >> confusing, maybe? Or don't have an unused extra level of abstraction in >> the first place :-) >> >> >> Segher > > Thanks Segher, Christophe for reviewing this. > > Yep I see how having a macro to perform rX = rX <> Y for arbitrary infix <> and operand > is unlikely to find much use outside of ZERO_GPRS. As I resubmit this patch series I > will rename it to ZERO_REGS or similar to be more explicitly coupled to ZERO_GPRS. > > Something like this I was thinking: > > .macro ZERO_REGS start, end > .Lreg=\start > .rept (\end - \start + 1) > li .Lreg, 0 > .Lreg=.Lreg+1 > .endr > .endm > I'd have a preference for using a verb, for instance ZEROISE_REGS or CLEAR_REGS Christophe
On Sat, Jun 11, 2022 at 08:42:27AM +0000, Christophe Leroy wrote: > Le 10/06/2022 à 05:32, Rohan McLure a écrit : > > .macro ZERO_REGS start, end > > .Lreg=\start > > .rept (\end - \start + 1) > > li .Lreg, 0 > > .Lreg=.Lreg+1 > > .endr > > .endm > > I'd have a preference for using a verb, for instance ZEROISE_REGS or > CLEAR_REGS "Zero" is a verb as well (as well as a noun and an adjective) :-) Segher
Segher Boessenkool <segher@kernel.crashing.org> writes: > On Sat, Jun 11, 2022 at 08:42:27AM +0000, Christophe Leroy wrote: >> Le 10/06/2022 à 05:32, Rohan McLure a écrit : >> > .macro ZERO_REGS start, end >> > .Lreg=\start >> > .rept (\end - \start + 1) >> > li .Lreg, 0 >> > .Lreg=.Lreg+1 >> > .endr >> > .endm >> >> I'd have a preference for using a verb, for instance ZEROISE_REGS or >> CLEAR_REGS > > "Zero" is a verb as well (as well as a noun and an adjective) :-) And "clear" is also a verb and an adjective, though helpfully the noun is "clearing" :D We could use "nullify", that has some existing usage in the kernel, although I don't really mind, "zeroise" sounds kind of cool :) cheers
On Tue, Jun 14, 2022 at 02:31:01PM +1000, Michael Ellerman wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > On Sat, Jun 11, 2022 at 08:42:27AM +0000, Christophe Leroy wrote: > >> I'd have a preference for using a verb, for instance ZEROISE_REGS or > >> CLEAR_REGS > > > > "Zero" is a verb as well (as well as a noun and an adjective) :-) > > And "clear" is also a verb and an adjective, though helpfully the noun > is "clearing" :D Yeah. I don't like "clear" here because it isn't as clear what it actually will do. The purpose here is to actually makes those registers hold the number zero, it isn't just to make it harmless some way. Which btw is the context in which "zeroisation" is normally used: in crypto and other security stuff. But "zero_regs" can be confusing in other ways, like, if it isn't clear to the reader it is a verb here. > We could use "nullify", that has some existing usage in the kernel, > although I don't really mind, "zeroise" sounds kind of cool :) That is a benefit yes :-) And it won't be confusing what it does. Segher
diff --git a/arch/powerpc/include/asm/ppc_asm.h b/arch/powerpc/include/asm/ppc_asm.h index 4dea2d963738..3fb37a9767f7 100644 --- a/arch/powerpc/include/asm/ppc_asm.h +++ b/arch/powerpc/include/asm/ppc_asm.h @@ -33,6 +33,19 @@ .endr .endm +/* + * Simplification of OP_REGS, for an arbitrary right hand operand. + * + * op reg, rhs + */ +.macro BINOP_REGS op, rhs, start, end + .Lreg=\start + .rept (\end - \start + 1) + \op .Lreg, \rhs + .Lreg=.Lreg+1 + .endr +.endm + /* * Macros for storing registers into and loading registers from * exception frames. @@ -49,6 +62,10 @@ #define REST_NVGPRS(base) REST_GPRS(13, 31, base) #endif +#define ZERO_GPRS(start, end) BINOP_REGS li, 0, start, end +#define ZERO_NVGPRS() ZERO_GPRS(14, 31) +#define ZERO_GPR(n) ZERO_GPRS(n, n) + #define SAVE_GPR(n, base) SAVE_GPRS(n, n, base) #define REST_GPR(n, base) REST_GPRS(n, n, base)
Macros for restoring saving registers to and from the stack exist. Provide a macro for simply zeroing a range of gprs, or an individual gpr. Signed-off-by: Rohan McLure <rmclure@linux.ibm.com> --- arch/powerpc/include/asm/ppc_asm.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)