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

[Jan Beulich <jbeulich@xxxxxxxx>]

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