Re: [PATCH v9 01/19] x86/boot: Place kernel_info at a fixed offset

[ross . philipson]

On 6/4/24 11:18 AM, Jarkko Sakkinen wrote:
> On Fri May 31, 2024 at 4:03 AM EEST, Ross Philipson wrote:
> > From: Arvind Sankar <nivedita@xxxxxxxxxxxx>
> >
> > There are use cases for storing the offset of a symbol in kernel_info.
> > For example, the trenchboot series [0] needs to store the offset of the
> > Measured Launch Environment header in kernel_info.
>
> So either there are other use cases that you should enumerate, or just
> be straight and state that this is done for Trenchboot.

The kernel_info concept came about because of the work we were doing on 
TrenchBoot but it was not done for TrenchBoot. It was a collaborative 
effort between the TrenchBoot team and H. Peter Anvin at Intel. He 
actually envisioned it being useful elsewhere. If you find the original 
commits for it (that went in stand-alone) from Daniel Kiper, there is a 
fair amount of detail what kernel_info is supposed to be and should be 
used for.

> I believe latter is the case, and there is no reason to project further.
> If it does not interfere kernel otherwise, it should be fine just by
> that.
>
> Also I believe that it is written as Trenchboot, without "series" ;-)
> Think when writing commit message that it will some day be part of the
> commit log, not a series flying in the air.
>
> Sorry for the nitpicks but better to be punctual and that way also
> transparent as possible, right?

No problem. We submit the patch sets to get feedback :)

Thanks for the feedback.

> > Since commit (note: commit ID from tip/master)
> >
> > commit 527afc212231 ("x86/boot: Check that there are no run-time relocations")
> >
> > run-time relocations are not allowed in the compressed kernel, so simply
> > using the symbol in kernel_info, as
> >
> > 	.long	symbol
> >
> > will cause a linker error because this is not position-independent.
> >
> > With kernel_info being a separate object file and in a different section
> > from startup_32, there is no way to calculate the offset of a symbol
> > from the start of the image in a position-independent way.
> >
> > To enable such use cases, put kernel_info into its own section which is
>
> "To allow Trenchboot to access the fields of kernel_info..."
>
> Much more understandable.
>
> > placed at a predetermined offset (KERNEL_INFO_OFFSET) via the linker
> > script. This will allow calculating the symbol offset in a
> > position-independent way, by adding the offset from the start of
> > kernel_info to KERNEL_INFO_OFFSET.
> >
> > Ensure that kernel_info is aligned, and use the SYM_DATA.* macros
> > instead of bare labels. This stores the size of the kernel_info
> > structure in the ELF symbol table.
>
> Aligned to which boundary and short explanation why to that boundary,
> i.e. state the obvious if you bring it up anyway here.
>
> Just seems to be progressing pretty well so taking my eye glass and
> looking into nitty gritty details...

So a lot of this is up in the air if you read the responses between us 
and Ard Biesheuvel. It would be nice to get rid of the part where 
kernel_info is forced to a fixed offset in the setup kernel.

Thanks
Ross

> BR, Jarkko