| Message ID | 20220829055223.24767-3-sv@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | objtool: Enable and implement --mcount option on powerpc | expand |
Le 29/08/2022 à 07:52, Sathvika Vasireddy a écrit : > Powerpc instructions must be word-aligned. Currently, > there is an alignment of 16 bytes (by default), and it is > much more than what is required for powerpc (4 bytes). > > The default expansion of __ALIGN() macro is: > #define __ALIGN .align 4,0x90 > > Since Powerpc Linux does not require a 16 byte alignment, > override __ALIGN() and __ALIGN_STR() macros to use required > 4 byte alignment. > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > arch/powerpc/include/asm/linkage.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/include/asm/linkage.h b/arch/powerpc/include/asm/linkage.h > index b71b9582e754..8df88fe61438 100644 > --- a/arch/powerpc/include/asm/linkage.h > +++ b/arch/powerpc/include/asm/linkage.h > @@ -2,8 +2,12 @@ > #ifndef _ASM_POWERPC_LINKAGE_H > #define _ASM_POWERPC_LINKAGE_H > > +#include <linux/stringify.h> > #include <asm/types.h> > > +#define __ALIGN .align 2 > +#define __ALIGN_STR __stringify(__ALIGN) > + I still can't see the added value of using __stringify() macro here. In order to use that macro you have to include linux/stringify.h . Usually we try to minimise the amount of headers required by other headers. > #ifdef CONFIG_PPC64_ELF_ABI_V1 > #define cond_syscall(x) \ > asm ("\t.weak " #x "\n\t.set " #x ", sys_ni_syscall\n" \
Hi Christophe, On 29/08/22 18:56, Christophe Leroy wrote: > > Le 29/08/2022 à 07:52, Sathvika Vasireddy a écrit : >> Powerpc instructions must be word-aligned. Currently, >> there is an alignment of 16 bytes (by default), and it is >> much more than what is required for powerpc (4 bytes). >> >> The default expansion of __ALIGN() macro is: >> #define __ALIGN .align 4,0x90 >> >> Since Powerpc Linux does not require a 16 byte alignment, >> override __ALIGN() and __ALIGN_STR() macros to use required >> 4 byte alignment. >> >> Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> >> --- >> arch/powerpc/include/asm/linkage.h | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/arch/powerpc/include/asm/linkage.h b/arch/powerpc/include/asm/linkage.h >> index b71b9582e754..8df88fe61438 100644 >> --- a/arch/powerpc/include/asm/linkage.h >> +++ b/arch/powerpc/include/asm/linkage.h >> @@ -2,8 +2,12 @@ >> #ifndef _ASM_POWERPC_LINKAGE_H >> #define _ASM_POWERPC_LINKAGE_H >> >> +#include <linux/stringify.h> >> #include <asm/types.h> >> >> +#define __ALIGN .align 2 >> +#define __ALIGN_STR __stringify(__ALIGN) >> + > I still can't see the added value of using __stringify() macro here. In > order to use that macro you have to include linux/stringify.h . Usually > we try to minimise the amount of headers required by other headers. Oh ok, makes sense to me. I'll wait for a day to see if there are any other comments, and make this change as part of v3. Thanks for reviewing! - Sathvika
On Mon Aug 29, 2022 at 3:52 PM AEST, Sathvika Vasireddy wrote: > Powerpc instructions must be word-aligned. Currently, > there is an alignment of 16 bytes (by default), and it is > much more than what is required for powerpc (4 bytes). > > The default expansion of __ALIGN() macro is: > #define __ALIGN .align 4,0x90 > > Since Powerpc Linux does not require a 16 byte alignment, > override __ALIGN() and __ALIGN_STR() macros to use required > 4 byte alignment. Alignment can be desirable beyond the minimum requirement, for example 16 byte alignment for functions could be helpful for instruction fetch. So it should be explained why possible benefits of the larger alignment are not worth it. And if you have the patch in a series, it should be explained why the patch is required for the series if it is not obvious. Thanks, Nick > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > arch/powerpc/include/asm/linkage.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/include/asm/linkage.h b/arch/powerpc/include/asm/linkage.h > index b71b9582e754..8df88fe61438 100644 > --- a/arch/powerpc/include/asm/linkage.h > +++ b/arch/powerpc/include/asm/linkage.h > @@ -2,8 +2,12 @@ > #ifndef _ASM_POWERPC_LINKAGE_H > #define _ASM_POWERPC_LINKAGE_H > > +#include <linux/stringify.h> > #include <asm/types.h> > > +#define __ALIGN .align 2 > +#define __ALIGN_STR __stringify(__ALIGN) > + > #ifdef CONFIG_PPC64_ELF_ABI_V1 > #define cond_syscall(x) \ > asm ("\t.weak " #x "\n\t.set " #x ", sys_ni_syscall\n" \ > -- > 2.31.1
"Nicholas Piggin" <npiggin@gmail.com> writes: > On Mon Aug 29, 2022 at 3:52 PM AEST, Sathvika Vasireddy wrote: >> Powerpc instructions must be word-aligned. Currently, >> there is an alignment of 16 bytes (by default), and it is >> much more than what is required for powerpc (4 bytes). >> >> The default expansion of __ALIGN() macro is: >> #define __ALIGN .align 4,0x90 >> >> Since Powerpc Linux does not require a 16 byte alignment, >> override __ALIGN() and __ALIGN_STR() macros to use required >> 4 byte alignment. > > Alignment can be desirable beyond the minimum requirement, for > example 16 byte alignment for functions could be helpful for > instruction fetch. So it should be explained why possible > benefits of the larger alignment are not worth it. Using ".align 2" matches what our existing _GLOBAL macro does. So this change basically just propagates that existing alignment into this new macro, which is used for similar purposes. So if we want to increase the alignment we should do that explicitly, and update _GLOBAL at the same time. The change log should probably just say ~= "use the same alignment as the existing _GLOBAL macro". What's more important, but not mentioned in the change log, is that we don't want to pad with 0x90, because repeated 0x90s are not a nop or trap on powerpc. cheers
On Mon, Aug 29, 2022 at 11:22:09AM +0530, Sathvika Vasireddy wrote: > Powerpc instructions must be word-aligned. Currently, > there is an alignment of 16 bytes (by default), and it is > much more than what is required for powerpc (4 bytes). > > The default expansion of __ALIGN() macro is: > #define __ALIGN .align 4,0x90 > > Since Powerpc Linux does not require a 16 byte alignment, > override __ALIGN() and __ALIGN_STR() macros to use required > 4 byte alignment. > > Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> > --- > arch/powerpc/include/asm/linkage.h | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/arch/powerpc/include/asm/linkage.h b/arch/powerpc/include/asm/linkage.h > index b71b9582e754..8df88fe61438 100644 > --- a/arch/powerpc/include/asm/linkage.h > +++ b/arch/powerpc/include/asm/linkage.h > @@ -2,8 +2,12 @@ > #ifndef _ASM_POWERPC_LINKAGE_H > #define _ASM_POWERPC_LINKAGE_H > > +#include <linux/stringify.h> > #include <asm/types.h> > > +#define __ALIGN .align 2 > +#define __ALIGN_STR __stringify(__ALIGN) Related thread: https://lkml.kernel.org/r/YxXJswK9QjhCGmPN@hirez.programming.kicks-ass.net
diff --git a/arch/powerpc/include/asm/linkage.h b/arch/powerpc/include/asm/linkage.h index b71b9582e754..8df88fe61438 100644 --- a/arch/powerpc/include/asm/linkage.h +++ b/arch/powerpc/include/asm/linkage.h @@ -2,8 +2,12 @@ #ifndef _ASM_POWERPC_LINKAGE_H #define _ASM_POWERPC_LINKAGE_H +#include <linux/stringify.h> #include <asm/types.h> +#define __ALIGN .align 2 +#define __ALIGN_STR __stringify(__ALIGN) + #ifdef CONFIG_PPC64_ELF_ABI_V1 #define cond_syscall(x) \ asm ("\t.weak " #x "\n\t.set " #x ", sys_ni_syscall\n" \
Powerpc instructions must be word-aligned. Currently, there is an alignment of 16 bytes (by default), and it is much more than what is required for powerpc (4 bytes). The default expansion of __ALIGN() macro is: #define __ALIGN .align 4,0x90 Since Powerpc Linux does not require a 16 byte alignment, override __ALIGN() and __ALIGN_STR() macros to use required 4 byte alignment. Signed-off-by: Sathvika Vasireddy <sv@linux.ibm.com> --- arch/powerpc/include/asm/linkage.h | 4 ++++ 1 file changed, 4 insertions(+)