| Message ID | 20250107-elf-v3-5-99cb505b1ab2@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | elf: Define note name macros | expand |
Hi, On Tue, Jan 07, 2025 at 09:45:56PM +0900, Akihiko Odaki wrote: > Use note name macros to match with the userspace's expectation. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > arch/s390/kernel/crash_dump.c | 62 ++++++++++++++++--------------------------- > 1 file changed, 23 insertions(+), 39 deletions(-) > > diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c [...] > @@ -281,10 +272,8 @@ static void *nt_init_name(void *buf, Elf64_Word type, void *desc, int d_len, > return PTR_ADD(buf, len); > } > > -static inline void *nt_init(void *buf, Elf64_Word type, void *desc, int d_len) > -{ > - return nt_init_name(buf, type, desc, d_len, nt_name(type)); > -} > +#define NT_INIT(buf, type, desc) \ > + (nt_init_name((buf), NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) Nit: this macro name clashes with the naming scheme in elf.h. I think that there is a (weak) convention that macros with upper-case names don't expand to a C function call; thus, a macro with an upper- case name can be invoked in places where a C function call would not be allowed. (This convention is not followed everywhere, though -- it's up to the maintainer what they prefer here.) (Note also, the outer parentheses and the parentheses around (buf) appear redundant -- although harmless?) > > /* > * Calculate the size of ELF note > @@ -300,10 +289,7 @@ static size_t nt_size_name(int d_len, const char *name) > return size; > } > > -static inline size_t nt_size(Elf64_Word type, int d_len) > -{ > - return nt_size_name(d_len, nt_name(type)); > -} > +#define NT_SIZE(type, desc) (nt_size_name(sizeof(desc), NN_ ## type)) Nit: name prefix clash (again); possibly redundant parentheses. [...] > @@ -348,16 +332,16 @@ static size_t get_cpu_elf_notes_size(void) > struct save_area *sa = NULL; > size_t size; > > - size = nt_size(NT_PRSTATUS, sizeof(struct elf_prstatus)); > - size += nt_size(NT_PRFPREG, sizeof(elf_fpregset_t)); > - size += nt_size(NT_S390_TIMER, sizeof(sa->timer)); > - size += nt_size(NT_S390_TODCMP, sizeof(sa->todcmp)); > - size += nt_size(NT_S390_TODPREG, sizeof(sa->todpreg)); > - size += nt_size(NT_S390_CTRS, sizeof(sa->ctrs)); > - size += nt_size(NT_S390_PREFIX, sizeof(sa->prefix)); > + size = NT_SIZE(PRSTATUS, struct elf_prstatus); > + size += NT_SIZE(PRFPREG, elf_fpregset_t); > + size += NT_SIZE(S390_TIMER, sa->timer); > + size += NT_SIZE(S390_TODCMP, sa->todcmp); > + size += NT_SIZE(S390_TODPREG, sa->todpreg); > + size += NT_SIZE(S390_CTRS, sa->ctrs); > + size += NT_SIZE(S390_PREFIX, sa->prefix); It might be worth fixing the funny spacing on these lines, since all the affected lines are being replaced. > if (cpu_has_vx()) { > - size += nt_size(NT_S390_VXRS_HIGH, sizeof(sa->vxrs_high)); > - size += nt_size(NT_S390_VXRS_LOW, sizeof(sa->vxrs_low)); > + size += NT_SIZE(S390_VXRS_HIGH, sa->vxrs_high); > + size += NT_SIZE(S390_VXRS_LOW, sa->vxrs_low); > } > > return size; > @@ -373,7 +357,7 @@ static void *nt_prpsinfo(void *ptr) > memset(&prpsinfo, 0, sizeof(prpsinfo)); > prpsinfo.pr_sname = 'R'; > strcpy(prpsinfo.pr_fname, "vmlinux"); > - return nt_init(ptr, NT_PRPSINFO, &prpsinfo, sizeof(prpsinfo)); > + return NT_INIT(ptr, PRPSINFO, prpsinfo); > } > > /* > @@ -589,7 +573,7 @@ static size_t get_elfcorehdr_size(int phdr_count) > /* PT_NOTES */ > size += sizeof(Elf64_Phdr); > /* nt_prpsinfo */ > - size += nt_size(NT_PRPSINFO, sizeof(struct elf_prpsinfo)); > + size += NT_SIZE(PRPSINFO, struct elf_prpsinfo); > /* regsets */ > size += get_cpu_cnt() * get_cpu_elf_notes_size(); > /* nt_vmcoreinfo */ Otherwise, this looks sensible to me. Cheers ---Dave
On 2025/01/08 1:17, Dave Martin wrote: > Hi, > > On Tue, Jan 07, 2025 at 09:45:56PM +0900, Akihiko Odaki wrote: >> Use note name macros to match with the userspace's expectation. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> arch/s390/kernel/crash_dump.c | 62 ++++++++++++++++--------------------------- >> 1 file changed, 23 insertions(+), 39 deletions(-) >> >> diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c > > [...] > >> @@ -281,10 +272,8 @@ static void *nt_init_name(void *buf, Elf64_Word type, void *desc, int d_len, >> return PTR_ADD(buf, len); >> } >> >> -static inline void *nt_init(void *buf, Elf64_Word type, void *desc, int d_len) >> -{ >> - return nt_init_name(buf, type, desc, d_len, nt_name(type)); >> -} >> +#define NT_INIT(buf, type, desc) \ >> + (nt_init_name((buf), NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) > > Nit: this macro name clashes with the naming scheme in elf.h. > > I think that there is a (weak) convention that macros with upper-case > names don't expand to a C function call; thus, a macro with an upper- > case name can be invoked in places where a C function call would not be > allowed. (This convention is not followed everywhere, though -- it's > up to the maintainer what they prefer here.) I wanted to clarify it is a macro as it concatenates tokens with ##, but I also find there are many macros that are named lower-case and performs token concatenation. S390 maintainers, please tell usr your opinion. > > (Note also, the outer parentheses and the parentheses around (buf) > appear redundant -- although harmless?) They only make a difference in trivial corner cases and may look needlessly verbose. > >> >> /* >> * Calculate the size of ELF note >> @@ -300,10 +289,7 @@ static size_t nt_size_name(int d_len, const char *name) >> return size; >> } >> >> -static inline size_t nt_size(Elf64_Word type, int d_len) >> -{ >> - return nt_size_name(d_len, nt_name(type)); >> -} >> +#define NT_SIZE(type, desc) (nt_size_name(sizeof(desc), NN_ ## type)) > > Nit: name prefix clash (again); possibly redundant parentheses. > > [...] > >> @@ -348,16 +332,16 @@ static size_t get_cpu_elf_notes_size(void) >> struct save_area *sa = NULL; >> size_t size; >> >> - size = nt_size(NT_PRSTATUS, sizeof(struct elf_prstatus)); >> - size += nt_size(NT_PRFPREG, sizeof(elf_fpregset_t)); >> - size += nt_size(NT_S390_TIMER, sizeof(sa->timer)); >> - size += nt_size(NT_S390_TODCMP, sizeof(sa->todcmp)); >> - size += nt_size(NT_S390_TODPREG, sizeof(sa->todpreg)); >> - size += nt_size(NT_S390_CTRS, sizeof(sa->ctrs)); >> - size += nt_size(NT_S390_PREFIX, sizeof(sa->prefix)); >> + size = NT_SIZE(PRSTATUS, struct elf_prstatus); >> + size += NT_SIZE(PRFPREG, elf_fpregset_t); >> + size += NT_SIZE(S390_TIMER, sa->timer); >> + size += NT_SIZE(S390_TODCMP, sa->todcmp); >> + size += NT_SIZE(S390_TODPREG, sa->todpreg); >> + size += NT_SIZE(S390_CTRS, sa->ctrs); >> + size += NT_SIZE(S390_PREFIX, sa->prefix); > > It might be worth fixing the funny spacing on these lines, since all > the affected lines are being replaced. > >> if (cpu_has_vx()) { >> - size += nt_size(NT_S390_VXRS_HIGH, sizeof(sa->vxrs_high)); >> - size += nt_size(NT_S390_VXRS_LOW, sizeof(sa->vxrs_low)); >> + size += NT_SIZE(S390_VXRS_HIGH, sa->vxrs_high); >> + size += NT_SIZE(S390_VXRS_LOW, sa->vxrs_low); >> } >> >> return size; >> @@ -373,7 +357,7 @@ static void *nt_prpsinfo(void *ptr) >> memset(&prpsinfo, 0, sizeof(prpsinfo)); >> prpsinfo.pr_sname = 'R'; >> strcpy(prpsinfo.pr_fname, "vmlinux"); >> - return nt_init(ptr, NT_PRPSINFO, &prpsinfo, sizeof(prpsinfo)); >> + return NT_INIT(ptr, PRPSINFO, prpsinfo); >> } >> >> /* >> @@ -589,7 +573,7 @@ static size_t get_elfcorehdr_size(int phdr_count) >> /* PT_NOTES */ >> size += sizeof(Elf64_Phdr); >> /* nt_prpsinfo */ >> - size += nt_size(NT_PRPSINFO, sizeof(struct elf_prpsinfo)); >> + size += NT_SIZE(PRPSINFO, struct elf_prpsinfo); >> /* regsets */ >> size += get_cpu_cnt() * get_cpu_elf_notes_size(); >> /* nt_vmcoreinfo */ > > Otherwise, this looks sensible to me. > > Cheers > ---Dave
On Wed, Jan 08, 2025 at 01:53:51PM +0900, Akihiko Odaki wrote: > On 2025/01/08 1:17, Dave Martin wrote: > > > +#define NT_INIT(buf, type, desc) \ > > > + (nt_init_name((buf), NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) > > > > Nit: this macro name clashes with the naming scheme in elf.h. > > > > I think that there is a (weak) convention that macros with upper-case > > names don't expand to a C function call; thus, a macro with an upper- > > case name can be invoked in places where a C function call would not be > > allowed. (This convention is not followed everywhere, though -- it's > > up to the maintainer what they prefer here.) > > I wanted to clarify it is a macro as it concatenates tokens with ##, but I > also find there are many macros that are named lower-case and performs token > concatenation. > > S390 maintainers, please tell usr your opinion. Just make the new macros lower case to avoid the naming scheme clashes, please. Otherwise it doesn't matter too much. > > > +#define NT_SIZE(type, desc) (nt_size_name(sizeof(desc), NN_ ## type)) > > > > Nit: name prefix clash (again); possibly redundant parentheses. Same here. > > > - size = nt_size(NT_PRSTATUS, sizeof(struct elf_prstatus)); > > > - size += nt_size(NT_PRFPREG, sizeof(elf_fpregset_t)); > > > - size += nt_size(NT_S390_TIMER, sizeof(sa->timer)); > > > - size += nt_size(NT_S390_TODCMP, sizeof(sa->todcmp)); > > > - size += nt_size(NT_S390_TODPREG, sizeof(sa->todpreg)); > > > - size += nt_size(NT_S390_CTRS, sizeof(sa->ctrs)); > > > - size += nt_size(NT_S390_PREFIX, sizeof(sa->prefix)); > > > + size = NT_SIZE(PRSTATUS, struct elf_prstatus); > > > + size += NT_SIZE(PRFPREG, elf_fpregset_t); > > > + size += NT_SIZE(S390_TIMER, sa->timer); > > > + size += NT_SIZE(S390_TODCMP, sa->todcmp); > > > + size += NT_SIZE(S390_TODPREG, sa->todpreg); > > > + size += NT_SIZE(S390_CTRS, sa->ctrs); > > > + size += NT_SIZE(S390_PREFIX, sa->prefix); > > > > It might be worth fixing the funny spacing on these lines, since all > > the affected lines are being replaced. Yes, please! Besides that this looks good: Acked-by: Heiko Carstens <hca@linux.ibm.com>
On Wed, Jan 08, 2025 at 01:53:51PM +0900, Akihiko Odaki wrote: > On 2025/01/08 1:17, Dave Martin wrote: > > Hi, > > > > On Tue, Jan 07, 2025 at 09:45:56PM +0900, Akihiko Odaki wrote: > > > Use note name macros to match with the userspace's expectation. > > > > > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > > > --- > > > arch/s390/kernel/crash_dump.c | 62 ++++++++++++++++--------------------------- > > > 1 file changed, 23 insertions(+), 39 deletions(-) > > > > > > diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c > > > > [...] > > > +#define NT_INIT(buf, type, desc) \ > > > + (nt_init_name((buf), NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) [...] > > (Note also, the outer parentheses and the parentheses around (buf) > > appear redundant -- although harmless?) > > They only make a difference in trivial corner cases and may look needlessly > verbose. (In case there was a misunderstanding here, I meant that some parentheses can be removed without affecting correctness: #define NT_INIT(buf, type, desc) \ nt_init_name(buf, NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) It still doesn't matter though -- and some people do prefer to be defensive anyway and err on the side of having too many parentheses rather than too few.) [...] Cheers ---Dave
On 2025/01/08 22:50, Dave Martin wrote: > On Wed, Jan 08, 2025 at 01:53:51PM +0900, Akihiko Odaki wrote: >> On 2025/01/08 1:17, Dave Martin wrote: >>> Hi, >>> >>> On Tue, Jan 07, 2025 at 09:45:56PM +0900, Akihiko Odaki wrote: >>>> Use note name macros to match with the userspace's expectation. >>>> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >>>> --- >>>> arch/s390/kernel/crash_dump.c | 62 ++++++++++++++++--------------------------- >>>> 1 file changed, 23 insertions(+), 39 deletions(-) >>>> >>>> diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c >>> >>> [...] > >>>> +#define NT_INIT(buf, type, desc) \ >>>> + (nt_init_name((buf), NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) > > [...] > >>> (Note also, the outer parentheses and the parentheses around (buf) >>> appear redundant -- although harmless?) >> >> They only make a difference in trivial corner cases and may look needlessly >> verbose. > > (In case there was a misunderstanding here, I meant that some > parentheses can be removed without affecting correctness: > > #define NT_INIT(buf, type, desc) \ > nt_init_name(buf, NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) > > It still doesn't matter though -- and some people do prefer to be > defensive anyway and err on the side of having too many parentheses > rather than too few.) Well, being very pedantic, there are some cases where these parentheses have some effect. If you omit the outer parentheses, the following code will have different consequences: a->NT_INIT(buf, PRSTATUS, desc) The parentheses around buf will make difference for the following code: #define COMMA , NT_INIT(NULL COMMA buf, PRSTATUS, desc) But nobody will write such code. Regards, Akihiko Odaki
diff --git a/arch/s390/kernel/crash_dump.c b/arch/s390/kernel/crash_dump.c index cd0c93a8fb8b..6359ce645756 100644 --- a/arch/s390/kernel/crash_dump.c +++ b/arch/s390/kernel/crash_dump.c @@ -248,15 +248,6 @@ bool is_kdump_kernel(void) } EXPORT_SYMBOL_GPL(is_kdump_kernel); -static const char *nt_name(Elf64_Word type) -{ - const char *name = "LINUX"; - - if (type == NT_PRPSINFO || type == NT_PRSTATUS || type == NT_PRFPREG) - name = KEXEC_CORE_NOTE_NAME; - return name; -} - /* * Initialize ELF note */ @@ -281,10 +272,8 @@ static void *nt_init_name(void *buf, Elf64_Word type, void *desc, int d_len, return PTR_ADD(buf, len); } -static inline void *nt_init(void *buf, Elf64_Word type, void *desc, int d_len) -{ - return nt_init_name(buf, type, desc, d_len, nt_name(type)); -} +#define NT_INIT(buf, type, desc) \ + (nt_init_name((buf), NT_ ## type, &(desc), sizeof(desc), NN_ ## type)) /* * Calculate the size of ELF note @@ -300,10 +289,7 @@ static size_t nt_size_name(int d_len, const char *name) return size; } -static inline size_t nt_size(Elf64_Word type, int d_len) -{ - return nt_size_name(d_len, nt_name(type)); -} +#define NT_SIZE(type, desc) (nt_size_name(sizeof(desc), NN_ ## type)) /* * Fill ELF notes for one CPU with save area registers @@ -324,18 +310,16 @@ static void *fill_cpu_elf_notes(void *ptr, int cpu, struct save_area *sa) memcpy(&nt_fpregset.fpc, &sa->fpc, sizeof(sa->fpc)); memcpy(&nt_fpregset.fprs, &sa->fprs, sizeof(sa->fprs)); /* Create ELF notes for the CPU */ - ptr = nt_init(ptr, NT_PRSTATUS, &nt_prstatus, sizeof(nt_prstatus)); - ptr = nt_init(ptr, NT_PRFPREG, &nt_fpregset, sizeof(nt_fpregset)); - ptr = nt_init(ptr, NT_S390_TIMER, &sa->timer, sizeof(sa->timer)); - ptr = nt_init(ptr, NT_S390_TODCMP, &sa->todcmp, sizeof(sa->todcmp)); - ptr = nt_init(ptr, NT_S390_TODPREG, &sa->todpreg, sizeof(sa->todpreg)); - ptr = nt_init(ptr, NT_S390_CTRS, &sa->ctrs, sizeof(sa->ctrs)); - ptr = nt_init(ptr, NT_S390_PREFIX, &sa->prefix, sizeof(sa->prefix)); + ptr = NT_INIT(ptr, PRSTATUS, nt_prstatus); + ptr = NT_INIT(ptr, PRFPREG, nt_fpregset); + ptr = NT_INIT(ptr, S390_TIMER, sa->timer); + ptr = NT_INIT(ptr, S390_TODCMP, sa->todcmp); + ptr = NT_INIT(ptr, S390_TODPREG, sa->todpreg); + ptr = NT_INIT(ptr, S390_CTRS, sa->ctrs); + ptr = NT_INIT(ptr, S390_PREFIX, sa->prefix); if (cpu_has_vx()) { - ptr = nt_init(ptr, NT_S390_VXRS_HIGH, - &sa->vxrs_high, sizeof(sa->vxrs_high)); - ptr = nt_init(ptr, NT_S390_VXRS_LOW, - &sa->vxrs_low, sizeof(sa->vxrs_low)); + ptr = NT_INIT(ptr, S390_VXRS_HIGH, sa->vxrs_high); + ptr = NT_INIT(ptr, S390_VXRS_LOW, sa->vxrs_low); } return ptr; } @@ -348,16 +332,16 @@ static size_t get_cpu_elf_notes_size(void) struct save_area *sa = NULL; size_t size; - size = nt_size(NT_PRSTATUS, sizeof(struct elf_prstatus)); - size += nt_size(NT_PRFPREG, sizeof(elf_fpregset_t)); - size += nt_size(NT_S390_TIMER, sizeof(sa->timer)); - size += nt_size(NT_S390_TODCMP, sizeof(sa->todcmp)); - size += nt_size(NT_S390_TODPREG, sizeof(sa->todpreg)); - size += nt_size(NT_S390_CTRS, sizeof(sa->ctrs)); - size += nt_size(NT_S390_PREFIX, sizeof(sa->prefix)); + size = NT_SIZE(PRSTATUS, struct elf_prstatus); + size += NT_SIZE(PRFPREG, elf_fpregset_t); + size += NT_SIZE(S390_TIMER, sa->timer); + size += NT_SIZE(S390_TODCMP, sa->todcmp); + size += NT_SIZE(S390_TODPREG, sa->todpreg); + size += NT_SIZE(S390_CTRS, sa->ctrs); + size += NT_SIZE(S390_PREFIX, sa->prefix); if (cpu_has_vx()) { - size += nt_size(NT_S390_VXRS_HIGH, sizeof(sa->vxrs_high)); - size += nt_size(NT_S390_VXRS_LOW, sizeof(sa->vxrs_low)); + size += NT_SIZE(S390_VXRS_HIGH, sa->vxrs_high); + size += NT_SIZE(S390_VXRS_LOW, sa->vxrs_low); } return size; @@ -373,7 +357,7 @@ static void *nt_prpsinfo(void *ptr) memset(&prpsinfo, 0, sizeof(prpsinfo)); prpsinfo.pr_sname = 'R'; strcpy(prpsinfo.pr_fname, "vmlinux"); - return nt_init(ptr, NT_PRPSINFO, &prpsinfo, sizeof(prpsinfo)); + return NT_INIT(ptr, PRPSINFO, prpsinfo); } /* @@ -589,7 +573,7 @@ static size_t get_elfcorehdr_size(int phdr_count) /* PT_NOTES */ size += sizeof(Elf64_Phdr); /* nt_prpsinfo */ - size += nt_size(NT_PRPSINFO, sizeof(struct elf_prpsinfo)); + size += NT_SIZE(PRPSINFO, struct elf_prpsinfo); /* regsets */ size += get_cpu_cnt() * get_cpu_elf_notes_size(); /* nt_vmcoreinfo */
Use note name macros to match with the userspace's expectation. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- arch/s390/kernel/crash_dump.c | 62 ++++++++++++++++--------------------------- 1 file changed, 23 insertions(+), 39 deletions(-)