| Message ID | 1475103092-27893-5-git-send-email-daniel.kiper@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.09.16 at 00:51, <daniel.kiper@oracle.com> wrote: > Currently xen ELF end of image address is calculated using first line from > "nm -nr xen/xen-syms" output. However, potentially it may contain symbol > address not related to the end of image in any way. It can happen if a symbol > is introduced with address larger than _end symbol address. Such situation > encountered when I linked xen ELF binary with xen/arch/x86/efi/relocs-dummy.S. > Then first line from "nm -nr xen/xen-syms" contained "ffff82d0c0000000 A ALT_START" > and xen ELF image memory size was silently set to 1023 MiB. This issue happened > because there is no check which symbol address is used to calculate end of > image address. So, let's fix it and take ELF end of image address by reading > _end symbol address from nm output. This way xen ELF image build process is > not prone to changes in order of nm output. > > This patch is prereq for "efi: build xen.gz with EFI code" patch which adds, > among others, xen/arch/x86/efi/relocs-dummy.S to xen.gz output. I still dislike how you put this. It would make a lot more sense if this extra paragraph came much earlier (perhaps right after the first sentence), and the rest of the description would then be slightly re-worded to fit this aspect. > --- a/xen/arch/x86/Makefile > +++ b/xen/arch/x86/Makefile > @@ -91,7 +91,7 @@ endif > > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \ > - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` > + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . _end$$/0x\1/p'` I thought we had agreed to use __2M_rwdata_end here. And also note that I had said "such that the command line argument here aligns with the first one on the previous line" in my reply to v7 regarding the indentation change. Jan
On Thu, Sep 29, 2016 at 01:34:30AM -0600, Jan Beulich wrote: > >>> On 29.09.16 at 00:51, <daniel.kiper@oracle.com> wrote: > > Currently xen ELF end of image address is calculated using first line from > > "nm -nr xen/xen-syms" output. However, potentially it may contain symbol > > address not related to the end of image in any way. It can happen if a symbol > > is introduced with address larger than _end symbol address. Such situation > > encountered when I linked xen ELF binary with xen/arch/x86/efi/relocs-dummy.S. > > Then first line from "nm -nr xen/xen-syms" contained "ffff82d0c0000000 A ALT_START" > > and xen ELF image memory size was silently set to 1023 MiB. This issue happened > > because there is no check which symbol address is used to calculate end of > > image address. So, let's fix it and take ELF end of image address by reading > > _end symbol address from nm output. This way xen ELF image build process is > > not prone to changes in order of nm output. > > > > This patch is prereq for "efi: build xen.gz with EFI code" patch which adds, > > among others, xen/arch/x86/efi/relocs-dummy.S to xen.gz output. > > I still dislike how you put this. It would make a lot more sense if this > extra paragraph came much earlier (perhaps right after the first > sentence), and the rest of the description would then be slightly > re-worded to fit this aspect. OK. > > --- a/xen/arch/x86/Makefile > > +++ b/xen/arch/x86/Makefile > > @@ -91,7 +91,7 @@ endif > > > > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \ > > - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` > > + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . _end$$/0x\1/p'` > > I thought we had agreed to use __2M_rwdata_end here. Oh, I forgot about that. > And also note that I had said "such that the command line argument > here aligns with the first one on the previous line" in my reply to v7 > regarding the indentation change. $(TARGET)-syms starts in the same column as $(notes_phdrs) does in the final output as you requested. However, here it looks that it is misaligned due to tab at the begging of the line. Daniel
>>> On 29.09.16 at 09:51, <daniel.kiper@oracle.com> wrote: > On Thu, Sep 29, 2016 at 01:34:30AM -0600, Jan Beulich wrote: >> >>> On 29.09.16 at 00:51, <daniel.kiper@oracle.com> wrote: >> > --- a/xen/arch/x86/Makefile >> > +++ b/xen/arch/x86/Makefile >> > @@ -91,7 +91,7 @@ endif >> > >> > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 >> > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \ >> > - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` >> > + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . _end$$/0x\1/p'` >> >> I thought we had agreed to use __2M_rwdata_end here. > > Oh, I forgot about that. > >> And also note that I had said "such that the command line argument >> here aligns with the first one on the previous line" in my reply to v7 >> regarding the indentation change. > > $(TARGET)-syms starts in the same column as $(notes_phdrs) does in the > final output as you requested. However, here it looks that it is > misaligned due to tab at the begging of the line. I specifically looked at this in an editor using a fixed width font, and I just took a look another time: I cannot confirm what you say - the first back quote sits under the k of mkelf32. Or wait - what you say is true, but is not what I had asked for. The whole back-quoted string is an argument to mkelf32. Jan
On Thu, Sep 29, 2016 at 02:01:46AM -0600, Jan Beulich wrote: > >>> On 29.09.16 at 09:51, <daniel.kiper@oracle.com> wrote: > > On Thu, Sep 29, 2016 at 01:34:30AM -0600, Jan Beulich wrote: > >> >>> On 29.09.16 at 00:51, <daniel.kiper@oracle.com> wrote: > >> > --- a/xen/arch/x86/Makefile > >> > +++ b/xen/arch/x86/Makefile > >> > @@ -91,7 +91,7 @@ endif > >> > > >> > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 > >> > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \ > >> > - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` > >> > + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . _end$$/0x\1/p'` > >> > >> I thought we had agreed to use __2M_rwdata_end here. > > > > Oh, I forgot about that. > > > >> And also note that I had said "such that the command line argument > >> here aligns with the first one on the previous line" in my reply to v7 > >> regarding the indentation change. > > > > $(TARGET)-syms starts in the same column as $(notes_phdrs) does in the > > final output as you requested. However, here it looks that it is > > misaligned due to tab at the begging of the line. > > I specifically looked at this in an editor using a fixed width font, > and I just took a look another time: I cannot confirm what you > say - the first back quote sits under the k of mkelf32. Or wait - > what you say is true, but is not what I had asked for. The whole > back-quoted string is an argument to mkelf32. Ahhh... It looks that I misunderstood you. You wish to have first back-quote under "$" sign which is the beginning of "$(notes_phdrs)". Am I right? Daniel
>>> On 29.09.16 at 10:17, <daniel.kiper@oracle.com> wrote: > On Thu, Sep 29, 2016 at 02:01:46AM -0600, Jan Beulich wrote: >> >>> On 29.09.16 at 09:51, <daniel.kiper@oracle.com> wrote: >> > On Thu, Sep 29, 2016 at 01:34:30AM -0600, Jan Beulich wrote: >> >> >>> On 29.09.16 at 00:51, <daniel.kiper@oracle.com> wrote: >> >> > --- a/xen/arch/x86/Makefile >> >> > +++ b/xen/arch/x86/Makefile >> >> > @@ -91,7 +91,7 @@ endif >> >> > >> >> > $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 >> >> > ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \ >> >> > - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` >> >> > + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . _end$$/0x\1/p'` >> >> >> >> I thought we had agreed to use __2M_rwdata_end here. >> > >> > Oh, I forgot about that. >> > >> >> And also note that I had said "such that the command line argument >> >> here aligns with the first one on the previous line" in my reply to v7 >> >> regarding the indentation change. >> > >> > $(TARGET)-syms starts in the same column as $(notes_phdrs) does in the >> > final output as you requested. However, here it looks that it is >> > misaligned due to tab at the begging of the line. >> >> I specifically looked at this in an editor using a fixed width font, >> and I just took a look another time: I cannot confirm what you >> say - the first back quote sits under the k of mkelf32. Or wait - >> what you say is true, but is not what I had asked for. The whole >> back-quoted string is an argument to mkelf32. > > Ahhh... It looks that I misunderstood you. You wish to have first > back-quote under "$" sign which is the beginning of "$(notes_phdrs)". > Am I right? Yes. Jan
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 931917d..8e003ff 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -91,7 +91,7 @@ endif $(TARGET): $(TARGET)-syms $(efi-y) boot/mkelf32 ./boot/mkelf32 $(notes_phdrs) $(TARGET)-syms $(TARGET) 0x100000 \ - `$(NM) -nr $(TARGET)-syms | head -n 1 | sed -e 's/^\([^ ]*\).*/0x\1/'` + `$(NM) $(TARGET)-syms | sed -ne 's/^\([^ ]*\) . _end$$/0x\1/p'` ALL_OBJS := $(BASEDIR)/arch/x86/boot/built_in.o $(BASEDIR)/arch/x86/efi/built_in.o $(ALL_OBJS)
Currently xen ELF end of image address is calculated using first line from "nm -nr xen/xen-syms" output. However, potentially it may contain symbol address not related to the end of image in any way. It can happen if a symbol is introduced with address larger than _end symbol address. Such situation encountered when I linked xen ELF binary with xen/arch/x86/efi/relocs-dummy.S. Then first line from "nm -nr xen/xen-syms" contained "ffff82d0c0000000 A ALT_START" and xen ELF image memory size was silently set to 1023 MiB. This issue happened because there is no check which symbol address is used to calculate end of image address. So, let's fix it and take ELF end of image address by reading _end symbol address from nm output. This way xen ELF image build process is not prone to changes in order of nm output. This patch is prereq for "efi: build xen.gz with EFI code" patch which adds, among others, xen/arch/x86/efi/relocs-dummy.S to xen.gz output. Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com> --- v8 - suggestions/fixes: - use spaces instead of tab in indentation (suggested by Jan Beulich and Konrad Rzeszutek Wilk), - improve commit message (suggested by Jan Beulich). v7 - suggestions/fixes: - use sed instead of awk (suggested by Jan Beulich), - improve commit message (suggested by Jan Beulich). --- xen/arch/x86/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)