[PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[Andy Shevchenko]

The init_size description of boot protocol has an example of the runtime
start address for the compressed bzImage. For non-relocatable kernel
it relies on the pref_address value (if not 0), but for relocatable case
only pays respect to the load_addres and kernel_alignment, and it is
inaccurate for the latter. Boot loader must consider the pref_address
as the Linux kernel relocates to it before being decompressed as nicely
described in the commit 43b1d3e68ee7 message.

Due to this inaccuracy some of the bootloaders (*) made a mistake in
the calculations and if kernel image is big enough, this may lead to
unbootable configurations.

*)
  In particular, kexec-tools missed that and resently got a couple of
  changes which will be part of v2.0.30 release. For the record,
  the 43b1d3e68ee7 fixed only kernel kexec implementation and also missed
  to update the init_size description.

While at it, make an example C-like looking as it's done elsewhere in
the document and fix indentation, so the syntax highliting will work
properly in some editors (vim).

Fixes: 43b1d3e68ee7 ("kexec: Allocate kernel above bzImage's pref_address")
Fixes: d297366ba692 ("x86: document new bzImage fields")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 Documentation/arch/x86/boot.rst | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/Documentation/arch/x86/boot.rst b/Documentation/arch/x86/boot.rst
index 4fd492cb4970..01f08d94e8df 100644
--- a/Documentation/arch/x86/boot.rst
+++ b/Documentation/arch/x86/boot.rst
@@ -896,10 +896,19 @@ Offset/size:	0x260/4
 
   The kernel runtime start address is determined by the following algorithm::
 
-	if (relocatable_kernel)
-	runtime_start = align_up(load_address, kernel_alignment)
-	else
-	runtime_start = pref_address
+    if ( relocatable_kernel ) {
+      if ( load_address < pref_address )
+        load_address = pref_address;
+      runtime_start = align_up(load_address, kernel_alignment);
+    } else {
+      runtime_start = pref_address;
+    }
+
+Hence the necessary memory window location and size can be estimated by
+a boot loader as::
+
+    memory_window_start = runtime_start;
+    memory_window_size = init_size;
 
 ============	===============
 Field name:	handover_offset
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[Randy Dunlap]

Hi Andy,

On 11/25/24 12:31 AM, Andy Shevchenko wrote:
> The init_size description of boot protocol has an example of the runtime
> start address for the compressed bzImage. For non-relocatable kernel
> it relies on the pref_address value (if not 0), but for relocatable case
> only pays respect to the load_addres and kernel_alignment, and it is
> inaccurate for the latter. Boot loader must consider the pref_address
> as the Linux kernel relocates to it before being decompressed as nicely
> described in the commit 43b1d3e68ee7 message.
> 
> Due to this inaccuracy some of the bootloaders (*) made a mistake in
> the calculations and if kernel image is big enough, this may lead to
> unbootable configurations.
> 
> *)
>   In particular, kexec-tools missed that and resently got a couple of
>   changes which will be part of v2.0.30 release. For the record,
>   the 43b1d3e68ee7 fixed only kernel kexec implementation and also missed
>   to update the init_size description.
> 
> While at it, make an example C-like looking as it's done elsewhere in
> the document and fix indentation, so the syntax highliting will work
> properly in some editors (vim).
> 
> Fixes: 43b1d3e68ee7 ("kexec: Allocate kernel above bzImage's pref_address")
> Fixes: d297366ba692 ("x86: document new bzImage fields")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  Documentation/arch/x86/boot.rst | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/arch/x86/boot.rst b/Documentation/arch/x86/boot.rst
> index 4fd492cb4970..01f08d94e8df 100644
> --- a/Documentation/arch/x86/boot.rst
> +++ b/Documentation/arch/x86/boot.rst
> @@ -896,10 +896,19 @@ Offset/size:	0x260/4
>  
>    The kernel runtime start address is determined by the following algorithm::
>  
> -	if (relocatable_kernel)
> -	runtime_start = align_up(load_address, kernel_alignment)
> -	else
> -	runtime_start = pref_address
> +    if ( relocatable_kernel ) {
> +      if ( load_address < pref_address )

What's up with the extra spaces around ( and ) ... and inconsistent with
the lines below?

> +        load_address = pref_address;
> +      runtime_start = align_up(load_address, kernel_alignment);
> +    } else {
> +      runtime_start = pref_address;
> +    }
> +
> +Hence the necessary memory window location and size can be estimated by
> +a boot loader as::
> +
> +    memory_window_start = runtime_start;
> +    memory_window_size = init_size;
>  
>  ============	===============
>  Field name:	handover_offset

-- 
~Randy

Re: [PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[Ingo Molnar]

* Randy Dunlap <rdunlap@infradead.org> wrote:

> Hi Andy,
> 
> On 11/25/24 12:31 AM, Andy Shevchenko wrote:
> > The init_size description of boot protocol has an example of the runtime
> > start address for the compressed bzImage. For non-relocatable kernel
> > it relies on the pref_address value (if not 0), but for relocatable case
> > only pays respect to the load_addres and kernel_alignment, and it is
> > inaccurate for the latter. Boot loader must consider the pref_address
> > as the Linux kernel relocates to it before being decompressed as nicely
> > described in the commit 43b1d3e68ee7 message.
> > 
> > Due to this inaccuracy some of the bootloaders (*) made a mistake in
> > the calculations and if kernel image is big enough, this may lead to
> > unbootable configurations.
> > 
> > *)
> >   In particular, kexec-tools missed that and resently got a couple of
> >   changes which will be part of v2.0.30 release. For the record,
> >   the 43b1d3e68ee7 fixed only kernel kexec implementation and also missed
> >   to update the init_size description.
> > 
> > While at it, make an example C-like looking as it's done elsewhere in
> > the document and fix indentation, so the syntax highliting will work
> > properly in some editors (vim).
> > 
> > Fixes: 43b1d3e68ee7 ("kexec: Allocate kernel above bzImage's pref_address")
> > Fixes: d297366ba692 ("x86: document new bzImage fields")
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  Documentation/arch/x86/boot.rst | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/arch/x86/boot.rst b/Documentation/arch/x86/boot.rst
> > index 4fd492cb4970..01f08d94e8df 100644
> > --- a/Documentation/arch/x86/boot.rst
> > +++ b/Documentation/arch/x86/boot.rst
> > @@ -896,10 +896,19 @@ Offset/size:	0x260/4
> >  
> >    The kernel runtime start address is determined by the following algorithm::
> >  
> > -	if (relocatable_kernel)
> > -	runtime_start = align_up(load_address, kernel_alignment)
> > -	else
> > -	runtime_start = pref_address
> > +    if ( relocatable_kernel ) {
> > +      if ( load_address < pref_address )
> 
> What's up with the extra spaces around ( and ) ... and inconsistent with
> the lines below?

Also, even pseudocode should follow the kernel's coding style and use 
tabs in particular - which it already does in (some...) other places of 
this document, such as the 'Sample Boot Configuration' chapter.

Thanks,

	Ingo

Re: [PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[Andy Shevchenko]

On Mon, Nov 25, 2024 at 09:45:36AM +0100, Ingo Molnar wrote:
> * Randy Dunlap <rdunlap@infradead.org> wrote:
> > On 11/25/24 12:31 AM, Andy Shevchenko wrote:

...

> > > -	if (relocatable_kernel)
> > > -	runtime_start = align_up(load_address, kernel_alignment)
> > > -	else
> > > -	runtime_start = pref_address
> > > +    if ( relocatable_kernel ) {
> > > +      if ( load_address < pref_address )
> > 
> > What's up with the extra spaces around ( and ) ... and inconsistent with
> > the lines below?

I can remove them. This file has a lot of inconsistencies it seems...

> Also, even pseudocode should follow the kernel's coding style and use 
> tabs in particular - which it already does in (some...) other places of 
> this document, such as the 'Sample Boot Configuration' chapter.

The problem is that reStructuredText syntax requires that indentation.
I may follow the rules after the rST requirements, though.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[Andy Shevchenko]

On Mon, Nov 25, 2024 at 12:46:25PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 25, 2024 at 09:45:36AM +0100, Ingo Molnar wrote:
> > * Randy Dunlap <rdunlap@infradead.org> wrote:
> > > On 11/25/24 12:31 AM, Andy Shevchenko wrote:

...

> > > > -	if (relocatable_kernel)
> > > > -	runtime_start = align_up(load_address, kernel_alignment)
> > > > -	else
> > > > -	runtime_start = pref_address
> > > > +    if ( relocatable_kernel ) {
> > > > +      if ( load_address < pref_address )
> > > 
> > > What's up with the extra spaces around ( and ) ... and inconsistent with
> > > the lines below?
> 
> I can remove them. This file has a lot of inconsistencies it seems...
> 
> > Also, even pseudocode should follow the kernel's coding style and use 
> > tabs in particular - which it already does in (some...) other places of 
> > this document, such as the 'Sample Boot Configuration' chapter.
> 
> The problem is that reStructuredText syntax requires that indentation.
> I may follow the rules after the rST requirements, though.

v2 has just been sent:
https://lore.kernel.org/r/20241125105005.1616154-1-andriy.shevchenko@linux.intel.com

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[Ingo Molnar]

* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> On Mon, Nov 25, 2024 at 09:45:36AM +0100, Ingo Molnar wrote:
> > * Randy Dunlap <rdunlap@infradead.org> wrote:
> > > On 11/25/24 12:31 AM, Andy Shevchenko wrote:
> 
> ...
> 
> > > > -	if (relocatable_kernel)
> > > > -	runtime_start = align_up(load_address, kernel_alignment)
> > > > -	else
> > > > -	runtime_start = pref_address
> > > > +    if ( relocatable_kernel ) {
> > > > +      if ( load_address < pref_address )
> > > 
> > > What's up with the extra spaces around ( and ) ... and inconsistent with
> > > the lines below?
> 
> I can remove them. This file has a lot of inconsistencies it seems...

Feel free to send a followup patch that fixes up all of those other 
details and harmonizes the style. Quality of the boot protocol 
documentation demonstrably matters quite a bit in functional terms as 
well ...

> 
> > Also, even pseudocode should follow the kernel's coding style and use 
> > tabs in particular - which it already does in (some...) other places of 
> > this document, such as the 'Sample Boot Configuration' chapter.
> 
> The problem is that reStructuredText syntax requires that indentation.
> I may follow the rules after the rST requirements, though.

That's a good solution - thank you!

	Ingo

Re: [PATCH v1 1/1] x86/Documentation: Update algo in init_size description of boot protocol

[H. Peter Anvin]

On November 25, 2024 12:43:37 PM PST, Ingo Molnar <mingo@kernel.org> wrote:
>
>* Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
>
>> On Mon, Nov 25, 2024 at 09:45:36AM +0100, Ingo Molnar wrote:
>> > * Randy Dunlap <rdunlap@infradead.org> wrote:
>> > > On 11/25/24 12:31 AM, Andy Shevchenko wrote:
>> 
>> ...
>> 
>> > > > -	if (relocatable_kernel)
>> > > > -	runtime_start = align_up(load_address, kernel_alignment)
>> > > > -	else
>> > > > -	runtime_start = pref_address
>> > > > +    if ( relocatable_kernel ) {
>> > > > +      if ( load_address < pref_address )
>> > > 
>> > > What's up with the extra spaces around ( and ) ... and inconsistent with
>> > > the lines below?
>> 
>> I can remove them. This file has a lot of inconsistencies it seems...
>
>Feel free to send a followup patch that fixes up all of those other 
>details and harmonizes the style. Quality of the boot protocol 
>documentation demonstrably matters quite a bit in functional terms as 
>well ...
>
>> 
>> > Also, even pseudocode should follow the kernel's coding style and use 
>> > tabs in particular - which it already does in (some...) other places of 
>> > this document, such as the 'Sample Boot Configuration' chapter.
>> 
>> The problem is that reStructuredText syntax requires that indentation.
>> I may follow the rules after the rST requirements, though.
>
>That's a good solution - thank you!
>
>	Ingo

I, too, would really appreciate help in cleaning up the document and yes, the number of inconsistencies that have crept in over the years is quite frankly embarrassing. In a few places I will admit to thinking "did I actually write this?"

And as Ingo says, it matters, because experience shows that boot loader authors don't ask for clarification, they make their own interpretation and now we are stuck with a legacy. 

One part of the problem is that reviewing documentation changes in patch form hides inconsistencies because you only get a few lines of context.

On top of that, it would be great if the markup language could be used to insert rationale and commentary into the document. In some cases I think it would really help getting better compliance with the spec as intended, explain why we make certain recommendations as opposed to "well it seems to work", and help catch inconsistencies.