[PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[Khalid Ali]

From: Khalid Ali <khaliidcaliy@gmail.com>

The current kernel entry point takes one argument which is boot_param
from RSI. The only argument entry point recieves is pointer to
boot_param.

In order to comply with the ABI calling convension the entry point must
recieve the boot_param from RDI instead of RSI. There were no specific
use case used for RDI, so the kernel can safely recieve argument from
that register to better comply with ABI.

This patch makes the kernel to recieve boot_param which is the only
argument it recieves, from RDI instead of RSI. All changes needed for
stability and clarity have being changed.

Changelog:
 * Kernel uncompressed entry point expects boot_param from RDI instead
   of RSI.
 * The decompressor has being adjusted to supply argument from RDI
   instead RSI.
 * libstub has being adjusted to supply argument from RDI instead of RSI.

After throughly tested there were no regression and UDs has being
observed. Looking forward for feedback.

 arch/x86/boot/compressed/head_64.S      | 2 +-
 arch/x86/kernel/head_64.S               | 4 ++--
 drivers/firmware/efi/libstub/x86-stub.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.49.0

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[Brian Gerst]

On Mon, Jun 23, 2025 at 4:16 AM Khalid Ali <khaliidcaliy@gmail.com> wrote:
>
> From: Khalid Ali <khaliidcaliy@gmail.com>
>
> The current kernel entry point takes one argument which is boot_param
> from RSI. The only argument entry point recieves is pointer to
> boot_param.
>
> In order to comply with the ABI calling convension the entry point must
> recieve the boot_param from RDI instead of RSI. There were no specific
> use case used for RDI, so the kernel can safely recieve argument from
> that register to better comply with ABI.
>
> This patch makes the kernel to recieve boot_param which is the only
> argument it recieves, from RDI instead of RSI. All changes needed for
> stability and clarity have being changed.
>
> Changelog:
>  * Kernel uncompressed entry point expects boot_param from RDI instead
>    of RSI.
>  * The decompressor has being adjusted to supply argument from RDI
>    instead RSI.
>  * libstub has being adjusted to supply argument from RDI instead of RSI.
>
> After throughly tested there were no regression and UDs has being
> observed. Looking forward for feedback.
>
>  arch/x86/boot/compressed/head_64.S      | 2 +-
>  arch/x86/kernel/head_64.S               | 4 ++--
>  drivers/firmware/efi/libstub/x86-stub.c | 4 ++--
>  3 files changed, 5 insertions(+), 5 deletions(-)

This was never intended to conform to the C ABI, why is it necessary
to change it?

Also, you cannot break this up into three patches.  Every patch must
be fully functional so that git bisect will work.


Brian Gerst

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[Khalid Ali]

> This was never intended to conform to the C ABI, why is it necessary
> to change it?

Technically speaking, you are right, however that doesn't mean we can put something where
ever we like. We came from C code which is bootloader and we end up to C code, so we should 
comply the ABI here too.

> Also, you cannot break this up into three patches.  Every patch must
> be fully functional so that git bisect will work.

Thanks for the tip. I will do next time.

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[H. Peter Anvin]

On 2025-06-23 09:29, Khalid Ali wrote:
>> This was never intended to conform to the C ABI, why is it necessary
>> to change it?
> 
> Technically speaking, you are right, however that doesn't mean we can put something where
> ever we like. We came from C code which is bootloader and we end up to C code, so we should
> comply the ABI here too.
> 

This is also invoked by some external bootloaders that boot the ELF 
image directly, even though this is strongly discouraged.

Therefore this patchset is NAKed with extreme prejudice.

	-hpa

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[Khalid Ali]

> This is also invoked by some external bootloaders that boot the ELF 
> image directly, even though this is strongly discouraged.
>
> Therefore this patchset is NAKed with extreme prejudice.

Thanks both of you peter and brian,

however, the boot protocol document saying "%rsi must hold the base address of the struct boot_params",
it doesn't mention why. Maybe the document needs update to justify the reasons. I wouldn't have known it 
if you didn't tell me, so this shouldn't confuse anyone else.

Thanks 
Khalid Ali

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[Brian Gerst]

On Mon, Jun 23, 2025 at 2:40 PM Khalid Ali <khaliidcaliy@gmail.com> wrote:
>
> > This is also invoked by some external bootloaders that boot the ELF
> > image directly, even though this is strongly discouraged.
> >
> > Therefore this patchset is NAKed with extreme prejudice.
>
> Thanks both of you peter and brian,
>
> however, the boot protocol document saying "%rsi must hold the base address of the struct boot_params",
> it doesn't mention why. Maybe the document needs update to justify the reasons. I wouldn't have known it
> if you didn't tell me, so this shouldn't confuse anyone else.

The use of RSI was inherited from the 32-bit kernel, but the real
reason is lost to history.  It's just always been that way and there
is no compelling reason to change it.


Brian Gerst

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[H. Peter Anvin]

On June 23, 2025 12:24:50 PM PDT, Brian Gerst <brgerst@gmail.com> wrote:
>On Mon, Jun 23, 2025 at 2:40 PM Khalid Ali <khaliidcaliy@gmail.com> wrote:
>>
>> > This is also invoked by some external bootloaders that boot the ELF
>> > image directly, even though this is strongly discouraged.
>> >
>> > Therefore this patchset is NAKed with extreme prejudice.
>>
>> Thanks both of you peter and brian,
>>
>> however, the boot protocol document saying "%rsi must hold the base address of the struct boot_params",
>> it doesn't mention why. Maybe the document needs update to justify the reasons. I wouldn't have known it
>> if you didn't tell me, so this shouldn't confuse anyone else.
>
>The use of RSI was inherited from the 32-bit kernel, but the real
>reason is lost to history.  It's just always been that way and there
>is no compelling reason to change it.
>
>
>Brian Gerst
>

The reason isn't lost to history: I picked %esi because I found that none of the weird bootloaders which looked the protected mode jump clobbered that particular register.

Re: [PATCH v1 0/3] x86/boot: Supply boot_param in rdi instead of rsi from startup_64()

[H. Peter Anvin]

On June 23, 2025 11:39:09 AM PDT, Khalid Ali <khaliidcaliy@gmail.com> wrote:
>> This is also invoked by some external bootloaders that boot the ELF 
>> image directly, even though this is strongly discouraged.
>>
>> Therefore this patchset is NAKed with extreme prejudice.
>
>Thanks both of you peter and brian,
>
>however, the boot protocol document saying "%rsi must hold the base address of the struct boot_params",
>it doesn't mention why. Maybe the document needs update to justify the reasons. I wouldn't have known it 
>if you didn't tell me, so this shouldn't confuse anyone else.
>
>Thanks 
>Khalid Ali

It is a *protocol*. An interface. "Because the interface specification says so" is really all the justification you need; otherwise you have to hunt down *every* user of the interface and verify your change.