| Message ID | 1454951267-30034-3-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: > --- a/xen/arch/x86/xen.lds.S > +++ b/xen/arch/x86/xen.lds.S > @@ -120,6 +120,12 @@ SECTIONS > .init.data : { > *(.init.rodata) > *(.init.rodata.str*) > + > + . = ALIGN(32); Why 32? > + __setup_start = .; > + *(.init.setup) > + __setup_end = .; > + > *(.init.data) > *(.init.data.rel) > *(.init.data.rel.*) > @@ -146,11 +152,6 @@ SECTIONS > __ctors_end = .; > } :text > . = ALIGN(32); > - .init.setup : { > - __setup_start = .; > - *(.init.setup) > - __setup_end = .; > - } :text If just because it was 32 here, I don't think that's a compelling reason. With it (above, not necessarily here) reduced to 8 (which of course could also be done while committing, if you agree and there are no deeper reasons), Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 09/02/16 12:43, Jan Beulich wrote: >>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >> --- a/xen/arch/x86/xen.lds.S >> +++ b/xen/arch/x86/xen.lds.S >> @@ -120,6 +120,12 @@ SECTIONS >> .init.data : { >> *(.init.rodata) >> *(.init.rodata.str*) >> + >> + . = ALIGN(32); > Why 32? > >> + __setup_start = .; >> + *(.init.setup) >> + __setup_end = .; >> + >> *(.init.data) >> *(.init.data.rel) >> *(.init.data.rel.*) >> @@ -146,11 +152,6 @@ SECTIONS >> __ctors_end = .; >> } :text >> . = ALIGN(32); >> - .init.setup : { >> - __setup_start = .; >> - *(.init.setup) >> - __setup_end = .; >> - } :text > If just because it was 32 here, I don't think that's a compelling > reason. With it (above, not necessarily here) reduced to 8 (which > of course could also be done while committing, if you agree and > there are no deeper reasons), > Reviewed-by: Jan Beulich <jbeulich@suse.com> It was just because of the code here. I still can't think of any specific reason why 32 is needed, so the ALIGN() can just be dropped. ~Andrew
>>> On 09.02.16 at 14:52, <andrew.cooper3@citrix.com> wrote: > On 09/02/16 12:43, Jan Beulich wrote: >>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >>> --- a/xen/arch/x86/xen.lds.S >>> +++ b/xen/arch/x86/xen.lds.S >>> @@ -120,6 +120,12 @@ SECTIONS >>> .init.data : { >>> *(.init.rodata) >>> *(.init.rodata.str*) >>> + >>> + . = ALIGN(32); >> Why 32? >> >>> + __setup_start = .; >>> + *(.init.setup) >>> + __setup_end = .; >>> + >>> *(.init.data) >>> *(.init.data.rel) >>> *(.init.data.rel.*) >>> @@ -146,11 +152,6 @@ SECTIONS >>> __ctors_end = .; >>> } :text >>> . = ALIGN(32); >>> - .init.setup : { >>> - __setup_start = .; >>> - *(.init.setup) >>> - __setup_end = .; >>> - } :text >> If just because it was 32 here, I don't think that's a compelling >> reason. With it (above, not necessarily here) reduced to 8 (which >> of course could also be done while committing, if you agree and >> there are no deeper reasons), >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > It was just because of the code here. I still can't think of any > specific reason why 32 is needed, so the ALIGN() can just be dropped. No, dropping ALIGN() altogether would make the placement of __setup_start dependent upon the alignment of the previous section (and since it's a strings section which precedes it, problems would be quite likely). (This is, btw., why it would be better if we used __startof__ and __alignof__ instead of linker script generated symbols. I may give that a try ...) Jan
On 09/02/16 14:32, Jan Beulich wrote: >>>> On 09.02.16 at 14:52, <andrew.cooper3@citrix.com> wrote: >> On 09/02/16 12:43, Jan Beulich wrote: >>>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >>>> --- a/xen/arch/x86/xen.lds.S >>>> +++ b/xen/arch/x86/xen.lds.S >>>> @@ -120,6 +120,12 @@ SECTIONS >>>> .init.data : { >>>> *(.init.rodata) >>>> *(.init.rodata.str*) >>>> + >>>> + . = ALIGN(32); >>> Why 32? >>> >>>> + __setup_start = .; >>>> + *(.init.setup) >>>> + __setup_end = .; >>>> + >>>> *(.init.data) >>>> *(.init.data.rel) >>>> *(.init.data.rel.*) >>>> @@ -146,11 +152,6 @@ SECTIONS >>>> __ctors_end = .; >>>> } :text >>>> . = ALIGN(32); >>>> - .init.setup : { >>>> - __setup_start = .; >>>> - *(.init.setup) >>>> - __setup_end = .; >>>> - } :text >>> If just because it was 32 here, I don't think that's a compelling >>> reason. With it (above, not necessarily here) reduced to 8 (which >>> of course could also be done while committing, if you agree and >>> there are no deeper reasons), >>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> It was just because of the code here. I still can't think of any >> specific reason why 32 is needed, so the ALIGN() can just be dropped. > No, dropping ALIGN() altogether would make the placement of > __setup_start dependent upon the alignment of the previous > section (and since it's a strings section which precedes it, > problems would be quite likely). (This is, btw., why it would be > better if we used __startof__ and __alignof__ instead of linker > script generated symbols. I may give that a try ...) Actually, thinking about it, andrewcoop@andrewcoop:/local/xen.git/xen$ pahole xen-syms -C kernel_param struct kernel_param { const char * name; /* 0 8 */ enum { OPT_STR = 0, OPT_UINT = 1, OPT_BOOL = 2, OPT_SIZE = 3, OPT_CUSTOM = 4, } type; /* 8 4 */ /* XXX 4 bytes hole, try to pack */ void * var; /* 16 8 */ unsigned int len; /* 24 4 */ /* size: 32, cachelines: 1, members: 4 */ /* sum members: 24, holes: 1, sum holes: 4 */ /* padding: 4 */ /* last cacheline: 32 bytes */ }; This will be where the 32 byte alignment comes from, although dropping the structure size to 24 bytes would be trivial. ~Andrew
>>> On 09.02.16 at 15:40, <andrew.cooper3@citrix.com> wrote: > On 09/02/16 14:32, Jan Beulich wrote: >>>>> On 09.02.16 at 14:52, <andrew.cooper3@citrix.com> wrote: >>> On 09/02/16 12:43, Jan Beulich wrote: >>>>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >>>>> --- a/xen/arch/x86/xen.lds.S >>>>> +++ b/xen/arch/x86/xen.lds.S >>>>> @@ -120,6 +120,12 @@ SECTIONS >>>>> .init.data : { >>>>> *(.init.rodata) >>>>> *(.init.rodata.str*) >>>>> + >>>>> + . = ALIGN(32); >>>> Why 32? >>>> >>>>> + __setup_start = .; >>>>> + *(.init.setup) >>>>> + __setup_end = .; >>>>> + >>>>> *(.init.data) >>>>> *(.init.data.rel) >>>>> *(.init.data.rel.*) >>>>> @@ -146,11 +152,6 @@ SECTIONS >>>>> __ctors_end = .; >>>>> } :text >>>>> . = ALIGN(32); >>>>> - .init.setup : { >>>>> - __setup_start = .; >>>>> - *(.init.setup) >>>>> - __setup_end = .; >>>>> - } :text >>>> If just because it was 32 here, I don't think that's a compelling >>>> reason. With it (above, not necessarily here) reduced to 8 (which >>>> of course could also be done while committing, if you agree and >>>> there are no deeper reasons), >>>> Reviewed-by: Jan Beulich <jbeulich@suse.com> >>> It was just because of the code here. I still can't think of any >>> specific reason why 32 is needed, so the ALIGN() can just be dropped. >> No, dropping ALIGN() altogether would make the placement of >> __setup_start dependent upon the alignment of the previous >> section (and since it's a strings section which precedes it, >> problems would be quite likely). (This is, btw., why it would be >> better if we used __startof__ and __alignof__ instead of linker >> script generated symbols. I may give that a try ...) > > Actually, thinking about it, > > andrewcoop@andrewcoop:/local/xen.git/xen$ pahole xen-syms -C kernel_param > struct kernel_param { > const char * name; /* 0 8 */ > enum { > OPT_STR = 0, > OPT_UINT = 1, > OPT_BOOL = 2, > OPT_SIZE = 3, > OPT_CUSTOM = 4, > } type; /* 8 4 */ > > /* XXX 4 bytes hole, try to pack */ > > void * var; /* 16 8 */ > unsigned int len; /* 24 4 */ > > /* size: 32, cachelines: 1, members: 4 */ > /* sum members: 24, holes: 1, sum holes: 4 */ > /* padding: 4 */ > /* last cacheline: 32 bytes */ > }; > > This will be where the 32 byte alignment comes from, But that's the structure _size_, unrelated to any needed alignment. > although dropping > the structure size to 24 bytes would be trivial. Patch already done (to be re-based on top of yours). Jan
>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: > There is no reason for any of it to be modified. Additionally, link > .init.setup beside the other constant .init data. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Sadly I've noticed only after pushing that this breaks the build with older gcc. 4.3.4 complains about a section type conflict in mwait-idle.c. I've reverted the change for the time being. Jan
On 22/02/16 16:36, Jan Beulich wrote: >>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >> There is no reason for any of it to be modified. Additionally, link >> .init.setup beside the other constant .init data. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Sadly I've noticed only after pushing that this breaks the build > with older gcc. 4.3.4 complains about a section type conflict in > mwait-idle.c. I've reverted the change for the time being. /sigh - I will investigate. ~Andrew
On 2/22/16 10:36 AM, Jan Beulich wrote: >>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >> There is no reason for any of it to be modified. Additionally, link >> .init.setup beside the other constant .init data. >> >> No functional change. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Sadly I've noticed only after pushing that this breaks the build > with older gcc. 4.3.4 complains about a section type conflict in > mwait-idle.c. I've reverted the change for the time being. > > Jan > Jan, Can you tell me the distro (and any specifics you've got) which has this environment? I'm setting up more build tests and I'll add this to there.
>>> On 22.02.16 at 19:43, <cardoe@cardoe.com> wrote: > On 2/22/16 10:36 AM, Jan Beulich wrote: >>>>> On 08.02.16 at 18:07, <andrew.cooper3@citrix.com> wrote: >>> There is no reason for any of it to be modified. Additionally, link >>> .init.setup beside the other constant .init data. >>> >>> No functional change. >>> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> >> Sadly I've noticed only after pushing that this breaks the build >> with older gcc. 4.3.4 complains about a section type conflict in >> mwait-idle.c. I've reverted the change for the time being. > > Can you tell me the distro (and any specifics you've got) which has this > environment? I'm setting up more build tests and I'll add this to there. SLE11; I don't think any other aspects matter much, nor do I think the specific distro matters, as I don't expect we would have too may patches on top of the respective upstream gcc. Jan
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S index f501a2f..0e2c582 100644 --- a/xen/arch/arm/xen.lds.S +++ b/xen/arch/arm/xen.lds.S @@ -115,6 +115,12 @@ SECTIONS .init.data : { *(.init.rodata) *(.init.rodata.str*) + + . = ALIGN(32); + __setup_start = .; + *(.init.setup) + __setup_end = .; + *(.init.data) *(.init.data.rel) *(.init.data.rel.*) @@ -125,11 +131,6 @@ SECTIONS __ctors_end = .; } :text . = ALIGN(32); - .init.setup : { - __setup_start = .; - *(.init.setup) - __setup_end = .; - } :text .init.proc.info : { __proc_info_start = .; *(.init.proc.info) diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S index c1ce027..86905c8 100644 --- a/xen/arch/x86/xen.lds.S +++ b/xen/arch/x86/xen.lds.S @@ -120,6 +120,12 @@ SECTIONS .init.data : { *(.init.rodata) *(.init.rodata.str*) + + . = ALIGN(32); + __setup_start = .; + *(.init.setup) + __setup_end = .; + *(.init.data) *(.init.data.rel) *(.init.data.rel.*) @@ -146,11 +152,6 @@ SECTIONS __ctors_end = .; } :text . = ALIGN(32); - .init.setup : { - __setup_start = .; - *(.init.setup) - __setup_end = .; - } :text .initcall.init : { __initcall_start = .; *(.initcallpresmp.init) diff --git a/xen/include/xen/init.h b/xen/include/xen/init.h index 6081102..142c5c8 100644 --- a/xen/include/xen/init.h +++ b/xen/include/xen/init.h @@ -87,8 +87,8 @@ struct kernel_param { extern struct kernel_param __setup_start, __setup_end; -#define __setup_str static __initdata __attribute__((__aligned__(1))) char -#define __kparam static __initsetup struct kernel_param +#define __setup_str static const __initconst __attribute__((__aligned__(1))) char +#define __kparam static const __initsetup struct kernel_param #define custom_param(_name, _var) \ __setup_str __setup_str_##_var[] = _name; \
There is no reason for any of it to be modified. Additionally, link .init.setup beside the other constant .init data. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Tim Deegan <tim@xen.org> CC: Ian Campbell <Ian.Campbell@citrix.com> CC: Stefano Stabellini <stefano.stabellini@citrix.com> --- xen/arch/arm/xen.lds.S | 11 ++++++----- xen/arch/x86/xen.lds.S | 11 ++++++----- xen/include/xen/init.h | 4 ++-- 3 files changed, 14 insertions(+), 12 deletions(-)