Mailing List Archive: [PATCH v6 3/4] libelf: Store maximum PHDR p

[PATCH v6 3/4] libelf: Store maximum PHDR p_align

Mar 27, 2024, 2:51 PM

Post #1 of 5 (22 views)

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(-)

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;
--
2.44.0

Re: [PATCH v6 3/4] libelf: Store maximum PHDR p_align [ In reply to ]

jbeulich at suse

Mar 28, 2024, 9:47 AM

Post #2 of 5 (22 views)

Permalink

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

Re: [PATCH v6 3/4] libelf: Store maximum PHDR p_align [ In reply to ]

jason.andryuk at amd

Mar 29, 2024, 7:41 AM

Post #3 of 5 (22 views)

Permalink

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

Re: [PATCH v6 3/4] libelf: Store maximum PHDR p_align [ In reply to ]

jbeulich at suse

Apr 1, 2024, 11:44 PM

Post #4 of 5 (9 views)

Permalink

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

Re: [PATCH v6 3/4] libelf: Store maximum PHDR p_align [ In reply to ]

jason.andryuk at amd

Apr 4, 2024, 7:00 AM

Post #5 of 5 (6 views)

Permalink

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