Re: [PATCH v5 1/6] Revert "xen/x86: bzImage parse kernel_alignment"

[Jason Andryuk <jason.andryuk@amd.com>]

On 2024-03-27 10:19, Jan Beulich wrote:
> On 27.03.2024 15:08, Jason Andryuk wrote:
>> On 2024-03-27 04:59, Roger Pau Monné wrote:
>>> On Wed, Mar 27, 2024 at 08:22:41AM +0100, Jan Beulich wrote:
>>>> On 26.03.2024 22:38, Jason Andryuk wrote:
>>>>> A new ELF note will specify the alignment for a relocatable PVH kernel.
>>>>> ELF notes are suitable for vmlinux and other ELF files, so this
>>>>> Linux-specific bzImage parsing in unnecessary.
>>>>>
>>>>> This reverts commit c44cac229067faeec8f49247d1cf281723ac2d40.
>>>>>
>>>>> Signed-off-by: Jason Andryuk <jason.andryuk@amd.com>
>>>>
>>>> Since you keep re-sending this: In private discussion Roger has indicated
>>>> that, like me, he too would prefer falling back to the ELF data, before
>>>> falling back to the arch default (Roger, please correct me if I got it
>>>> wrong). That would make it necessary that the change you're proposing to
>>>> revert here is actually kept.
>>>
>>> Sorry, was meaning to reply yesterday but Jason is very fast at
>>> sending new version so I'm always one version behind.
>>
>> :)
>>
>> I was hoping to finish this up and get it in...
>>
>>> IMO the order: ELF note, PHDR alignment, arch default should be the
>>> preferred one.
>>>
>>>> Or wait - what you're reverting is taking the alignment out of the
>>>> bzImage header. I don't expect the BSDs to use that protocol; aiui that's
>>>> entirely Linux-specific.
>>>
>>> Yeah, I don't have strong opinions in keeping this, we already do
>>> bzImage parsing, so we might as well attempt to fetch the alignment
>>> from there if correct:
>>>
>>> ELF note, bzImage kernel_alignment, ELF PHDR alignment, arch default
>>
>> I'm not sure how to handle ELF PHDR vs. arch default.  ELF PHDR will
>> always be set, AFAIU.  Should that always be respected, which means we
>> don't need an arch default?
> 
> A value of 0 (and 1) is specifically permitted, to indicate no alignment.
> We may take 0 to mean default, but what you suggest below is another
> plausible approach. Yet another might be to take anything below PAGE_SIZE
> as "use default".
> 
>> To include arch default, it would be something like this:
>>
>>       if ( parms->phys_align != UNSET_ADDR )
>>           align = parms->phys_align;
>>       else if ( bz_align )
>>           align = bz_align;
> 
> Why do you include bz again here? Didn't you previously indicate the
> header field can't be relied upon? Which is also why, finally, I committed
> this revert earlier today.

Roger wanted to consult the bz value?  He mentioned it above.  Locally, 
I haven't synced with staging yet, and I dropped the revert to start 
re-working this...

If present, the bzImage header field is valid.  But being 
bzImage-specific, it isn't useful for other ELF files.  Xen will only 
move a kernel with the PHSY32_RELOC Note, so it can just specify an 
alignment if it needs to.

Personally, I think using the Note's value or a default is fine.  It 
seems like the PHDR aligment will just be 0x200000 anyway (for x86-64 at 
lease), which matches the default.  Specifying the PHYS32_RELOC Note, 
but relying on a search for the alignment, seems slightly questionable 
to me.

Still, it seemed like the path of least resistance is to just implement 
the fallback search like Roger requested.  Dropping the bzImage, I guess 
I'd go with your PAGE_SIZE suggestions for:

     if ( parms->phys_align != UNSET_ADDR )
         align = parms->phys_align;
     else if ( elf->palign > PAGE_SIZE )
         align = elf->palign;
     else
         align = PHYS32_RELOC_ALIGN_DEFAULT;

Thanks for your reviews.

Regards,
Jason