| Message ID | 20240327215102.136001-4-jason.andryuk@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/pvh: Support relocating dom0 kernel | expand |
On 27.03.2024 22:51, Jason Andryuk wrote: > --- a/xen/common/libelf/libelf-loader.c > +++ b/xen/common/libelf/libelf-loader.c > @@ -468,6 +468,7 @@ void elf_parse_binary(struct elf_binary *elf) > { > ELF_HANDLE_DECL(elf_phdr) phdr; > uint64_t low = -1, high = 0, paddr, memsz; > + uint64_t max_align = 0, palign; > unsigned i, count; > > count = elf_phdr_count(elf); > @@ -481,17 +482,23 @@ void elf_parse_binary(struct elf_binary *elf) > continue; > paddr = elf_uval(elf, phdr, p_paddr); > memsz = elf_uval(elf, phdr, p_memsz); > - elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n", > - paddr, memsz); > + palign = elf_uval(elf, phdr, p_align); > + elf_msg(elf, > + "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 " palign=%#" PRIx64 "\n", > + paddr, memsz, palign); > if ( low > paddr ) > low = paddr; > if ( high < paddr + memsz ) > high = paddr + memsz; > + if ( max_align < palign ) > + max_align = palign; > } > elf->pstart = low; > elf->pend = high; > - elf_msg(elf, "ELF: memory: %#" PRIx64 " -> %#" PRIx64 "\n", > - elf->pstart, elf->pend); > + elf->palign = max_align; > + elf_msg(elf, > + "ELF: memory: %#" PRIx64 " -> %#" PRIx64 " align:%#" PRIx64 "\n", > + elf->pstart, elf->pend, elf->palign); > } Hmm, it's just this final logging change which I'm a little concerned by: Having looked at Linux'es phdr, I noticed that the addresses there aren't necessarily matching the corresponding alignment. Therefore I'm a little concerned that the output here might raise questions when people see seemingly inconsistent values in the log. Could you/we at least make it read like e.g. "align (max): ..."? Jan
On 2024-03-28 12:47, Jan Beulich wrote: > On 27.03.2024 22:51, Jason Andryuk wrote: >> --- a/xen/common/libelf/libelf-loader.c >> +++ b/xen/common/libelf/libelf-loader.c >> @@ -468,6 +468,7 @@ void elf_parse_binary(struct elf_binary *elf) >> { >> ELF_HANDLE_DECL(elf_phdr) phdr; >> uint64_t low = -1, high = 0, paddr, memsz; >> + uint64_t max_align = 0, palign; >> unsigned i, count; >> >> count = elf_phdr_count(elf); >> @@ -481,17 +482,23 @@ void elf_parse_binary(struct elf_binary *elf) >> continue; >> paddr = elf_uval(elf, phdr, p_paddr); >> memsz = elf_uval(elf, phdr, p_memsz); >> - elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n", >> - paddr, memsz); >> + palign = elf_uval(elf, phdr, p_align); >> + elf_msg(elf, >> + "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 " palign=%#" PRIx64 "\n", >> + paddr, memsz, palign); >> if ( low > paddr ) >> low = paddr; >> if ( high < paddr + memsz ) >> high = paddr + memsz; >> + if ( max_align < palign ) >> + max_align = palign; >> } >> elf->pstart = low; >> elf->pend = high; >> - elf_msg(elf, "ELF: memory: %#" PRIx64 " -> %#" PRIx64 "\n", >> - elf->pstart, elf->pend); >> + elf->palign = max_align; >> + elf_msg(elf, >> + "ELF: memory: %#" PRIx64 " -> %#" PRIx64 " align:%#" PRIx64 "\n", >> + elf->pstart, elf->pend, elf->palign); >> } > > Hmm, it's just this final logging change which I'm a little concerned by: > Having looked at Linux'es phdr, I noticed that the addresses there aren't > necessarily matching the corresponding alignment. Therefore I'm a little > concerned that the output here might raise questions when people see > seemingly inconsistent values in the log. Could you/we at least make it > read like e.g. "align (max): ..."? max_align? Looking at my test vmlinux, it looks like PHDR 0 (.text) and PHDR 1 (.data) are aligned. It's the PHDR 2 (.init) that isn't aligned. Is that what you see? This line is already printing the min and max across all the PHDRs, so it would only look confusing if the start didn't match the align. I'm not sure how useful it is to print the alignment, and I considered not printing it. It's only used with PVH Dom0 right now, so it's not relevant in most cases. Regards, Jason
On 29.03.2024 15:41, Jason Andryuk wrote: > On 2024-03-28 12:47, Jan Beulich wrote: >> On 27.03.2024 22:51, Jason Andryuk wrote: >>> --- a/xen/common/libelf/libelf-loader.c >>> +++ b/xen/common/libelf/libelf-loader.c >>> @@ -468,6 +468,7 @@ void elf_parse_binary(struct elf_binary *elf) >>> { >>> ELF_HANDLE_DECL(elf_phdr) phdr; >>> uint64_t low = -1, high = 0, paddr, memsz; >>> + uint64_t max_align = 0, palign; >>> unsigned i, count; >>> >>> count = elf_phdr_count(elf); >>> @@ -481,17 +482,23 @@ void elf_parse_binary(struct elf_binary *elf) >>> continue; >>> paddr = elf_uval(elf, phdr, p_paddr); >>> memsz = elf_uval(elf, phdr, p_memsz); >>> - elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n", >>> - paddr, memsz); >>> + palign = elf_uval(elf, phdr, p_align); >>> + elf_msg(elf, >>> + "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 " palign=%#" PRIx64 "\n", >>> + paddr, memsz, palign); >>> if ( low > paddr ) >>> low = paddr; >>> if ( high < paddr + memsz ) >>> high = paddr + memsz; >>> + if ( max_align < palign ) >>> + max_align = palign; >>> } >>> elf->pstart = low; >>> elf->pend = high; >>> - elf_msg(elf, "ELF: memory: %#" PRIx64 " -> %#" PRIx64 "\n", >>> - elf->pstart, elf->pend); >>> + elf->palign = max_align; >>> + elf_msg(elf, >>> + "ELF: memory: %#" PRIx64 " -> %#" PRIx64 " align:%#" PRIx64 "\n", >>> + elf->pstart, elf->pend, elf->palign); >>> } >> >> Hmm, it's just this final logging change which I'm a little concerned by: >> Having looked at Linux'es phdr, I noticed that the addresses there aren't >> necessarily matching the corresponding alignment. Therefore I'm a little >> concerned that the output here might raise questions when people see >> seemingly inconsistent values in the log. Could you/we at least make it >> read like e.g. "align (max): ..."? > > max_align? > > Looking at my test vmlinux, it looks like PHDR 0 (.text) and PHDR 1 > (.data) are aligned. It's the PHDR 2 (.init) that isn't aligned. Is > that what you see? > > This line is already printing the min and max across all the PHDRs, so > it would only look confusing if the start didn't match the align. > > I'm not sure how useful it is to print the alignment, and I considered > not printing it. It's only used with PVH Dom0 right now, so it's not > relevant in most cases. Well, yes, not printing the alignment would be fine with me. I just didn't suggest that as an alternative since I was assuming you put its printing there for a reason. Jan
On 2024-04-02 02:44, Jan Beulich wrote: > On 29.03.2024 15:41, Jason Andryuk wrote: >> On 2024-03-28 12:47, Jan Beulich wrote: >>> On 27.03.2024 22:51, Jason Andryuk wrote: >>>> --- a/xen/common/libelf/libelf-loader.c >>>> +++ b/xen/common/libelf/libelf-loader.c >>>> @@ -468,6 +468,7 @@ void elf_parse_binary(struct elf_binary *elf) >>>> { >>>> ELF_HANDLE_DECL(elf_phdr) phdr; >>>> uint64_t low = -1, high = 0, paddr, memsz; >>>> + uint64_t max_align = 0, palign; >>>> unsigned i, count; >>>> >>>> count = elf_phdr_count(elf); >>>> @@ -481,17 +482,23 @@ void elf_parse_binary(struct elf_binary *elf) >>>> continue; >>>> paddr = elf_uval(elf, phdr, p_paddr); >>>> memsz = elf_uval(elf, phdr, p_memsz); >>>> - elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n", >>>> - paddr, memsz); >>>> + palign = elf_uval(elf, phdr, p_align); >>>> + elf_msg(elf, >>>> + "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 " palign=%#" PRIx64 "\n", >>>> + paddr, memsz, palign); >>>> if ( low > paddr ) >>>> low = paddr; >>>> if ( high < paddr + memsz ) >>>> high = paddr + memsz; >>>> + if ( max_align < palign ) >>>> + max_align = palign; >>>> } >>>> elf->pstart = low; >>>> elf->pend = high; >>>> - elf_msg(elf, "ELF: memory: %#" PRIx64 " -> %#" PRIx64 "\n", >>>> - elf->pstart, elf->pend); >>>> + elf->palign = max_align; >>>> + elf_msg(elf, >>>> + "ELF: memory: %#" PRIx64 " -> %#" PRIx64 " align:%#" PRIx64 "\n", >>>> + elf->pstart, elf->pend, elf->palign); >>>> } >>> >>> Hmm, it's just this final logging change which I'm a little concerned by: >>> Having looked at Linux'es phdr, I noticed that the addresses there aren't >>> necessarily matching the corresponding alignment. Therefore I'm a little >>> concerned that the output here might raise questions when people see >>> seemingly inconsistent values in the log. Could you/we at least make it >>> read like e.g. "align (max): ..."? >> >> max_align? >> >> Looking at my test vmlinux, it looks like PHDR 0 (.text) and PHDR 1 >> (.data) are aligned. It's the PHDR 2 (.init) that isn't aligned. Is >> that what you see? >> >> This line is already printing the min and max across all the PHDRs, so >> it would only look confusing if the start didn't match the align. >> >> I'm not sure how useful it is to print the alignment, and I considered >> not printing it. It's only used with PVH Dom0 right now, so it's not >> relevant in most cases. > > Well, yes, not printing the alignment would be fine with me. I just didn't > suggest that as an alternative since I was assuming you put its printing > there for a reason. I added it to make the information available, which might be useful if there was a problem finding a load location. Since it's usually not useful information, I'll just drop it. Thanks, Jason
diff --git a/xen/common/libelf/libelf-loader.c b/xen/common/libelf/libelf-loader.c index 629cc0d3e6..a5f6389f82 100644 --- a/xen/common/libelf/libelf-loader.c +++ b/xen/common/libelf/libelf-loader.c @@ -468,6 +468,7 @@ void elf_parse_binary(struct elf_binary *elf) { ELF_HANDLE_DECL(elf_phdr) phdr; uint64_t low = -1, high = 0, paddr, memsz; + uint64_t max_align = 0, palign; unsigned i, count; count = elf_phdr_count(elf); @@ -481,17 +482,23 @@ void elf_parse_binary(struct elf_binary *elf) continue; paddr = elf_uval(elf, phdr, p_paddr); memsz = elf_uval(elf, phdr, p_memsz); - elf_msg(elf, "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 "\n", - paddr, memsz); + palign = elf_uval(elf, phdr, p_align); + elf_msg(elf, + "ELF: phdr: paddr=%#" PRIx64 " memsz=%#" PRIx64 " palign=%#" PRIx64 "\n", + paddr, memsz, palign); if ( low > paddr ) low = paddr; if ( high < paddr + memsz ) high = paddr + memsz; + if ( max_align < palign ) + max_align = palign; } elf->pstart = low; elf->pend = high; - elf_msg(elf, "ELF: memory: %#" PRIx64 " -> %#" PRIx64 "\n", - elf->pstart, elf->pend); + elf->palign = max_align; + elf_msg(elf, + "ELF: memory: %#" PRIx64 " -> %#" PRIx64 " align:%#" PRIx64 "\n", + elf->pstart, elf->pend, elf->palign); } elf_errorstatus elf_load_binary(struct elf_binary *elf) diff --git a/xen/include/xen/libelf.h b/xen/include/xen/libelf.h index 1c77e3df31..2d971f958e 100644 --- a/xen/include/xen/libelf.h +++ b/xen/include/xen/libelf.h @@ -196,6 +196,7 @@ struct elf_binary { size_t dest_size; uint64_t pstart; uint64_t pend; + uint64_t palign; uint64_t reloc_offset; uint64_t bsd_symtab_pstart;
While parsing the PHDRs, store the maximum p_align value. This may be consulted for moving a PVH image's load address. Signed-off-by: Jason Andryuk <jason.andryuk@amd.com> --- v6: New --- xen/common/libelf/libelf-loader.c | 15 +++++++++++---- xen/include/xen/libelf.h | 1 + 2 files changed, 12 insertions(+), 4 deletions(-)