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

[Jason Andryuk <jason.andryuk@xxxxxxx>]

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@xxxxxxx>
> > 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?
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;
    else if ( elf->palign > PHYS32_RELOC_ALIGN_DEFAULT )
        align = elf->palign;
    else
        align = PHYS32_RELOC_ALIGN_DEFAULT;


> > I further meanwhile realized that consulting the ELF phdrs may also be
> > ambiguous, as there may be more than one. I guess it would need to be the
> > maximum of all of them then.
> My suggestion (not sure if I mentioned this before) was to use the
> alignment of the first LOAD PHDR, which is the one that defines the
> value of the dest_base field used as the image load start address.
>
> Using the maximum of all load PHDRs might be safer.
I'll find the maximum.

Thanks,
Jason